Re: [PATCH] btrfs: clear free space cache when nospace_cache mount option is specified

[Josef Bacik <josef@toxicpanda.com>]

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