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

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.

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.

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

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