| 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.
On Tue, 1 Apr 2025 at 00:57, David Sterba <dsterba@suse.cz> wrote: > > 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. Hmm, so for zlib and zstd if the level is out of range, it's just clipped and not failed as invalid. I guess zstd-fast should also do the same to be consistent. > 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.
在 2025/4/2 19:07, Daniel Vacek 写道: > On Tue, 1 Apr 2025 at 00:57, David Sterba <dsterba@suse.cz> wrote: [...] >>>> 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. > > Hmm, so for zlib and zstd if the level is out of range, it's just > clipped and not failed as invalid. I guess zstd-fast should also do > the same to be consistent. Or we can change the zlib/zstd level checks so that it can return -EINVAL when invalid levels are provided. But to avoid huge surprise, I'd recommend to add warning/error messages first. I'm not a huge fan when invalid values are silently clamped, even it's just an optional level parameter. Thanks, Qu > >> 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. >
On Wed, 2 Apr 2025 at 11:08, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > 在 2025/4/2 19:07, Daniel Vacek 写道: > > On Tue, 1 Apr 2025 at 00:57, David Sterba <dsterba@suse.cz> wrote: > [...] > >>>> 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. > > > > Hmm, so for zlib and zstd if the level is out of range, it's just > > clipped and not failed as invalid. I guess zstd-fast should also do > > the same to be consistent. > > Or we can change the zlib/zstd level checks so that it can return > -EINVAL when invalid levels are provided. > > But to avoid huge surprise, I'd recommend to add warning/error messages > first. > > I'm not a huge fan when invalid values are silently clamped, even it's > just an optional level parameter. I agree. Well, one by one. Let's nail the `zstd-fast` first and clean up in subsequent patches. I already plan to fix another issue I noticed. For example `compress=zlibfoo` is still accepted as zlib, etc. > Thanks, > Qu > > > > >> 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. > > >
On Tue, 1 Apr 2025 at 00:57, David Sterba <dsterba@suse.cz> wrote: > > 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. I'd still opt for keeping full range and functionality including negative levels using the plain `zstd:N` option and having the other just as an additional alias (for maybe being a bit nicer to some humans, but not a big deal really and a matter of preference). Checking the official documentation, it still mentions "negative compression levels" being the fast option. https://facebook.github.io/zstd/ https://facebook.github.io/zstd/zstd_manual.html The deprecation part looks like just some gossip. It looks more about the cli tool api and we are defining a kernel mount api - perfectly unrelated. > 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(+)