Re: [PATCH] btrfs: Accept and ignore compression level for lzo

[Qu Wenruo]

在 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@xxxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > >     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).


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?

Thanks,
Qu

> > > In my case it was actually little ARM boards with an /etc/fstab
> > > generated by templating code that didn't understand lzo is special.
> > >
> > > I'm not debating that it's incorrect (I've already fixed it). But given
> > > that passing the level has worked forever, I'm sure this thing sitting
> > > on my desk right now is not the only thing in the world that assumed it
> > > would keep working...
> > >
> > > > I'm fine with warning, but the mount should still fail.
> > > > Or those people will never learn to read the doc.
> > >
> > > The warning is pointless IMHO, it's already obvious why it failed. My
> > > only goal was to avoid breaking existing systems in the real world when
> > > they upgrade the kernel.
> > >
> > > If you'd take a patch that makes it work with a WARN(), I'll happily
> > > send you that. But I'm not going to add the WARN() and keep it failing:
> > > if that's all you'll accept, let's just drop it.
> > >
> > > Thanks,
> > > Calvin
> > >
> > > > > 3. the @may_have_level semantics remain consistent.