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

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

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

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

Hmm, I thought it as "I want to explicitely say to btrfs to compress
this file using whatever optins I set for the fs", e.g. in a situation
where a file was created without compress=, then compress= is used...but
that would be the exact same, as new data would be compressed all the
same because of compress=.
Another (very edge) use case I thought about was "file gets marked as
NOCOMPRESS, then gets truncated and compressible data gets written to it
so I want it to compress again" (since +c resets NOCOPRESS)...but I
hadn't tested it. Now that I did, I realized in that case it probably
wouldn't get past the compression heuristics so it wouldn't be marked as
NOCOMPRESS anyway.

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

It should be fine, as the current code also accepts "zlibabcd",
"zlib:invalid" and so on; it gets applied successfully with btrfs prop
set and gets rightly compressed using "zlib" (well, using the level from
the mount point because of the "bug", but yeah).

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

I'll change it back to the simple assignment and send v2 of the patch
series then.

Thanks again.

Best,
koraynilay

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