* [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