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

["koraynilay" <koray.fra@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]

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

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



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

Thanks again.

Best,
koraynilay

[1]: https://github.com/kdave/btrfs-progs/pull/1152/commits/7ae9e2aa7a35af5e7b656424957a558a9d0dd676

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]