| Message ID | 20210514020308.3824607-1-naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: zoned: disable space cache using proper function | expand |
On 2021/5/14 上午10:03, Naohiro Aota wrote: > As btrfs_set_free_space_cache_v1_active() is introduced, this patch uses > it to disable space cache v1 properly. > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > --- > fs/btrfs/super.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 4a396c1147f1..89ffc17d074c 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -592,7 +592,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > if (btrfs_is_zoned(info)) { > btrfs_info(info, > "zoned: clearing existing space cache"); > - btrfs_set_super_cache_generation(info->super_copy, 0); > + btrfs_set_free_space_cache_v1_active(info, false); Can we have a better naming? The set_..._active() really looks like to *enable* space cache, other than disabling it. Thanks, Qu > } else { > btrfs_set_opt(info->mount_opt, SPACE_CACHE); > } >
On Fri, May 14, 2021 at 11:03:08AM +0900, Naohiro Aota wrote: > As btrfs_set_free_space_cache_v1_active() is introduced, this patch uses > it to disable space cache v1 properly. Can you please describe what problem is it fixing, of if it's a problem at all? The two functions do have quite different effects, resetting generation is simple but the new function starts transaction and iterates over all space inodes. That's beyond obvious so this needs an explanation. Thanks.
On Fri, May 14, 2021 at 06:05:05PM +0800, Qu Wenruo wrote: > > > On 2021/5/14 上午10:03, Naohiro Aota wrote: > > As btrfs_set_free_space_cache_v1_active() is introduced, this patch uses > > it to disable space cache v1 properly. > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > --- > > fs/btrfs/super.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > index 4a396c1147f1..89ffc17d074c 100644 > > --- a/fs/btrfs/super.c > > +++ b/fs/btrfs/super.c > > @@ -592,7 +592,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > > if (btrfs_is_zoned(info)) { > > btrfs_info(info, > > "zoned: clearing existing space cache"); > > - btrfs_set_super_cache_generation(info->super_copy, 0); > > + btrfs_set_free_space_cache_v1_active(info, false); > > Can we have a better naming? > > The set_..._active() really looks like to *enable* space cache, other > than disabling it. Agreed, it's really confusing and does the opposite of the name, this needs fixing.
On Fri, May 14, 2021 at 05:30:49PM +0200, David Sterba wrote: > On Fri, May 14, 2021 at 06:05:05PM +0800, Qu Wenruo wrote: > > > > > > On 2021/5/14 上午10:03, Naohiro Aota wrote: > > > As btrfs_set_free_space_cache_v1_active() is introduced, this patch uses > > > it to disable space cache v1 properly. > > > > > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > > > --- > > > fs/btrfs/super.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > > index 4a396c1147f1..89ffc17d074c 100644 > > > --- a/fs/btrfs/super.c > > > +++ b/fs/btrfs/super.c > > > @@ -592,7 +592,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, > > > if (btrfs_is_zoned(info)) { > > > btrfs_info(info, > > > "zoned: clearing existing space cache"); > > > - btrfs_set_super_cache_generation(info->super_copy, 0); > > > + btrfs_set_free_space_cache_v1_active(info, false); > > > > Can we have a better naming? > > > > The set_..._active() really looks like to *enable* space cache, other > > than disabling it. > > Agreed, it's really confusing and does the opposite of the name, this > needs fixing. Sure. I'll add btrfs_{enable/disable}_free_space_cache_v1() as a prep patch. Thanks,
On Fri, May 14, 2021 at 05:27:42PM +0200, David Sterba wrote: > On Fri, May 14, 2021 at 11:03:08AM +0900, Naohiro Aota wrote: > > As btrfs_set_free_space_cache_v1_active() is introduced, this patch uses > > it to disable space cache v1 properly. > > Can you please describe what problem is it fixing, of if it's a problem > at all? The two functions do have quite different effects, resetting > generation is simple but the new function starts transaction and > iterates over all space inodes. That's beyond obvious so this needs an > explanation. Thanks. Sure. I'll rework the commit message to tell we want "cache_generation > 0 iff the file system is using space_cache v1." Thanks,
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 4a396c1147f1..89ffc17d074c 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -592,7 +592,7 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options, if (btrfs_is_zoned(info)) { btrfs_info(info, "zoned: clearing existing space cache"); - btrfs_set_super_cache_generation(info->super_copy, 0); + btrfs_set_free_space_cache_v1_active(info, false); } else { btrfs_set_opt(info->mount_opt, SPACE_CACHE); }
As btrfs_set_free_space_cache_v1_active() is introduced, this patch uses it to disable space cache v1 properly. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)