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

[David Sterba <dsterba@suse.cz>]

On Fri, Aug 22, 2025 at 08:54:13AM -0700, Calvin Owens wrote:
> 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@wbinvd.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.
> 
> 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,

Which is the reason to restore accepting the level, it's observable
behaviour and it also has impact on functionality when the mount fails.