Message-ID: <aKs2mCRjtv3Ki06Z@mozart.vkv.me>
Date: Sun, 24 Aug 2025 08:58:16 -0700
From: Calvin Owens <calvin@...nvd.org>
To: Qu Wenruo <quwenruo.btrfs@....com>
Cc: Sun YangKai <sunk67188@...il.com>, clm@...com, dsterba@...e.com,
	josef@...icpanda.com, linux-btrfs@...r.kernel.org,
	linux-kernel@...r.kernel.org, neelx@...e.com
Subject: Re: [PATCH] btrfs: Accept and ignore compression level for lzo

On Saturday 08/23 at 09:09 +0930, Qu Wenruo wrote:
> 在 2025/8/23 08:54, Calvin Owens 写道:
> > On Saturday 08/23 at 07:14 +0930, Qu Wenruo wrote:
> > > 在 2025/8/23 01:24, Calvin Owens 写道:
> > > > On Friday 08/22 at 19:53 +0930, Qu Wenruo wrote:
> > > > > 在 2025/8/22 19:50, Sun YangKai 写道:
> > > > > > > The compression level is meaningless for lzo, but before commit
> > > > > > > 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
> > > > > > > it was silently ignored if passed.
> > > > > > > 
> > > > > > > After that commit, passing a level with lzo fails to mount:
> > > > > > >        BTRFS error: unrecognized compression value lzo:1
> > > > > > > 
> > > > > > > Restore the old behavior, in case any users were relying on it.
> > > > > > > 
> > > > > > > Fixes: 3f093ccb95f30 ("btrfs: harden parsing of compression mount options")
> > > > > > > Signed-off-by: Calvin Owens <calvin@...nvd.org>
> > > > > > > ---
> > > > > > > 
> > > > > > >     fs/btrfs/super.c | 2 +-
> > > > > > >     1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> > > > > > > index a262b494a89f..7ee35038c7fb 100644
> > > > > > > --- a/fs/btrfs/super.c
> > > > > > > +++ b/fs/btrfs/super.c
> > > > > > > @@ -299,7 +299,7 @@ static int btrfs_parse_compress(struct btrfs_fs_context
> > > > > > > *ctx,>
> > > > > > >     		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> > > > > > >     		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
> > > > > > >     		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
> > > > > > > 
> > > > > > > -	} else if (btrfs_match_compress_type(string, "lzo", false)) {
> > > > > > > +	} else if (btrfs_match_compress_type(string, "lzo", true)) {
> > > > > > > 
> > > > > > >     		ctx->compress_type = BTRFS_COMPRESS_LZO;
> > > > > > >     		ctx->compress_level = 0;
> > > > > > >     		btrfs_set_opt(ctx->mount_opt, COMPRESS);
> > > > > > > 
> > > > > > > --
> > > > > > > 2.47.2
> > > > > > 
> > > > > > A possible improvement would be to emit a warning in
> > > > > > btrfs_match_compress_type() when @may_have_level is false but a
> > > > > > level is still provided. And the warning message can be something like
> > > > > > "Providing a compression level for {compression_type} is not supported, the
> > > > > > level is ignored."
> > > > > > 
> > > > > > This way:
> > > > > > 1. users receive a clearer hint about what happened,
> > > > > 
> > > > > I'm fine with the extra warning, but I do not believe those kind of users
> > > > > who provides incorrect mount option will really read the dmesg.
> > > > > 
> > > > > > 2. existing setups relying on this behavior continue to work,
> > > > > 
> > > > > Or let them fix the damn incorrect mount option.
> > > > 
> > > > You're acting like I'm asking for "compress=lzo:iamafancyboy" to keep
> > > > working here. I think what I proposed is a lot more reasonable than
> > > > that, I'm *really* surprised you feel so strongly about this.
> > > 
> > > Because there are too many things in btrfs that are being abused when it was
> > > never supposed to work.
> > > 
> > > You are not aware about how damaging those damn legacies are.
> > > 
> > > Thus I strongly opposite anything that is only to keep things working when
> > > it is not supposed to be in the first place.
> > > 
> > > I'm already so tired of fixing things we should have not implemented a
> > > decade ago, and those things are still popping here and there.
> > > 
> > > If you feel offended, then I'm sorry but I just don't want bad examples
> > > anymore, even it means regression.
> > 
> > I'm not offended Qu. I empathize with your point of view, I apologize if
> > I came across as dismissive earlier.
> > 
> > I think trivial regression fixes like this can actually save you pain in
> > the long term, when they're caught as quickly as this one was. I think
> > this will prevent a steady trickle of user complaints over the next five
> > years from happening.
> > 
> > I can't speak for anybody else, but I'm *always* willing to do extra
> > work to deal with breaking changes if the end result is that things are
> > better or simpler. This just seems to me like a case where nothing
> > tangible is gained by breaking compatibility, and nothing is lost by
> > keeping it.
> > 
> > I'm absolutely not arguing that the mount options should be backwards
> > compatible with any possible abuse, this is a specific exception. Would
> > clarifying that in the commit message help? I understand if you're
> > concerned about the "precedent".
> 
> Then I'm fine with a such patch, but still prefer a warning (not WARN(),
> just much simpler btrfs_warn()) line to be shown when a level is provided
> for lzo.
> 
> Furthermore, since we already have something like btrfs_lzo_compress
> indicating the supported level, setting to the proper default value would be
> better. (Already done by btrfs_compress_set_level() call in your v2 patch).

