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

[David Sterba <dsterba@suse.cz>]

On Mon, Apr 29, 2024 at 04:21:46PM +0100, Cabiddu, Giovanni wrote:
> 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?

I have a work in progress to rework the messages in compression. The
messages with pr_() helpers are there for historical reasons and using
proper btrfs_info/... need extracting structures like fs_info from
various data. Josef's comment is valid but you can skip that for the QAT
series.