Message-ID: <9ada7f0f-2b46-4e7d-812d-666af3b5c023@suse.com>
Date: Mon, 25 Aug 2025 07:10:58 +0930
From: Qu Wenruo <wqu@...e.com>
To: Calvin Owens <calvin@...nvd.org>, 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



在 2025/8/25 01:28, Calvin Owens 写道:
> 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.

It can be added to fstests.

> 
> 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>

Reviewed-by: Qu Wenruo <wqu@...e.com>

Thanks,
Qu

> ---
>   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);

Powered by blists - more mailing lists