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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPjX3FfOJMFC8cXCqLa2yS1qSYmhu5cjV__+7xVRFGuKu=RqiA@mail.gmail.com>
Date: Mon, 14 Apr 2025 08:53:21 +0200
From: Daniel Vacek <neelx@...e.com>
To: dsterba@...e.cz
Cc: Qu Wenruo <quwenruo.btrfs@....com>, Qu Wenruo <wqu@...e.com>, Chris Mason <clm@...com>, 
	Josef Bacik <josef@...icpanda.com>, David Sterba <dsterba@...e.com>, Nick Terrell <terrelln@...com>, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: zstd: add `zstd-fast` alias mount option for fast modes

On Wed, 2 Apr 2025 at 16:31, Daniel Vacek <neelx@...e.com> wrote:
>
> On Tue, 1 Apr 2025 at 00:57, David Sterba <dsterba@...e.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.