fs/btrfs/super.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
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);
+ 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 =
--
2.47.2
在 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.
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.
On Wed, 2 Apr 2025 at 16:31, Daniel Vacek <neelx@suse.com> wrote:
>
> 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.
Any feedback, Dave? I tend to drop this ida of `zstd-fast` alias.
> > 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/14 16:23, Daniel Vacek 写道: > On Wed, 2 Apr 2025 at 16:31, Daniel Vacek <neelx@suse.com> wrote: [...] >> 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. > > Any feedback, Dave? I tend to drop this ida of `zstd-fast` alias. Not Dave here, but if the future of "zstd-fast" is not that clear, we can definitely wait for a while. It's always safer to adapt when the terminology is mature enough. 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, Apr 15, 2025 at 01:20:15PM +0930, Qu Wenruo wrote: > 在 2025/4/14 16:23, Daniel Vacek 写道: > > On Wed, 2 Apr 2025 at 16:31, Daniel Vacek <neelx@suse.com> wrote: > [...] > >> 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. > > > > Any feedback, Dave? I tend to drop this ida of `zstd-fast` alias. > > Not Dave here, but if the future of "zstd-fast" is not that clear, we > can definitely wait for a while. > > It's always safer to adapt when the terminology is mature enough. Thanks all for the feedback, OK let's put it on hold.
> On Apr 14, 2025, at 11:50 PM, Qu Wenruo <wqu@suse.com> wrote: > > > > > > 在 2025/4/14 16:23, Daniel Vacek 写道: >> On Wed, 2 Apr 2025 at 16:31, Daniel Vacek <neelx@suse.com> wrote: > [...] >>> 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://urldefense.com/v3/__https://facebook.github.io/zstd/__;!!Bt8RZUm9aw!7KPURbKO2g65XCAyShKtwZo6K7VjTovi2iOlXsfo1zUBg-bqxGY6TFndfisxqKk_kQzI$ https://urldefense.com/v3/__https://facebook.github.io/zstd/zstd_manual.html__;!!Bt8RZUm9aw!7KPURbKO2g65XCAyShKtwZo6K7VjTovi2iOlXsfo1zUBg-bqxGY6TFndfisxqHPiTlxm$ >>> 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. >> Any feedback, Dave? I tend to drop this ida of `zstd-fast` alias. > > Not Dave here, but if the future of "zstd-fast" is not that clear, we can definitely wait for a while. > > It's always safer to adapt when the terminology is mature enough. Upstream refers to the negative compression levels as fast levels. Both because it describes their aim (to be fast), and because passing a negative compression level is hard in the CLI where `zstd -1` means level 1. So the CLI says `zstd --fast=1` means level negative 1. However, on the library side there is no "zstd-fast" concept. You just pass a negative compression level to zstd. Other libraries, like folly::compression, also refer to negative compression levels as ZSTD_FAST [0]. However, this is only because there were pre-existing "enum" values that assigned different semantics to levels -1 through -3, so we couldn't just pass a negative compression level. Overall, the concept of "fast" compression levels meaning negative levels isn't going anywhere. However, neither is passing negative compression levels to the upstream API. Both are valid. I'd lean towards just referring to using `zstd:-N` because it keeps the user interface smaller. Best, Nick Terrell [0] https://github.com/facebook/folly/blob/5237d93b450bfd9170c4746f00aa583f0e662c2d/folly/compression/Compression.h#L113-L125 [1] https://github.com/facebook/folly/blob/5237d93b450bfd9170c4746f00aa583f0e662c2d/folly/compression/Compression.h#L446-L448 > 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.
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. > > >
© 2016 - 2025 Red Hat, Inc.