Re: [PATCH v2 3/4] btrfs: Add zstd support

["Austin S. Hemmelgarn" <ahferroin7@gmail.com>]

On 2017-07-05 15:35, Nick Terrell wrote:
> On 7/5/17, 11:45 AM, "Austin S. Hemmelgarn" <ahferroin7@gmail.com> wrote:
>> On 2017-07-05 14:18, Adam Borowski wrote:
>>> On Wed, Jul 05, 2017 at 07:43:27AM -0400, Austin S. Hemmelgarn
>>> wrote:
>>>> On 2017-06-30 19:01, Nick Terrell wrote:
>>>>>> There is also the fact of deciding what to use for the default
>>>>>> when specified without a level.  This is easy for lzo and zlib,
>>>>>> where we can just use the existing level, but for zstd we would
>>>>>> need to decide how to handle a user just specifying 'zstd'
>>>>>> without a level.  I agree with E V that level 3 appears to be
>>>>>> the turnover point, and would suggest using that for the
>>>>>> default.
>>>>>
>>>>> I chose level 1 because I thought if we had to choose one
>>>>> speed/compression trade off, faster would be better. However,
>>>>> with a configerable compression level, level 3 is a great
>>>>> default, and is the default of the zstd CLI.
>>>> Actually, even if it's not configurable, I would prefer 3, as that
>>>> still performs better in both respects (speed and compression
>>>> ratio) than zlib while being sufficiently different from lzo
>>>> performance to make it easy to decide on one or the other.  As far
>>>> as configurable levels for regular usage on a filesystem, there are
>>>> only three levels you benchmarked that I would be interested in,
>>>> namely level 1 (highly active data on slow storage with a fast
>>>> CPU), 3 (stuff I would use zlib for today), and 15 (stuff I would
>>>> use out-of-band compression for today (for example, archival
>>>> storage)).
>>>
>>> If you guys are going to argue between 1 and 3, just go the cracovian
>>> deal and settle at 2. :þ
> 
> I'll change the default to level 3 in the next update.
> 
>>>
>>> But more seriously: zstd looks so much better than lzo and zlib than
>>> I'd suggest making it the default compression in cases where there's
>>> no way to choose, such as chattr +c.  But then, changing the default
>>> before the previous LTS kernel can mount it would be irresponsible --
>>> thus, if you can get it into 4.14, we're looking at 4.19 at soonest
>>> (or later if we consider distro kernels).
>> To be entirely honest, we probably should have switched to LZO as the
>> default a while back to put things more in-line with ZFS (which
>> traditionally favors performance for in-line compression) and Windows
>> (which uses a custom LZ77 derivative that's wicked fast on most modern
>> systems).  I would say that as soon as we get to the point that the last
>> two LTS releases support it, zstd should probably become the default.
>>
>> Also, slightly OT, but I would love to have the ability to set per
>> volume (not subvolume but volume itself) what to use for compression
>> when no algorithm is specified.
>>>
>>> Which means the timing is quite tight: if, per DSterba's request,
>>> /lib/ parts are going via a non-btrfs tree, there'll be not enough
>>> adequate testing in -next.  Thus, would it be possible to have /lib/
>>> patches in btrfs-next but not in for-linus?  That would allow testing
>>> so you can catch the LTS train.
>>>
>>>>>>> So, I don't see any problem making the level configurable.
>>>>>> I would actually love to see this, I regularly make use of
>>>>>> varying compression both on BTRFS (with separate filesystems)
>>>>>> and on the ZFS-based NAS systems we have at work (where it can
>>>>>> be set per-dataset) to allow better compression on less
>>>>>> frequently accessed data.
>>>>>
>>>>> I would love to see configurable compression level as well. Would
>>>>> you want me to add it to my patch set, or should I adapt my patch
>>>>> set to work on top of it when it is ready?
>>>
>>> Note that as zstd is new, there's no backwards compat to care of,
>>> thus you are free to use whatever -o/prop syntax you'd like.  If zstd
>>> ends up being configurable while zlib is not -- oh well, there's no
>>> reason to use zlib anymore other than for mounting with old kernels,
>>> in which case we can't use configurable props anyway.  Unlike zlib,
>>> lzo is not strictly worse than configurable zstd, but it has only one
>>> level so there's nothing to configure as well.
>>>
>>> Thus, I'd suggest skipping the issue and implement levels for zstd
>>> only.
>> I would mostly agree, with one possible exception.  _If_ zlib at the max
>> level gets similar compression ratios to zstd on it's higher levels
>> _and_ it also gets better performance on at least one aspect (I think
>> zlib at level 9 will probably have better compression performance than
>> zstd at level 15, but I'm not sure about the compression ratio), then I
>> would argue that it might be worth implementing levels for zlib.
>> Actually determining that will of course require proper testing, but
>> that's probably better left as a separate discussion.
> 
> For every zlib level, there is one or more level if zstd that performes
> better in all of compression speed, decompression speed, and compression
> level. zstd also has levels that compress better than zlib, but slower
> compression speed and faster decompression speed.
It's the slower compression speed that has me arguing for the 
possibility of configurable levels on zlib.  11MB/s is painfully slow 
considering that most decent HDD's these days can get almost 5-10x that 
speed with no compression.  There are cases (WORM pattern archival 
storage for example) where slow writes to that degree may be acceptable, 
but for most users they won't be, and zlib at level 9 would probably be 
a better choice.  I don't think it can beat zstd at level 15 for 
compression ratio, but if they're even close, then zlib would still be a 
better option at that high of a compression level most of the time.
> 
> These benchmarks are run in userland, but I expect the results to be the
> same in BtrFS. See the lzbench compression benchmark
> https://github.com/inikep/lzbench. There are corner cases where zlib can
> compress better than zstd, but I wouldn't expect to see them often, and
> the gains on other files should offset the small loss on the edge cases.
> 
>>> As for testing: I assume you guys are doing stress testing on amd64
>>> already, right?  I got two crappy arm64 machines but won't be able to
>>> test there before 4.13 (Icenowy has been lazy and didn't push any
>>> near-mainline patch set recently; every single Pine64/Pinebook kernel
>>> pushed by anyone but her didn't work for me so I don't even bother
>>> trying anymore).  On the other hand, the armhf box I'm running Debian
>>> rebuilds on is a gem among cheap SoCs.  After Nick fixed the
>>> flickering workspaces bug, there were no further hiccups; on monday I
>>> switched to 4.12.0 + btrfs-for-4.13 + zstd (thus luckily avoiding
>>> that nowait aio bug), also no explosions yet.
>> Which reminds me, I forgot to mention in my last e-mail, I ran stress
>> tests over the holiday weekend with pretty much the same kernel
>> combination on QEMU instances for amd64, i686, armv7a, armv6 (EABI only
>> on both), arm64, ppc64 (both big and little endian), and 32-bit MIPS
>> (new ABI only, also both big and little endian), and saw no issues
>> relating to zstd or BTRFS (I did run into some other issues, but they
>> were because I still don't have things quite configured properly for
>> this new testing setup).
> 
> Thanks for running the stress tests!
> 
Always glad to help.