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

[Nick Terrell]

> On Sep 17, 2020, at 7:28 AM, Chris Mason <clm@xxxxxx> wrote:
> 
> On 17 Sep 2020, at 6:04, Christoph Hellwig wrote:
> 
>> On Wed, Sep 16, 2020 at 09:35:51PM -0400, Rik van Riel wrote:
>>>> 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.
>>> 
>>> Zstd would not be the first part of the kernel to
>>> come from somewhere else, and have wrappers when
>>> it gets integrated into the kernel. There certainly
>>> is precedence there.
>>> 
>>> It would be interesting to know what Christoph's
>>> preference is.
>> 
>> Yes, I think kernel wrappers would be a pretty sensible step forward.
>> That also avoid the need to do strange upgrades to a new version,
>> and instead we can just change APIs on a as-needed basis.
> 
> When we add wrappers, we end up creating a kernel specific API that doesn’t match the upstream zstd docs, and it doesn’t leverage as much of the zstd fuzzing and testing.
> 
> So we’re actually making kernel zstd slightly less usable in hopes that our kernel specific part of the API is familiar enough to us that it makes zstd more usable.  There’s no way to compare the two until the wrappers are done, but given the code today I’d prefer that we focus on making it really easy to track upstream.  I really understand Christoph’s side here, but I’d rather ride a camel with the group than go it alone.
> 
> I’d also much rather spend time on any problems where the structure of the zstd APIs don’t fit the kernel’s needs.  The btrfs streaming compression/decompression looks pretty clean to me, but I think Johannes mentioned some possibilities to improve things for zswap (optimizations for page-at-atime).  If there are places where the zstd memory management or error handling don’t fit naturally into the kernel, that would also be higher on my list.

This update includes the recent optimizations for ZSwap that I've made, which
gives a 30% speed boost for page-at-a-time decompression.

We're very open to improving and changing zstd to better fit the needs of the
kernel. If there are use cases that can't use the existing API, or the existing
API isn't optimal, or any other problems, we’re happy to help figure out the
best solution. Opening an issue on our upstream GitHub repo is the best way to
get our attention

-Nick

> Fixing those are probably going to be much easier if we’re close to the zstd upstream, again so that we can leverage testing and long term code maintenance done there.
> 
> -chris