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

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

[Calvin Owens]

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

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

[Qu Wenruo]

在 2025/8/22 17:15, Calvin Owens 写道:
> The compression level is meaningless for lzo, but before commit
> 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
> it was silently ignored if passed.

Since LZO doesn't support compression level, why providing a level 
parameter in the first place?

I think it's time for those users to properly update their mount options.

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

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

[Calvin Owens]

On Friday 08/22 at 17:57 +0930, Qu Wenruo wrote:
> 在 2025/8/22 17:15, Calvin Owens 写道:
> > The compression level is meaningless for lzo, but before commit
> > 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
> > it was silently ignored if passed.
> 
> Since LZO doesn't support compression level, why providing a level parameter
> in the first place?

Interpreting "no level" as "level is always one" doesn't seem that
unreasonable to me, especially since it has worked forever.

> I think it's time for those users to properly update their mount options.

It's a user visable regression, and fixing it has zero possible
downside. I think you should take my patch :)

Thanks,
Calvin

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

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

[Qu Wenruo]

在 2025/8/22 18:58, Calvin Owens 写道:
> On Friday 08/22 at 17:57 +0930, Qu Wenruo wrote:
>> 在 2025/8/22 17:15, Calvin Owens 写道:
>>> The compression level is meaningless for lzo, but before commit
>>> 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
>>> it was silently ignored if passed.
>>
>> Since LZO doesn't support compression level, why providing a level parameter
>> in the first place?
> 
> Interpreting "no level" as "level is always one" doesn't seem that
> unreasonable to me, especially since it has worked forever.

No means no, period.

> 
>> I think it's time for those users to properly update their mount options.
> 
> It's a user visable regression, and fixing it has zero possible
> downside. I think you should take my patch :)

I do not want to encourage such usage.

Sanity overrides "regression". If it shouldn't work in the first place, 
it's not a regression.

> 
> Thanks,
> Calvin
> 
>>>
>>> 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);
>>
> 

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

[David Sterba]

On Fri, Aug 22, 2025 at 02:28:29AM -0700, Calvin Owens wrote:
> On Friday 08/22 at 17:57 +0930, Qu Wenruo wrote:
> > 在 2025/8/22 17:15, Calvin Owens 写道:
> > > The compression level is meaningless for lzo, but before commit
> > > 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
> > > it was silently ignored if passed.
> > 
> > Since LZO doesn't support compression level, why providing a level parameter
> > in the first place?
> 
> Interpreting "no level" as "level is always one" doesn't seem that
> unreasonable to me, especially since it has worked forever.

As it currently works, no level means use the default, which is defined
for all compression. For LZO it's implicit and 1.

> > I think it's time for those users to properly update their mount options.
> 
> It's a user visable regression, and fixing it has zero possible
> downside. I think you should take my patch :)

I tend to agree this is a usability regression, even if LZO is a bit odd
with levels, accepting the allowed values should work.

The mount options and level combinations that should work:

- compress=NAME   - use default level for NAME
- compress=NAME:0 - use default, while accepting the level setting
- compress=NAME:N - if N is in the allowed range for NAME then take it

The syntax is consistent for all three compressions.

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

[Calvin Owens]

On Friday 08/22 at 20:45 +0200, David Sterba wrote:
> On Fri, Aug 22, 2025 at 02:28:29AM -0700, Calvin Owens wrote:
> > On Friday 08/22 at 17:57 +0930, Qu Wenruo wrote:
> > > 在 2025/8/22 17:15, Calvin Owens 写道:
> > > > The compression level is meaningless for lzo, but before commit
> > > > 3f093ccb95f30 ("btrfs: harden parsing of compression mount options"),
> > > > it was silently ignored if passed.
> > > 
> > > Since LZO doesn't support compression level, why providing a level parameter
> > > in the first place?
> > 
> > Interpreting "no level" as "level is always one" doesn't seem that
> > unreasonable to me, especially since it has worked forever.
> 
> As it currently works, no level means use the default, which is defined
> for all compression. For LZO it's implicit and 1.
> 
> > > I think it's time for those users to properly update their mount options.
> > 
> > It's a user visable regression, and fixing it has zero possible
> > downside. I think you should take my patch :)
> 
> I tend to agree this is a usability regression, even if LZO is a bit odd
> with levels, accepting the allowed values should work.
> 
> The mount options and level combinations that should work:
> 
> - compress=NAME   - use default level for NAME
> - compress=NAME:0 - use default, while accepting the level setting
> - compress=NAME:N - if N is in the allowed range for NAME then take it
> 
> The syntax is consistent for all three compressions.

