Re: [PATCH 5/9] btrfs: zstd: Switch to the zstd-1.4.6 API

[Nick Terrell]

> On Sep 16, 2020, at 7:46 AM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> 
> On Wed, Sep 16, 2020 at 10:43:04AM -0400, Chris Mason wrote:
>> Otherwise we just end up with drift and kernel-specific bugs that are harder
>> to debug.  To the extent those APIs make us contort the kernel code, I???m
>> sure Nick is interested in improving things in both places.
> 
> Seriously, we do not care elsewhere.  Why would zlib be any different?
> 
>> There are probably 1000 constructive ways to have that conversation.  Please
>> choose one of those instead of being an asshole.
> 
> I think you are the asshole here by ignoring the practices we are using
> elsewhere and think your employers pet project is somehow special.  It
> is not, and claiming so is everything but constructive.

My goal in updating the zstd kernel to use the upstream API directly is to
make frequent syncs into the kernel easy. This is important so the kernel
doesn't miss out on bug fixes and performance improvements.

The upstream zstd is continuously fuzzed and is battle tested in production
and across many different projects external to Facebook. That means that
zstd-1.4.6 has an additional 3 years of continuous fuzzing, as well as
improvements to our fuzz and test suite.

The zstd version in the kernel works fine. But, you can see that the version
that got imported stagnated where upstream had 14 released versions. I
don't think it makes sense to have kernel developers maintain their own copy
of zstd. Their time would be better spent working on the rest of the kernel.
Using upstream directly lets the kernel profit from the work that we, the zstd
developers, are doing. And it still allows kernel developers to fix bugs if any
show up, and we can back-port them to upstream.

For example, I’ve measured that BtrFS decompression + read performance
is improved 15% with this patch. And ZRAM performance improves 30%.
And SquashFS decompression + read performance improves 15%.

Admittedly, the API provided for static workspace allocation is verbose. Most
zstd users don’t need it, so our efforts to improve the ergonomics of the API
haven’t been focused here. At this point, we couldn’t rename these APIs easily,
since we have users relying on our API. It could be done, because we don’t
guarantee ABI stability for this portion of the API, but we would have to have
a good reason for it.

One possibility is to have a kernel wrapper on top of the zstd API to make it
more ergonomic. I personally don’t really see the value in it, since it adds
another layer of indirection between zstd and the caller, but it could be done.

Of all the compressors in the kernel, only lz4 and zstd are under active
development. And lz4 has switched to using the upstream API directly.
Xz does see a little bit of development, but nothing has been synced to the
kernel.

Best,
Nick