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

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

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.

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