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

[Zygo Blaxell <ce3g8jdj@umail.furryterror.org>]

On Mon, Aug 10, 2026 at 10:21:11AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/8/10 09:58, Zygo Blaxell 写道:
> > On Mon, Aug 10, 2026 at 08:50:45AM +0930, Qu Wenruo wrote:
> > > 
> > > 
> > > 在 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.
> > 
> > Option 3 prevents existing mount-option compression level specifications
> > from working when the attribute agress with the mount option; otherwise,
> > they would be blocked by a btrfs.compression string that doesn't specify
> > a level.  That's a _regression_.
> 
> Let me be this clear, the current one nor option 2 is not working either.
> 
> If the current algo is different from the XATTR algo, it will be whatever
> random number clamped to the XATTR algo for the current code.
> 
> This applies to the option 2 solution. When mount option changed, the level
> will suddenly change from whatever previous mount option to the default.
> 
> It's always broken no matter whatever, I do not want to waste time to use
> another broken behavior to mask a more broken one.

It's not broken during the time when the mount option is the same as
the xattr, and that case seems to be the most common one.  Users don't
change the compression type on the fly or per inode--they use zstd all
the time, and only change the level when it matters.

This stops working with the option 3 proposal (or more specifically, with
my comment on the other patch about when to set compress_level to 0):
it always sets the level to the default, and ignores the mount option,
even when that would make sense and be the intended result.

> > The current behavior is documented all over the place, used in scripts,
> > etc.  The top two from Google:
> > 
> >      https://wiki.tnonline.net/w/Btrfs/Compression
> >      https://fedoraproject.org/wiki/Changes/BtrfsTransparentCompression#Q:_If_I_use_'btrfs_property'_to_set_compression,_what_level_is_used_for_zstd?
> 
> None of them is the official btrfs-progs man page.
> 
> The first URL doesn't even resolve here.
> 
> I don't want to repeat myself, all those confusion is from the bug itself.
> 
> Instead of building everything upon an uncertain basis, do it correctly.
> 
> And I have repeated it several times, even the behavior change is minimal
> only affecting the level.
> 
> I see no point why you treat broken behavior so dogmatically.