Re: [RFC PATCH 6/6] btrfs: zlib: add support for zlib-deflate through acomp

["Cabiddu, Giovanni" <giovanni.cabiddu@intel.com>]

On Mon, Apr 29, 2024 at 09:56:45AM -0400, Josef Bacik wrote:
> On Fri, Apr 26, 2024 at 11:54:29AM +0100, Giovanni Cabiddu wrote:
> > From: Weigang Li <weigang.li@intel.com>
> > 
> > Add support for zlib compression and decompression through the acomp
> > APIs.
> > Input pages are added to an sg-list and sent to acomp in one request.
> > Since acomp is asynchronous, the thread is put to sleep and then the CPU
> > is freed up. Once compression is done, the acomp callback is triggered
> > and the thread is woke up.
> > 
> > This patch doesn't change the BTRFS disk format, this means that files
> > compressed by hardware engines can be de-compressed by the zlib software
> > library, and vice versa.
> > 
> > Limitations:
> >   * The implementation tries always to use an acomp even if only
> >     zlib-deflate-scomp is present
> >   * Acomp does not provide a way to support compression levels
> 
> That's a non-starter.  We can't just lie to the user about the compression level
> that is being used.  If the user just does "-o compress=zlib" then you need to
> update btrfs_compress_set_level() to figure out the compression level that acomp
> is going to use and set that appropriately, so we can report to the user what is
> actually being used.
> 
> Additionally if a user specifies a compression level you need to make sure we
> don't do acomp if it doesn't match what acomp is going to do.
Thanks for the feedback. We should then extend the acomp API to take the
compression level.
@Herbert, do you have any objection if we add the compression level to
the acomp tfm and we add an API to set it? Example:

    tfm = crypto_alloc_acomp("deflate", 0, 0);
    acomp_set_level(tfm, compression_level);

> Finally, for the normal code review, there's a bunch of things that need to be
> fixed up before I take a closer look
> 
> - We don't use pr_(), we have btrfs specific printk helpers, please use those.
> - We do 1 variable per line, fix up the variable declarations in your functions.
I see that the code in fs/btrfs/zlib.c uses both pr_() and more than one
variable per line. If we change it, will mixed style be a concern?

-- 
Giovanni