* [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used
@ 2019-03-25 9:06 robbieko
2019-03-25 9:35 ` Nikolay Borisov
0 siblings, 1 reply; 5+ messages in thread
From: robbieko @ 2019-03-25 9:06 UTC (permalink / raw)
To: linux-btrfs; +Cc: Robbie Ko
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;
+ }
+
again:
inode = lookup_free_space_inode(fs_info, block_group, path);
if (IS_ERR(inode) && PTR_ERR(inode) != -ENOENT) {
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used
2019-03-25 9:06 [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used robbieko
@ 2019-03-25 9:35 ` Nikolay Borisov
2019-03-25 9:52 ` robbieko
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2019-03-25 9:35 UTC (permalink / raw)
To: robbieko, linux-btrfs
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) {
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used
2019-03-25 9:35 ` Nikolay Borisov
@ 2019-03-25 9:52 ` robbieko
2019-03-25 11:18 ` Nikolay Borisov
0 siblings, 1 reply; 5+ messages in thread
From: robbieko @ 2019-03-25 9:52 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
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) {
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used
2019-03-25 9:52 ` robbieko
@ 2019-03-25 11:18 ` Nikolay Borisov
2019-03-26 1:59 ` robbieko
0 siblings, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2019-03-25 11:18 UTC (permalink / raw)
To: robbieko; +Cc: linux-btrfs
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) {
>>>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used
2019-03-25 11:18 ` Nikolay Borisov
@ 2019-03-26 1:59 ` robbieko
0 siblings, 0 replies; 5+ messages in thread
From: robbieko @ 2019-03-26 1:59 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs, linux-btrfs-owner
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) {
>>>>
>>
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-26 1:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-25 9:06 [PATCH] Btrfs: improve unnecessary free space inode update when space_cache=v1 has never been used robbieko
2019-03-25 9:35 ` Nikolay Borisov
2019-03-25 9:52 ` robbieko
2019-03-25 11:18 ` Nikolay Borisov
2019-03-26 1:59 ` robbieko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).