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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250331225705.GN32661@twin.jikos.cz>
Date: Tue, 1 Apr 2025 00:57:05 +0200
From: David Sterba <dsterba@...e.cz>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: Daniel Vacek <neelx@...e.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 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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