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

[David Sterba]

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.