| Message ID | 20250331082347.1407151-1-neelx@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: zstd: add `zstd-fast` alias mount option for fast modes | expand |
在 2025/3/31 18:53, Daniel Vacek 写道: > Now that zstd fast compression levels are allowed with `compress=zstd:-N` > mount option, allow also specifying the same using the `compress=zstd-fast:N` > alias. > > Upstream zstd deprecated the negative levels in favor of the `zstd-fast` > label anyways so this is actually the preferred way now. And indeed it also > looks more human friendly. > > Signed-off-by: Daniel Vacek <neelx@suse.com> > --- > fs/btrfs/super.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 40709e2a44fce..c1bc8d4db440a 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -368,6 +368,16 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param) > btrfs_set_opt(ctx->mount_opt, COMPRESS); > btrfs_clear_opt(ctx->mount_opt, NODATACOW); > btrfs_clear_opt(ctx->mount_opt, NODATASUM); > + } else if (strncmp(param->string, "zstd-fast", 9) == 0) { > + ctx->compress_type = BTRFS_COMPRESS_ZSTD; > + ctx->compress_level = > + -btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD, > + param->string + 9 Can we just use some temporary variable to save the return value of btrfs_compress_str2level()? ); > + if (ctx->compress_level > 0) > + ctx->compress_level = -ctx->compress_level; This also means, if we pass something like "compress=zstd-fast:-9", it will still set the level to the correct -9. Not some weird double negative, which is good. But I'm also wondering, should we even allow minus value for "zstd-fast". > + btrfs_set_opt(ctx->mount_opt, COMPRESS); > + btrfs_clear_opt(ctx->mount_opt, NODATACOW); > + btrfs_clear_opt(ctx->mount_opt, NODATASUM); > } else if (strncmp(param->string, "zstd", 4) == 0) { > ctx->compress_type = BTRFS_COMPRESS_ZSTD; > ctx->compress_level = Another thing is, if we want to prefer using zstd-fast:9 other than zstd:-9, should we also change our compress handling in btrfs_show_options() to show zstd-fast:9 instead? Thanks, Qu
On Mon, 31 Mar 2025 at 10:49, Qu Wenruo <wqu@suse.com> wrote: > 在 2025/3/31 18:53, Daniel Vacek 写道: > > Now that zstd fast compression levels are allowed with `compress=zstd:-N` > > mount option, allow also specifying the same using the `compress=zstd-fast:N` > > alias. > > > > Upstream zstd deprecated the negative levels in favor of the `zstd-fast` > > label anyways so this is actually the preferred way now. And indeed it also > > looks more human friendly. > > > > Signed-off-by: Daniel Vacek <neelx@suse.com> > > --- > > fs/btrfs/super.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > index 40709e2a44fce..c1bc8d4db440a 100644 > > --- a/fs/btrfs/super.c > > +++ b/fs/btrfs/super.c > > @@ -368,6 +368,16 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param) > > btrfs_set_opt(ctx->mount_opt, COMPRESS); > > btrfs_clear_opt(ctx->mount_opt, NODATACOW); > > btrfs_clear_opt(ctx->mount_opt, NODATASUM); > > + } else if (strncmp(param->string, "zstd-fast", 9) == 0) { > > + ctx->compress_type = BTRFS_COMPRESS_ZSTD; > > + ctx->compress_level = > > + -btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD, > > + param->string + 9 > > Can we just use some temporary variable to save the return value of > btrfs_compress_str2level()? I don't see any added value. Isn't `ctx->compress_level` temporary enough at this point? Note that the FS is not mounted yet so there's no other consumer of `ctx`. Or did you mean just for readability? > ); > > + if (ctx->compress_level > 0) > > + ctx->compress_level = -ctx->compress_level; > > This also means, if we pass something like "compress=zstd-fast:-9", it > will still set the level to the correct -9. > > Not some weird double negative, which is good. > > But I'm also wondering, should we even allow minus value for "zstd-fast". It was meant as a safety in a sense that `s/zstd:-/zstd-fast:-/` still works the same. Hence it defines that "fast level -3 <===> fast level 3" (which is still level -3 in internal zstd representation). So if you mounted `compress=zstd-fast:-9` it's understood you actually meant `compress=zstd-fast:9` in the same way as if you did `compress=zstd:-9`. I thought this was solid. Or would you rather bail out with -EINVAL? > > + btrfs_set_opt(ctx->mount_opt, COMPRESS); > > + btrfs_clear_opt(ctx->mount_opt, NODATACOW); > > + btrfs_clear_opt(ctx->mount_opt, NODATASUM); > > } else if (strncmp(param->string, "zstd", 4) == 0) { > > ctx->compress_type = BTRFS_COMPRESS_ZSTD; > > ctx->compress_level = > > Another thing is, if we want to prefer using zstd-fast:9 other than > zstd:-9, should we also change our compress handling in > btrfs_show_options() to show zstd-fast:9 instead? At this point it's not about preference but about compatibility. I actually tested changing this but as a side-effect it also had an influence on `btrfs.compression` xattr (our `compression` property) which is rather an incompatible on-disk format change. Hence I falled back keeping it simple. Showing `zstd:-9` is still a valid output. --nX > Thanks, > Qu
在 2025/3/31 20:36, Daniel Vacek 写道: [...] >>> btrfs_set_opt(ctx->mount_opt, COMPRESS); >>> btrfs_clear_opt(ctx->mount_opt, NODATACOW); >>> btrfs_clear_opt(ctx->mount_opt, NODATASUM); >>> + } else if (strncmp(param->string, "zstd-fast", 9) == 0) { >>> + ctx->compress_type = BTRFS_COMPRESS_ZSTD; >>> + ctx->compress_level = >>> + -btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD, >>> + param->string + 9 >> >> Can we just use some temporary variable to save the return value of >> btrfs_compress_str2level()? > > I don't see any added value. Isn't `ctx->compress_level` temporary > enough at this point? Note that the FS is not mounted yet so there's > no other consumer of `ctx`. > > Or did you mean just for readability? Doing all the same checks and flipping the sign of ctx->compress_level is already cursed to me. Isn't something like this easier to understand? level = btrfs_compress_str2level(); if (level > 0) ctx->compress_level = -level; else ctx->compress_level = level; > >> ); >>> + if (ctx->compress_level > 0) >>> + ctx->compress_level = -ctx->compress_level; >> >> This also means, if we pass something like "compress=zstd-fast:-9", it >> will still set the level to the correct -9. >> >> Not some weird double negative, which is good. >> >> But I'm also wondering, should we even allow minus value for "zstd-fast". > > It was meant as a safety in a sense that `s/zstd:-/zstd-fast:-/` still > works the same. Hence it defines that "fast level -3 <===> fast level > 3" (which is still level -3 in internal zstd representation). > So if you mounted `compress=zstd-fast:-9` it's understood you actually > meant `compress=zstd-fast:9` in the same way as if you did > `compress=zstd:-9`. > > I thought this was solid. Or would you rather bail out with -EINVAL? I prefer to bail out with -EINVAL, but it's only my personal choice. You'd better wait for feedbacks from other people. Thanks, Qu> >>> + btrfs_set_opt(ctx->mount_opt, COMPRESS); >>> + btrfs_clear_opt(ctx->mount_opt, NODATACOW); >>> + btrfs_clear_opt(ctx->mount_opt, NODATASUM); >>> } else if (strncmp(param->string, "zstd", 4) == 0) { >>> ctx->compress_type = BTRFS_COMPRESS_ZSTD; >>> ctx->compress_level = >> >> Another thing is, if we want to prefer using zstd-fast:9 other than >> zstd:-9, should we also change our compress handling in >> btrfs_show_options() to show zstd-fast:9 instead? > > At this point it's not about preference but about compatibility. I > actually tested changing this but as a side-effect it also had an > influence on `btrfs.compression` xattr (our `compression` property) > which is rather an incompatible on-disk format change. Hence I falled > back keeping it simple. Showing `zstd:-9` is still a valid output. > > --nX > >> Thanks, >> Qu >
On Tue, Apr 01, 2025 at 07:44:01AM +1030, Qu Wenruo wrote: > > > 在 2025/3/31 20:36, Daniel Vacek 写道: > [...] > >>> btrfs_set_opt(ctx->mount_opt, COMPRESS); > >>> btrfs_clear_opt(ctx->mount_opt, NODATACOW); > >>> btrfs_clear_opt(ctx->mount_opt, NODATASUM); > >>> + } else if (strncmp(param->string, "zstd-fast", 9) == 0) { > >>> + ctx->compress_type = BTRFS_COMPRESS_ZSTD; > >>> + ctx->compress_level = > >>> + -btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD, > >>> + param->string + 9 > >> > >> Can we just use some temporary variable to save the return value of > >> btrfs_compress_str2level()? > > > > I don't see any added value. Isn't `ctx->compress_level` temporary > > enough at this point? Note that the FS is not mounted yet so there's > > no other consumer of `ctx`. > > > > Or did you mean just for readability? > > Doing all the same checks and flipping the sign of ctx->compress_level > is already cursed to me. > > Isn't something like this easier to understand? > > level = btrfs_compress_str2level(); > if (level > 0) > ctx->compress_level = -level; > else > ctx->compress_level = level; > > > > >> ); > >>> + if (ctx->compress_level > 0) > >>> + ctx->compress_level = -ctx->compress_level; > >> > >> This also means, if we pass something like "compress=zstd-fast:-9", it > >> will still set the level to the correct -9. > >> > >> Not some weird double negative, which is good. > >> > >> But I'm also wondering, should we even allow minus value for "zstd-fast". > > > > It was meant as a safety in a sense that `s/zstd:-/zstd-fast:-/` still > > works the same. Hence it defines that "fast level -3 <===> fast level > > 3" (which is still level -3 in internal zstd representation). > > So if you mounted `compress=zstd-fast:-9` it's understood you actually > > meant `compress=zstd-fast:9` in the same way as if you did > > `compress=zstd:-9`. > > > > I thought this was solid. Or would you rather bail out with -EINVAL? > > I prefer to bail out with -EINVAL, but it's only my personal choice. I tend to agree with you, the idea for the alias was based on feedback that upstream zstd calls the levels fast, not by the negative numbers. So I think we stick to the zstd: and zstd-fast: prefixes followed only by the positive numbers. We can make this change before 6.15 final so it's not in any released kernel and we don't have to deal with compatibility.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 40709e2a44fce..c1bc8d4db440a 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -368,6 +368,16 @@ static int btrfs_parse_param(struct fs_context *fc, struct fs_parameter *param) btrfs_set_opt(ctx->mount_opt, COMPRESS); btrfs_clear_opt(ctx->mount_opt, NODATACOW); btrfs_clear_opt(ctx->mount_opt, NODATASUM); + } else if (strncmp(param->string, "zstd-fast", 9) == 0) { + ctx->compress_type = BTRFS_COMPRESS_ZSTD; + ctx->compress_level = + -btrfs_compress_str2level(BTRFS_COMPRESS_ZSTD, + param->string + 9); + if (ctx->compress_level > 0) + ctx->compress_level = -ctx->compress_level; + btrfs_set_opt(ctx->mount_opt, COMPRESS); + btrfs_clear_opt(ctx->mount_opt, NODATACOW); + btrfs_clear_opt(ctx->mount_opt, NODATASUM); } else if (strncmp(param->string, "zstd", 4) == 0) { ctx->compress_type = BTRFS_COMPRESS_ZSTD; ctx->compress_level =
Now that zstd fast compression levels are allowed with `compress=zstd:-N` mount option, allow also specifying the same using the `compress=zstd-fast:N` alias. Upstream zstd deprecated the negative levels in favor of the `zstd-fast` label anyways so this is actually the preferred way now. And indeed it also looks more human friendly. Signed-off-by: Daniel Vacek <neelx@suse.com> --- fs/btrfs/super.c | 10 ++++++++++ 1 file changed, 10 insertions(+)