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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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
>