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.