| Message ID | a682e48c161eece38f8d803103068fed5959537d.1729665365.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] btrfs: fix mount failure due to remount races | expand |
On Wed, Oct 23, 2024 at 05:08:51PM +1030, Qu Wenruo wrote: > + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > + ret = btrfs_reconfigure(fc); This gives me a warning (gcc 13.3.0): fs/btrfs/super.c: In function ‘btrfs_reconfigure_for_mount’: fs/btrfs/super.c:2011:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses] 2011 | if (!fc->oldapi || !(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
在 2024/10/24 03:02, David Sterba 写道: > On Wed, Oct 23, 2024 at 05:08:51PM +1030, Qu Wenruo wrote: >> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >> + ret = btrfs_reconfigure(fc); > > This gives me a warning (gcc 13.3.0): > > fs/btrfs/super.c: In function ‘btrfs_reconfigure_for_mount’: > fs/btrfs/super.c:2011:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses] > 2011 | if (!fc->oldapi || !(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > Weird, my local patch/branch shows no fc->oldapi usage, thus no warning. The involved lines are: - ret = btrfs_reconfigure(fc); + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) + ret = btrfs_reconfigure(fc); Thus no warning. Thanks, Qu
On Thu, Oct 24, 2024 at 07:23:20AM +1030, Qu Wenruo wrote: > > > 在 2024/10/24 03:02, David Sterba 写道: > > On Wed, Oct 23, 2024 at 05:08:51PM +1030, Qu Wenruo wrote: > >> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > >> + ret = btrfs_reconfigure(fc); > > > > This gives me a warning (gcc 13.3.0): > > > > fs/btrfs/super.c: In function ‘btrfs_reconfigure_for_mount’: > > fs/btrfs/super.c:2011:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses] > > 2011 | if (!fc->oldapi || !(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > Weird, my local patch/branch shows no fc->oldapi usage, thus no warning. > > The involved lines are: > > - ret = btrfs_reconfigure(fc); > + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > + ret = btrfs_reconfigure(fc); > > Thus no warning. This was caused on my side. The patch does not apply cleanly on for-next or my misc-next, I'm using wiggle to merge the changes and it for some reason always adds the fc->oldapi to the conditions. Please refresh the patch and resend, thanks.
在 2024/10/26 05:32, David Sterba 写道: > On Thu, Oct 24, 2024 at 07:23:20AM +1030, Qu Wenruo wrote: >> >> >> 在 2024/10/24 03:02, David Sterba 写道: >>> On Wed, Oct 23, 2024 at 05:08:51PM +1030, Qu Wenruo wrote: >>>> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >>>> + ret = btrfs_reconfigure(fc); >>> >>> This gives me a warning (gcc 13.3.0): >>> >>> fs/btrfs/super.c: In function ‘btrfs_reconfigure_for_mount’: >>> fs/btrfs/super.c:2011:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses] >>> 2011 | if (!fc->oldapi || !(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> >>> >> Weird, my local patch/branch shows no fc->oldapi usage, thus no warning. >> >> The involved lines are: >> >> - ret = btrfs_reconfigure(fc); >> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >> + ret = btrfs_reconfigure(fc); >> >> Thus no warning. > > This was caused on my side. The patch does not apply cleanly on for-next > or my misc-next, I'm using wiggle to merge the changes and it for some > reason always adds the fc->oldapi to the conditions. Please refresh the > patch and resend, thanks. I have already mentioned this has a dependency on the following patch: https://lore.kernel.org/linux-btrfs/e1a70aa6dd0fc9ba6c7050a5befb3bd5b75a1377.1729664802.git.wqu@suse.com/ Furthermore that patch should have a higher priority, as it breaks the very basic of subvolume RO/RW mount. Thanks, Qu
On Sat, Oct 26, 2024 at 07:27:02AM +1030, Qu Wenruo wrote: > 在 2024/10/26 05:32, David Sterba 写道: > > On Thu, Oct 24, 2024 at 07:23:20AM +1030, Qu Wenruo wrote: > >> 在 2024/10/24 03:02, David Sterba 写道: > >>> On Wed, Oct 23, 2024 at 05:08:51PM +1030, Qu Wenruo wrote: > >>>> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > >>>> + ret = btrfs_reconfigure(fc); > >>> > >>> This gives me a warning (gcc 13.3.0): > >>> > >>> fs/btrfs/super.c: In function ‘btrfs_reconfigure_for_mount’: > >>> fs/btrfs/super.c:2011:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses] > >>> 2011 | if (!fc->oldapi || !(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > >>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>> > >> Weird, my local patch/branch shows no fc->oldapi usage, thus no warning. > >> > >> The involved lines are: > >> > >> - ret = btrfs_reconfigure(fc); > >> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) > >> + ret = btrfs_reconfigure(fc); > >> > >> Thus no warning. > > > > This was caused on my side. The patch does not apply cleanly on for-next > > or my misc-next, I'm using wiggle to merge the changes and it for some > > reason always adds the fc->oldapi to the conditions. Please refresh the > > patch and resend, thanks. > > I have already mentioned this has a dependency on the following patch: > > https://lore.kernel.org/linux-btrfs/e1a70aa6dd0fc9ba6c7050a5befb3bd5b75a1377.1729664802.git.wqu@suse.com/ > > Furthermore that patch should have a higher priority, as it breaks the > very basic of subvolume RO/RW mount. Sorry, I have a hard time to keep up context for each patch, I've been sweeping mails for anything that we need to get to 6.13. Can you please resend patches that are related together in one series/thread, like the mount option fixes and the folio or subpage fixes? That would help to ensure I don't miss something. Thanks.
在 2024/10/30 10:29, David Sterba 写道: > On Sat, Oct 26, 2024 at 07:27:02AM +1030, Qu Wenruo wrote: >> 在 2024/10/26 05:32, David Sterba 写道: >>> On Thu, Oct 24, 2024 at 07:23:20AM +1030, Qu Wenruo wrote: >>>> 在 2024/10/24 03:02, David Sterba 写道: >>>>> On Wed, Oct 23, 2024 at 05:08:51PM +1030, Qu Wenruo wrote: >>>>>> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >>>>>> + ret = btrfs_reconfigure(fc); >>>>> >>>>> This gives me a warning (gcc 13.3.0): >>>>> >>>>> fs/btrfs/super.c: In function ‘btrfs_reconfigure_for_mount’: >>>>> fs/btrfs/super.c:2011:56: warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses] >>>>> 2011 | if (!fc->oldapi || !(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >>>>> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> >>>> Weird, my local patch/branch shows no fc->oldapi usage, thus no warning. >>>> >>>> The involved lines are: >>>> >>>> - ret = btrfs_reconfigure(fc); >>>> + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) >>>> + ret = btrfs_reconfigure(fc); >>>> >>>> Thus no warning. >>> >>> This was caused on my side. The patch does not apply cleanly on for-next >>> or my misc-next, I'm using wiggle to merge the changes and it for some >>> reason always adds the fc->oldapi to the conditions. Please refresh the >>> patch and resend, thanks. >> >> I have already mentioned this has a dependency on the following patch: >> >> https://lore.kernel.org/linux-btrfs/e1a70aa6dd0fc9ba6c7050a5befb3bd5b75a1377.1729664802.git.wqu@suse.com/ >> >> Furthermore that patch should have a higher priority, as it breaks the >> very basic of subvolume RO/RW mount. > > Sorry, I have a hard time to keep up context for each patch, I've been > sweeping mails for anything that we need to get to 6.13. Can you please > resend patches that are related together in one series/thread, like the > mount option fixes and the folio or subpage fixes? That would help to > ensure I don't miss something. Thanks. > Sure, will send out the mount fixes (just two patches) first. Then combine all the subpage related ones that are not yet merged into a patchset. Thanks, Qu
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index d77cce8d633e..d137ce2b5038 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1885,18 +1885,21 @@ static int btrfs_get_tree_super(struct fs_context *fc) if (sb->s_root) { btrfs_close_devices(fs_devices); - if ((fc->sb_flags ^ sb->s_flags) & SB_RDONLY) - ret = -EBUSY; + /* + * At this stage we may have RO flag mismatch between + * fc->sb_flags and sb->s_flags. + * Caller should detect such mismatch and reconfigure + * with sb->s_umount rwsem hold if needed. + */ } else { snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev); shrinker_debugfs_rename(sb->s_shrink, "sb-btrfs:%s", sb->s_id); btrfs_sb(sb)->bdev_holder = &btrfs_fs_type; ret = btrfs_fill_super(sb, fs_devices); - } - - if (ret) { - deactivate_locked_super(sb); - return ret; + if (ret) { + deactivate_locked_super(sb); + return ret; + } } btrfs_clear_oneshot_options(fs_info); @@ -1983,39 +1986,18 @@ static int btrfs_get_tree_super(struct fs_context *fc) * * So here we always needs the remount hack to support per-subvolume RO/RW flags. */ -static struct vfsmount *btrfs_reconfigure_for_mount(struct fs_context *fc) +static int btrfs_reconfigure_for_mount(struct fs_context *fc, struct vfsmount *mnt) { - struct vfsmount *mnt; - int ret; - const bool ro2rw = !(fc->sb_flags & SB_RDONLY); + int ret = 0; - /* - * We got an EBUSY because our SB_RDONLY flag didn't match the existing - * super block, so invert our setting here and retry the mount so we - * can get our vfsmount. - */ - if (ro2rw) - fc->sb_flags |= SB_RDONLY; - else - fc->sb_flags &= ~SB_RDONLY; + if (fc->sb_flags & SB_RDONLY) + return ret; - mnt = fc_mount(fc); - if (IS_ERR(mnt)) - return mnt; - - if (!ro2rw) - return mnt; - - /* We need to convert to rw, call reconfigure. */ - fc->sb_flags &= ~SB_RDONLY; down_write(&mnt->mnt_sb->s_umount); - ret = btrfs_reconfigure(fc); + if (!(fc->sb_flags & SB_RDONLY) && (mnt->mnt_sb->s_flags & SB_RDONLY)) + ret = btrfs_reconfigure(fc); up_write(&mnt->mnt_sb->s_umount); - if (ret) { - mntput(mnt); - return ERR_PTR(ret); - } - return mnt; + return ret; } static int btrfs_get_tree_subvol(struct fs_context *fc) @@ -2025,6 +2007,7 @@ static int btrfs_get_tree_subvol(struct fs_context *fc) struct fs_context *dup_fc; struct dentry *dentry; struct vfsmount *mnt; + int ret = 0; /* * Setup a dummy root and fs_info for test/set super. This is because @@ -2067,11 +2050,16 @@ static int btrfs_get_tree_subvol(struct fs_context *fc) fc->security = NULL; mnt = fc_mount(dup_fc); - if (PTR_ERR_OR_ZERO(mnt) == -EBUSY) - mnt = btrfs_reconfigure_for_mount(dup_fc); - put_fs_context(dup_fc); - if (IS_ERR(mnt)) + if (IS_ERR(mnt)) { + put_fs_context(dup_fc); return PTR_ERR(mnt); + } + ret = btrfs_reconfigure_for_mount(dup_fc, mnt); + put_fs_context(dup_fc); + if (ret) { + mntput(mnt); + return ret; + } /* * This free's ->subvol_name, because if it isn't set we have to
[BUG] The following reproducer can cause btrfs mount to fail: dev="/dev/test/scratch1" mnt1="/mnt/test" mnt2="/mnt/scratch" mkfs.btrfs -f $dev mount $dev $mnt1 btrfs subvolume create $mnt1/subvol1 btrfs subvolume create $mnt1/subvol2 umount $mnt1 mount $dev $mnt1 -o subvol=subvol1 while mount -o remount,ro $mnt1; do mount -o remount,rw $mnt1; done & bg=$! while mount $dev $mnt2 -o subvol=subvol2; do umount $mnt2; done kill $bg wait umount -R $mnt1 umount -R $mnt2 The script will fail with the following error: mount: /mnt/scratch: /dev/mapper/test-scratch1 already mounted on /mnt/test. dmesg(1) may have more information after failed mount system call. umount: /mnt/test: target is busy. umount: /mnt/scratch/: not mounted And there is no kernel error message. [CAUSE] During the btrfs mount, to support mounting different subvolumes with different RO/RW flags, we have a small hack during the mount: Retry with matching RO flags if the initial mount fail with -EBUSY. The problem is, during that retry we do not hold any super block lock (s_umount), this means there can be another remount process changing the RO flags of the original fs super block. If so, we can have an EBUSY error again during retry. And this time we treat any failure as an error, without any retry and cause the above EBUSY mount failure. [FIX] The current retry behavior is racy because we do not have a super block thus no way to hold s_umount to prevent the race with remount. Solve the root problem by allowing fc->sb_flags to mismatch from the sb->s_flags at btrfs_get_tree_super(). Then at the re-entry point btrfs_get_tree_subvol(), manually check the fc->s_flags against sb->s_flags, if it's a RO->RW mismatch, then reconfigure with s_umount lock hold. This will prevent other remount process to change the flag, and we can reconfigure the fs race-free. Fixes: f044b318675f ("btrfs: handle the ro->rw transition for mounting different subvolumes") Reported-by: Enno Gotthold <egotthold@suse.com> Reported-by: Fabian Vogt <fvogt@suse.com> [ Special thanks for the reproducer and early analyze pointing to btrfs ] Link: https://bugzilla.suse.com/show_bug.cgi?id=1231836 Signed-off-by: Qu Wenruo <wqu@suse.com> --- Changelog: v2: - Instead of brute force retry-until-success-or-failure, allow sb flags mismatch during btrfs_get_tree_super() So that we can utilize sb->s_umount to prevent race. - Rebased after patch "btrfs: fix per-subvolume RO/RW flags with new mount API" Fortunately or unfortunately my distro is already utilizing the new mount API and I found the per-subvolume RO/RW support is broken again. Unlike the original expectation, the new API is not the new hope, but only allows the old bug to strike back. So this patch can only be applied after that fix. --- fs/btrfs/super.c | 66 ++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 39 deletions(-)