linux-kernel - Re: [PATCH v5 1/9] lib: zstd: Add zstd compatibility wrapper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <B1AF526E-A34C-4CA1-B4CA-2DC5C5934C15@fb.com>
Date:   Tue, 10 Nov 2020 19:33:30 +0000
From:   Nick Terrell <terrelln@...com>
To:     "dsterba@...e.cz" <dsterba@...e.cz>
CC:     Chris Mason <clm@...com>, Christoph Hellwig <hch@...radead.org>,
        "Nick Terrell" <nickrterrell@...il.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
        "squashfs-devel@...ts.sourceforge.net" 
        <squashfs-devel@...ts.sourceforge.net>,
        "linux-f2fs-devel@...ts.sourceforge.net" 
        <linux-f2fs-devel@...ts.sourceforge.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>, Petr Malat <oss@...at.biz>,
        Johannes Weiner <jweiner@...com>,
        Niket Agarwal <niketa@...com>, Yann Collet <cyan@...com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v5 1/9] lib: zstd: Add zstd compatibility wrapper

> On Nov 10, 2020, at 7:25 AM, David Sterba <dsterba@...e.cz> wrote:
> 
> On Mon, Nov 09, 2020 at 02:01:41PM -0500, Chris Mason wrote:
>> On 6 Nov 2020, at 13:38, Christoph Hellwig wrote:
>>> You just keep resedning this crap, don't you?  Haven't you been told
>>> multiple times to provide a proper kernel API by now?
>> 
>> You do consistently ask for a shim layer, but you haven’t explained 
>> what we gain by diverging from the documented and tested API of the 
>> upstream zstd project.  It’s an important discussion given that we 
>> hope to regularly update the kernel side as they make improvements in 
>> zstd.
>> 
>> The only benefit described so far seems to be camelcase related, but if 
>> there are problems in the API beyond that, I haven’t seen you describe 
>> them.  I don’t think the camelcase alone justifies the added costs of 
>> the shim.
> 
> The API change in this patchset is adding churn that wouldn't be
> necessary if there were an upstream<->kernel API from the beginning.
> 
> The patch 5/9 is almost entirely renaming just some internal identifiers
> 
> -			      ZSTD_CStreamWorkspaceBound(params.cParams),
> -			      ZSTD_DStreamWorkspaceBound(ZSTD_BTRFS_MAX_INPUT));
> +			      ZSTD_estimateCStreamSize_usingCParams(params.cParams),
> +			      ZSTD_estimateDStreamSize(ZSTD_BTRFS_MAX_INPUT));
> 
> plus updating the names in the error strings. The compression API that
> filesystems need is simple:
> 
> - set up workspace and parameters
> - compress buffer
> - decompress buffer
> 
> We really should not care if upstream has 3 functions for initializing
> stream (ZSTD_initCStream/ZSTD_initStaticCStream/ZSTD_initCStream_advanced),
> or if the name changes again in the future.

Upstream will not change these function names. We guarantee the stable
portion of our API has a fixed ABI. The unstable portion doesn’t make this
guarantee, but we guarantee never to change function semantics in an
incompatible way, and to provide long deprecation periods (years) when we
delete functions.

For reference, the only functions we’ve deleted/modified since v1.0.0 when we
stabilized the zstd format 4 years ago are an old streaming API that was
deprecated before v1.0.0. We’ve added new functions, and provided new
recommended ways to use our API that we think are better. But, we highly
value not breaking our users code, so all the older APIs are still supported.

This churn is caused because the current version of zstd inside the kernel is
not upstream zstd. At the time of integration upstream zstd wasn’t ready to
be used as-is in the kernel. When I integrated zstd into the kernel, I should’ve
done more work to use upstream as-is. It was a mistake that I would like to
correct, so the kernel can benefit from the significant performance
improvements that upstream has made in the last few years.

> This should not require explicit explanation, this should be a natural
> requirement especially for separate projects that don't share the same
> coding style but have to be integrated in some way.

I’m not completely against providing a kernel shim. Personally, I don’t believe
it provides much benefit. But if the consensus of kernel developers is that a
shim provides a better API, then I’m happy to provide it. So far, I haven’t seen
a clear consensus either way. That leaves me kind of stuck.

Best,
Nick