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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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