lkml.org 
[lkml]   [2020]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 1/9] lib: zstd: Add zstd compatibility wrapper
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.

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.

\
 
 \ /
  Last update: 2020-11-10 16:27    [W:0.122 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site