Re: [PATCH 0/4] btrfs: add per-inode compression levels in xattrs

在 2026/8/10 08:47, Zygo Blaxell 写道:
> On Sun, Aug 09, 2026 at 10:30:45AM +0930, Qu Wenruo wrote:
>>
>>
>> 在 2026/8/9 10:05, koraynilay 写道:
>>> On Sun Aug 9, 2026 at 2:17 AM CEST, Qu Wenruo wrote:
>>>> I'm not sure if this is the correct behavior in the first place.
>>>>
>>>> As you already mentioned, zstd and zlib have very different compression
>>>> level range, using the incorrect level makes no sense (and it's being
>>>> clamped anyway).
>>>>
>>>> I think we should go the default level when not specified, which makes
>>>> more sense, and that would definitely be something worth fixing.
>>>
>>> Yes, I also think that would be best, but my main concern would be it
>>> changing how chattr +c behaves (I'm less concerned about the btrfs prop
>>> set file compression "zstd" case, since IMO that implies the user wants
>>> the default level).
>>
>> Mind to explain more about the "chattr +c" problem?
>>
>> IIRC "chattr +c" just set the btrfs.compression XATTR to the default zlib if
>> no mount option is specified.
> 
> chattr with _any_ argument resets the btrfs.compression xattr _every_
> time, even if the caller changes a flag something that doesn't look like
> it would affect compression, e.g. chattr +A or +d.

This is the known problem of chattr.

> 
> That can lead to surprising results, like changing compress type from
> zstd to zlib, if the original btrfs.compression attribute doesn't match
> the mount option.

This doesn't only apply to c, but all other chattr options.

That's why proper chattr should always include the full attrs, not only 
the one to change.

> 
>> In that case it should be no difference compared to any existing XATTR based
>> compression setting.
>>
>> Thus it's just the same missing level handling, and IMHO since XATTR
>> compression level is never specified in XATTR, then the behavior is never
>> fully determined, and users should not depend on it.
>>
>> Even if we changed the behavior to option 3, it should not be a super huge
>> user affecting change.
>> In the end, it's just compression level, affecting compression ratio and
>> speed, not really a huge behavior change.
>>
>>>
>>> The options I considered were:
>>> 1) keep the "bug", like I did for now;
>>> 2) keep the "bug", but only if the compress= algo is the same
>>>      as the btrfs.compression one, if they aren't, use the default for the
>>>      btrfs.compression algo (e.g. compress=zstd:15 and btrfs.compression=zlib
>>>      would compress the extent at zlib:3 instead of clamp(zlib, 15) = 9)
>>>      (suggested by Zygo);
>>> 3) fix the "bug" entirely, which is what I actually accidentally did at
>>>      first, by just setting compress_level = inode->prop_compress_level
>>>      without any check prior to that (which means that by default it would
>>>      use algo:0).
>>
>> IHMO both option 2 and 3 are acceptable.
>>
>>
>> The only extra concern is, if we have a new level field in XATTR, can older
>> kernels handle it?
>>
>> And thankfully the existing prop apply handler is checking only the first
>> several bytes for different algos, thus the existing code should handle the
>> extra appended ":<level>" correctly by just ignoring the level.
>>
>> So either option 2 or 3 would be fine to me. Although I personally prefer
>> option 3 a little more, just because it's much cleaner code wise.
> 
> Option 2 preserves legacy behavior that is 12 years old now, and it
> costs a single comparison in two 'if' statements.
> 
> Option 3 makes an already confusing situation worse--it makes the
> underspecified behavior change depending on kernel version.

One should never rely on something not documented in the first place.

We're to make it more clear, not making some undocumented BS to be the 
common case.

> 
>> Thanks,
>> Qu
>>
>>>
>>> Option 2) is probably the best compromise between breaking existing
>>> scripts and the behaviour making sense, plus it shouldn't change the
>>> chattr +c behaviour, since btrfs takes the algorithm from compress=.
>>>
>>> Thanks.
>>>
>>> Best,
>>> koraynilay
>>
>>