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 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.

Thanks.

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