Thanks David.

Maybe the below is a little more palatable? Letting the single level be
a detail so the branches in btrfs_parse_compress() all match?

But, the compression level ends up being printk'd as '1', where it has
always been '0' in the past (and still is in 6.17-rc):

    - BTRFS info (device vda state M): use lzo compression, level 0
    + BTRFS info (device vda state M): use lzo compression, level 1

With my v1 it's still always printed as zero, if that's preferable.

-----8<-----
From: Calvin Owens <calvin@wbinvd.org>
Subject: [PATCH v2] 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

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index a262b494a89f..bbcaac7022b0 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -299,9 +299,10 @@ 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 + 4);
 		btrfs_set_opt(ctx->mount_opt, COMPRESS);
 		btrfs_clear_opt(ctx->mount_opt, NODATACOW);
 		btrfs_clear_opt(ctx->mount_opt, NODATASUM);
-- 
2.47.2

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

[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,
2. existing setups relying on this behavior continue to work,
3. the @may_have_level semantics remain consistent.

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

[Qu Wenruo]

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

I'm fine with warning, but the mount should still fail.
Or those people will never learn to read the doc.

> 3. the @may_have_level semantics remain consistent.
> 
> 

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

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

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

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

[David Sterba]

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.

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

[Qu Wenruo]

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

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

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

[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".

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

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

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

[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@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).

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

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

[Qu Wenruo]

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

Reviewed-by: Qu Wenruo <wqu@suse.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);

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

[Daniel Vacek]

On Sun, 24 Aug 2025 at 17:58, Calvin Owens <calvin@wbinvd.org> wrote:
> From: Calvin Owens <calvin@wbinvd.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@wbinvd.org>
> ---
>  fs/btrfs/super.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

v3 looks good to me. The original hardening was meant to gate complete
nonsense like "compress=lzoutput", etc...

Reviewed-by: Daniel Vacek <neelx@suse.com>

Thank you.

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

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

[Qu Wenruo]

在 2025/8/25 18:21, Daniel Vacek 写道:
> On Sun, 24 Aug 2025 at 17:58, Calvin Owens <calvin@wbinvd.org> wrote:
>> From: Calvin Owens <calvin@wbinvd.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@wbinvd.org>
>> ---
>>   fs/btrfs/super.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> v3 looks good to me. The original hardening was meant to gate complete
> nonsense like "compress=lzoutput", etc...
> 
> Reviewed-by: Daniel Vacek <neelx@suse.com>

Now merged and pushed to for-next branch with the latest reviewed-by tags.

Thanks,
Qu
> 
> Thank you.
> 
>> 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
>>

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

[Filipe Manana]

On Mon, Aug 25, 2025 at 10:03 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2025/8/25 18:21, Daniel Vacek 写道:
> > On Sun, 24 Aug 2025 at 17:58, Calvin Owens <calvin@wbinvd.org> wrote:
> >> From: Calvin Owens <calvin@wbinvd.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@wbinvd.org>
> >> ---
> >>   fs/btrfs/super.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > v3 looks good to me. The original hardening was meant to gate complete
> > nonsense like "compress=lzoutput", etc...
> >
> > Reviewed-by: Daniel Vacek <neelx@suse.com>
>
> Now merged and pushed to for-next branch with the latest reviewed-by tags.

Btw, don't forget a couple things:

1) In the subject, after the prefix "btrfs: " the first word should
not be capitalized;

2) In the log message (btrfs_warn() call), the first word should also
not be capitalized.

These are the styles we follow, so we should be consistent.

>
> Thanks,
> Qu
> >
> > Thank you.
> >
> >> 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
> >>
>
>

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

[Calvin Owens]

On Friday 08/22 at 18:20 +0800, Sun YangKai wrote:
> > 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,
> 2. existing setups relying on this behavior continue to work,
> 3. the @may_have_level semantics remain consistent.

Thanks Sun, sorry for not acknowledging your suggestion in my last
response. Repeating what I said there: if it helps get this in, I'm
happy to do it, but it sounds like Qu is pretty fundamentally opposed
to keeping the old behavior.

>