Re: [PATCH] btrfs: zstd: add `zstd-fast` alias mount option for fast modes

[David Sterba]

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.