diff mbox series

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

Message ID 1553504798-8116-1-git-send-email-robbieko@synology.com (mailing list archive)
State New, archived
Headers show
Series Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used | expand

Commit Message

robbieko March 25, 2019, 9:06 a.m. UTC 
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(+)

Comments

Nikolay Borisov March 25, 2019, 9:35 a.m. UTC | #1 
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?

>  again:
>  	inode = lookup_free_space_inode(fs_info, block_group, path);
>  	if (IS_ERR(inode) && PTR_ERR(inode) != -ENOENT) {
>
robbieko March 25, 2019, 9:52 a.m. UTC | #2 
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) {
>>
Nikolay Borisov March 25, 2019, 11:18 a.m. UTC | #3 
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) {
>>>
> 
>
robbieko March 26, 2019, 1:59 a.m. UTC | #4 
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) {
>>>> 
>> 
>>
diff mbox series

Patch

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;
+	}
+
 again:
 	inode = lookup_free_space_inode(fs_info, block_group, path);
 	if (IS_ERR(inode) && PTR_ERR(inode) != -ENOENT) {