| Message ID | 20211102084242.30581-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: clear free space cache when nospace_cache mount option is specified | expand |
On Tue, Nov 02, 2021 at 04:42:42PM +0800, Qu Wenruo wrote: > [BUG] > With latest btrfs-progs v5.15.x testing branch, fstests/btrfs/215 will > fail like the following: > > MKFS_OPTIONS -- /dev/mapper/test-scratch1 > MOUNT_OPTIONS -- /dev/mapper/test-scratch1 /mnt/scratch > > btrfs/215 0s ... [failed, exit status 1]- output mismatch (see ~/xfstests-dev/results//btrfs/215.out.bad) > --- tests/btrfs/215.out 2020-11-03 15:05:07.920000001 +0800 > +++ ~/xfstests-dev/results//btrfs/215.out.bad 2021-11-02 16:31:17.626666667 +0800 > @@ -1,2 +1,4 @@ > QA output created by 215 > -Silence is golden > +mount: /mnt/scratch: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch1, missing codepage or helper program, or other error. > +mount -o nospace_cache /dev/mapper/test-scratch1 /mnt/scratch failed > +(see ~/xfstests-dev/results//btrfs/215.full for details) > ... > (Run 'diff -u ~/xfstests-dev/tests/btrfs/215.out ~/xfstests-dev/results//btrfs/215.out.bad' to see the entire diff) > Ran: btrfs/215 > > [CAUSE] > Currently btrfs doesn't allow mounting with nospace_cache when there is > already a v2 cache. > > The logic looks like this, in btrfs_parse_options(): > > case Opt_no_space_cache: > if (btrfs_test_opt(info, FREE_SPACE_TREE)) { > btrfs_set_opt(info->mount_opt, CLEAR_CACHE); > btrfs_clear_and_info(info, FREE_SPACE_TREE, > "disabling and clearing free space tree"); > } > break; > > Then at the end of the same function: > > if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) && > !btrfs_test_opt(info, FREE_SPACE_TREE) && > !btrfs_test_opt(info, CLEAR_CACHE)) { > btrfs_err(info, "cannot disable free space tree"); > ret = -EINVAL; > } > > Thus causing above mount failure. > > [FIX] > I'm not sure why we don't allow nospace_cache with v2 cache, my > assumption is we don't have generation check for v2 cache, thus if we > make v2 space cache get out of sync with the on-disk data, it can > corrupt the fs. > > That needs to be properly addressed, but for now, to allow nospace_cache > with v2 cache, we can simply force to clear v2 cache when nospace_cache > mount option is specified. > > Since we're here, also remove a unnecessary new line the v2 cache check > at the end btrfs_parse_options(). > > Reported-by: Wang Yugui <wangyugui@e16-tech.com> > Signed-off-by: Qu Wenruo <wqu@suse.com> I'd rather we just not allow nospace_cache with the free space tree. It was an option meant for the original space cache, not for the free space tree. I don't want to surprise clear the free space tree for an unrelated mount option. Thanks, Josef
On 2021/11/2 21:08, Josef Bacik wrote: > On Tue, Nov 02, 2021 at 04:42:42PM +0800, Qu Wenruo wrote: >> [BUG] >> With latest btrfs-progs v5.15.x testing branch, fstests/btrfs/215 will >> fail like the following: >> >> MKFS_OPTIONS -- /dev/mapper/test-scratch1 >> MOUNT_OPTIONS -- /dev/mapper/test-scratch1 /mnt/scratch >> >> btrfs/215 0s ... [failed, exit status 1]- output mismatch (see ~/xfstests-dev/results//btrfs/215.out.bad) >> --- tests/btrfs/215.out 2020-11-03 15:05:07.920000001 +0800 >> +++ ~/xfstests-dev/results//btrfs/215.out.bad 2021-11-02 16:31:17.626666667 +0800 >> @@ -1,2 +1,4 @@ >> QA output created by 215 >> -Silence is golden >> +mount: /mnt/scratch: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch1, missing codepage or helper program, or other error. >> +mount -o nospace_cache /dev/mapper/test-scratch1 /mnt/scratch failed >> +(see ~/xfstests-dev/results//btrfs/215.full for details) >> ... >> (Run 'diff -u ~/xfstests-dev/tests/btrfs/215.out ~/xfstests-dev/results//btrfs/215.out.bad' to see the entire diff) >> Ran: btrfs/215 >> >> [CAUSE] >> Currently btrfs doesn't allow mounting with nospace_cache when there is >> already a v2 cache. >> >> The logic looks like this, in btrfs_parse_options(): >> >> case Opt_no_space_cache: >> if (btrfs_test_opt(info, FREE_SPACE_TREE)) { >> btrfs_set_opt(info->mount_opt, CLEAR_CACHE); >> btrfs_clear_and_info(info, FREE_SPACE_TREE, >> "disabling and clearing free space tree"); >> } >> break; >> >> Then at the end of the same function: >> >> if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) && >> !btrfs_test_opt(info, FREE_SPACE_TREE) && >> !btrfs_test_opt(info, CLEAR_CACHE)) { >> btrfs_err(info, "cannot disable free space tree"); >> ret = -EINVAL; >> } >> >> Thus causing above mount failure. >> >> [FIX] >> I'm not sure why we don't allow nospace_cache with v2 cache, my >> assumption is we don't have generation check for v2 cache, thus if we >> make v2 space cache get out of sync with the on-disk data, it can >> corrupt the fs. >> >> That needs to be properly addressed, but for now, to allow nospace_cache >> with v2 cache, we can simply force to clear v2 cache when nospace_cache >> mount option is specified. >> >> Since we're here, also remove a unnecessary new line the v2 cache check >> at the end btrfs_parse_options(). >> >> Reported-by: Wang Yugui <wangyugui@e16-tech.com> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > I'd rather we just not allow nospace_cache with the free space tree. It was an > option meant for the original space cache, not for the free space tree. I don't > want to surprise clear the free space tree for an unrelated mount option. OK, then we need to update the test cases to use v2 cache. While I'm not sure fstests guys won't complain as they may want to run it on much older kernels. Thanks, Qu > Thanks, > > Josef >
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 537d90bf5d84..59e4a756cea6 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -887,8 +887,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, "disabling disk space caching"); } if (btrfs_test_opt(info, FREE_SPACE_TREE)) { + /* + * For v2 cache with nospace_cache mount option, + * v2 cache can get out of sync if not cleared. + * Thus here we set to clear v2 cache. + */ + btrfs_set_opt(info->mount_opt, CLEAR_CACHE); btrfs_clear_and_info(info, FREE_SPACE_TREE, - "disabling free space tree"); + "disabling and clearing free space tree"); } break; case Opt_inode_cache: @@ -1036,7 +1042,6 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, !btrfs_test_opt(info, CLEAR_CACHE)) { btrfs_err(info, "cannot disable free space tree"); ret = -EINVAL; - } if (!ret) ret = btrfs_check_mountopts_zoned(info);
[BUG] With latest btrfs-progs v5.15.x testing branch, fstests/btrfs/215 will fail like the following: MKFS_OPTIONS -- /dev/mapper/test-scratch1 MOUNT_OPTIONS -- /dev/mapper/test-scratch1 /mnt/scratch btrfs/215 0s ... [failed, exit status 1]- output mismatch (see ~/xfstests-dev/results//btrfs/215.out.bad) --- tests/btrfs/215.out 2020-11-03 15:05:07.920000001 +0800 +++ ~/xfstests-dev/results//btrfs/215.out.bad 2021-11-02 16:31:17.626666667 +0800 @@ -1,2 +1,4 @@ QA output created by 215 -Silence is golden +mount: /mnt/scratch: wrong fs type, bad option, bad superblock on /dev/mapper/test-scratch1, missing codepage or helper program, or other error. +mount -o nospace_cache /dev/mapper/test-scratch1 /mnt/scratch failed +(see ~/xfstests-dev/results//btrfs/215.full for details) ... (Run 'diff -u ~/xfstests-dev/tests/btrfs/215.out ~/xfstests-dev/results//btrfs/215.out.bad' to see the entire diff) Ran: btrfs/215 [CAUSE] Currently btrfs doesn't allow mounting with nospace_cache when there is already a v2 cache. The logic looks like this, in btrfs_parse_options(): case Opt_no_space_cache: if (btrfs_test_opt(info, FREE_SPACE_TREE)) { btrfs_set_opt(info->mount_opt, CLEAR_CACHE); btrfs_clear_and_info(info, FREE_SPACE_TREE, "disabling and clearing free space tree"); } break; Then at the end of the same function: if (btrfs_fs_compat_ro(info, FREE_SPACE_TREE) && !btrfs_test_opt(info, FREE_SPACE_TREE) && !btrfs_test_opt(info, CLEAR_CACHE)) { btrfs_err(info, "cannot disable free space tree"); ret = -EINVAL; } Thus causing above mount failure. [FIX] I'm not sure why we don't allow nospace_cache with v2 cache, my assumption is we don't have generation check for v2 cache, thus if we make v2 space cache get out of sync with the on-disk data, it can corrupt the fs. That needs to be properly addressed, but for now, to allow nospace_cache with v2 cache, we can simply force to clear v2 cache when nospace_cache mount option is specified. Since we're here, also remove a unnecessary new line the v2 cache check at the end btrfs_parse_options(). Reported-by: Wang Yugui <wangyugui@e16-tech.com> Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/super.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)