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

[robbieko <robbieko@synology.com>]

Nikolay Borisov 於 2019-03-25 19:18 寫到:
> 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?
> 

Before my patch, whether it is space_cache v1/v2 or nospace_cache,
when updating the block group, it will check whether the existing
free space inode has space and release the space in cache_save_setup.

But my patch will skip lookup_free space_inode, create_free_space inode,
and truncate_free_space_cache, so I have to make sure that no
free space_cache=v1 has been used, otherwise the free space inode
space will not be freed.

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