[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b1437d32-8c85-4e5d-9c68-76938dcf6573@gmx.com>
Date: Tue, 1 Apr 2025 07:44:01 +1030
From: Qu Wenruo <quwenruo.btrfs@....com>
To: Daniel Vacek <neelx@...e.com>, Qu Wenruo <wqu@...e.com>
Cc: 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
在 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
>
Powered by blists - more mailing lists