On Sat, Apr 02, 2022 at 06:57:58AM +0800, Qu Wenruo wrote:
> >> index d456f426924c..34eb6d4b904a 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -3675,6 +3675,17 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >> if (sectorsize < PAGE_SIZE) {
> >> struct btrfs_subpage_info *subpage_info;
> >>
> >> + /*
> >> + * V1 space cache has some hardcoded PAGE_SIZE usage, and is
> >> + * going to be deprecated.
> >> + *
> >> + * Force to use v2 cache for subpage case.
> >> + */
> >> + btrfs_clear_opt(fs_info->mount_opt, SPACE_CACHE);
> >> + btrfs_set_and_info(fs_info, FREE_SPACE_TREE,
> >> + "forcing free space tree for sector size %u with page size %lu",
> >> + sectorsize, PAGE_SIZE);
> >
> > I'm not sure this is implemented the right way. Why is it unconditional?
>
> Isn't the same thing we do when parsing the mount options for switch
> v1->v2 cache?
The mount code checks if the v1 is active ie. has some existing
structures and then clears v1.
>
> > Does any subsequent mount have to clear and set the bits after it has
> > been already? Or what if the free space tree is set at mkfs time, which
> > is now the default.
>
> The function btrfs_set_and_info() will only inform the end users if the
> bit is not set.
>
> >
> > Next, remounting v1->v2 does more things, like removing the v1 tree if
> > it exists. And due to some bugs there are more bits for free space tree
> > to be set like FREE_SPACE_TREE_VALID. So I don't thing this patch
> > covers all cases for the v2.
>
> You're right on remounting, but in the opposite way.
> There is nothing prevent the users from re-enabling v1 cache.
>
> I should also prevent user from setting v1 cache.
Good point, for the subpage case yes, but otherwise not though it's
a valid operation it does not make much sense.
> Another concern is, I didn't see code cleaning up v1 cache when we do
> the v1->v2 switch.
> The only code doing such cleanup is cleanup_free_space_cache_v1() in
> free-space-cache.c, but it only gets called in
> btrfs_set_free_space_cache_v1_active().
>
> Or did I miss something?
No, that's the code I had in mind, and clearing the v1 code once v2 is
activated has been added because it was not there before.