Thanks Qu. v3 below.

There was an off-by-one in my v2, len("lzo") is three, doh.

> BTW, since you mentioned something like "compress=lzo:asdf",
> btrfs_compress_set_level() just ignores any kstrtoint() error, allowing
> things like "compress=zstd:invalid" to pass the option parsing.
> 
> I can definitely send out something to enhance that check, but just want to
> be sure, would you opposite such extra sanity checks?

I have no objection to that at all, IMHO that's a good thing to do.

Is it worth adding a testcase somewhere for the compression options? I'm
happy to do that too, but I'm not sure what the right place for it is.

Thanks,
Calvin

-----8<-----
From: Calvin Owens <calvin@...nvd.org>
Subject: [PATCH v3] btrfs: Accept and ignore compression level for lzo

The compression level is meaningless for lzo, but before commit
3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
it was silently ignored if passed.

After that commit, passing a level with lzo fails to mount:

    BTRFS error: unrecognized compression value lzo:1

It seems reasonable for users to expect that lzo would permit a numeric
level option, as all the other algos do, even though the kernel's
implementation of LZO currently only supports a single level. Because it
has always worked to pass a level, it seems likely to me that users in
the real world are relying on doing so.

This patch restores the old behavior, giving "lzo:N" the same semantics
as all of the other compression algos.

To be clear, silly variants like "lzo:one", "lzo:the_first_option", or
"lzo:armageddon" also used to work. This isn't meant to suggest that
any possible mis-interpretation of mount options that once worked must
continue to work forever. This is an exceptional case where it makes
sense to preserve compatibility, both because the mis-interpretation is
reasonable, and because nothing tangible is sacrificed.

Fixes: 3f093ccb95f30 ("btrfs: harden parsing of compression mount options")
Signed-off-by: Calvin Owens <calvin@...nvd.org>
---
 fs/btrfs/super.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index a262b494a89f..18eb00b3639b 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -299,9 +299,12 @@ static int btrfs_parse_compress(struct btrfs_fs_context *ctx,
 		btrfs_set_opt(ctx->mount_opt, COMPRESS);
 		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-	} else if (btrfs_match_compress_type(string, "lzo", false)) {
+	} else if (btrfs_match_compress_type(string, "lzo", true)) {
 		ctx->compress_type = BTRFS_COMPRESS_LZO;
-		ctx->compress_level = 0;
+		ctx->compress_level = btrfs_compress_str2level(BTRFS_COMPRESS_LZO,
+							       string + 3);
+		if (string[3] == ':' && string[4])
+			btrfs_warn(NULL, "Compression level ignored for LZO");
 		btrfs_set_opt(ctx->mount_opt, COMPRESS);
 		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-- 
2.49.1

Powered by blists - more mailing lists