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

[Qu Wenruo <wqu@suse.com>]

在 2026/8/11 12:25, koraynilay 写道:
>> I'd prefer to have a dedicated patch to set compress_level to the
>> default value 0, as a proper bug fix as the first patch of the series.
>>
>> As you mentioned in the cover-letter, this is in fact fixing a bug in
>> the old behavior (mismatched algo and level).
>>
>> So it's definitely worth a dedicated fix, so that we can backport the
>> fix without pulling in the full series for older kernels.
> 
> Ok so, after thinking about it more and bouncing ideas around (with Zygo
> too) I realized one thing, that while it *is* technically a bug, I don't
> think it's a bug worth backporting.
> 
> My reasoning is simply that there is no use-case where the current
> "buggy" behaviour would be damaging, as the levels get clamped to the
> supported range anyway, while arguably there are (albeit very rare and
> probably not very smart in the first place) use-cases where fixing it
> could be somewhat (limitedly) damaging.

To be honest, if the current behavior is not damaging, which I agree, 
then it's also not damaging to use the default level.

After all, it's just a level change, which is never damaging.

> 
> More importantly IMO, doing this would allow us to explicitly explain
> the currently undocumented behaviour in the btrfs-property(8) manpage as
> "just so you know, for kernel versions < 7.x cross-algo level leakage
> from -o compress was happening".

Which also applies to option 3.

> 
> 
> 
> As for how to handle it after having support for levels in the XATTR,
> option 2, aka leaking the compress level only if the algo matches, would
> be the best imo:
> 
> Example use-case:
> 
> - /fs has various types of files, from media to git repos, that would
>    benefit from the normal compress mount option;
> - /fs also has big virtual machine disks, that have very compressible
>    parts but also very uncompressible parts;
> 
> using only `mount -o compress=zstd:7 /fs` may mark the vm disks with
> NOCOMPRESS as soon as an incompressible extent gets found, but setting
> btrfs.compression=zstd won't, as it will try to compress every extent
> anyway[1].

BTW, the default level is 3, so 7 is already trying to compress harder 
than default.
(At least from the official man page)

> This way if the user intends to change the compress level for the whole
> fs, they can just change the mount option, knowing that the new level
> will apply to the (new) vm extents too, like it will for all other files.
> 
> In this example "btrfs.compression=zstd" and "btrfs.compression=zstd:0"
> would behave the same, which means the file's extent will get compressed
> with zstd:7, but when the user remounts with e.g. zstd:15, they will use
> this new level (only for the extents written from that point afterwards,
> of course).

I'd say, in this particular case, user should specify a different level 
for VM images, after the level support in XATTR, other than relying on 
the global mount option level.

> 
> (I'm ignoring the case where the user wants to change the algorithm and
> let the vms inherit it, as for this specific use-case that would likely
> require a whole new feature/property to say "try to compress anyway but
> not as much as compress-force" and probably most people use zstd anyway
> nowadays).
> 
> 
> 
> After coming to this conclusion, I'm personally pretty satisfied with
> this solution, while I wasn't as much with the other ones.

Since my idea is pretty different on option 2 vs 3, and I do not find we 
can persuade each other, so I'll leave David to do the final call.

> 
> Thanks again.
> 
> Best,
> koraynilay
> 
> [1]: https://github.com/kdave/btrfs-progs/pull/1152/commits/7ae9e2aa7a35af5e7b656424957a558a9d0dd676