Re: [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used

[Nikolay Borisov <nborisov@suse.com>]

On 25.03.19 г. 11:52 ч., robbieko wrote:
> Nikolay Borisov 於 2019-03-25 17:35 寫到:
>> On 25.03.19 г. 11:06 ч., robbieko wrote:
>>> From: Robbie Ko <robbieko@synology.com>
>>>
>>> When we have never used space_cache=v1, we don't need to update
>>> the free space inode, and try to free up the space occupied by the
>>> free space inode.
>>>
>>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>>> ---
>>>  fs/btrfs/extent-tree.c | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 1d49694..d3bba07 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -3403,6 +3403,13 @@ static int cache_save_setup(struct
>>> btrfs_block_group_cache *block_group,
>>>
>>>      if (trans->aborted)
>>>          return 0;
>>> +
>>> +    if (btrfs_super_cache_generation(fs_info->super_copy) == (u64)-1 &&
>>> +        !btrfs_test_opt(fs_info, SPACE_CACHE)) {
>>> +        dcs = BTRFS_DC_WRITTEN;
>>> +        goto out;
>>> +    }
>>> +
>>
>> Looking at the code it seems this function can only be called from
>> btrfs_write_dirty_block_groups, since its other caller
>> btrfs_setup_space_cache already checks if SPACE_CACHE option is used.
>>
>> IMO this patch is fine if we aren't using v1 space cache no point in
>> executing that function however.
>>
>> However, why is it necessary to do the cache_generation check, isn't it
>> sufficient to predicate the execution only on SPACE_CACHE mount option?
>>
> 
> When superblock cache_generation != -1,
> it means that space_cache=v1 has been used.
> 
> If we have used space_cache=v1 before,
> we will generate a free space cache inode,
> which will take up space, so we must free up
> the space occupied by the free space inode.

But if we have used space_cache=v1 before and now using it in the
current mount then should we even care about the freespace inode? I
think the answer is "no". Furthermore I don't see how freeing the space
cache has anything to do with the code you are modifying. Can you elaborate?

> 
> Thanks.
> 
>>>  again:
>>>      inode = lookup_free_space_inode(fs_info, block_group, path);
>>>      if (IS_ERR(inode) && PTR_ERR(inode) != -ENOENT) {
>>>
> 
>