Received-SPF: pass (google.com: domain of linux-crypto+bounces-3931-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 29 Apr 2024 17:41:29 +0200
From: David Sterba <dsterba@suse.cz>
To: Josef Bacik <josef@toxicpanda.com>
Cc: Giovanni Cabiddu <giovanni.cabiddu@intel.com>, clm@fb.com,
	dsterba@suse.com, herbert@gondor.apana.org.au,
	linux-btrfs@vger.kernel.org, linux-crypto@vger.kernel.org,
	qat-linux@intel.com, embg@meta.com, cyan@meta.com,
	brian.will@intel.com, weigang.li@intel.com
Subject: Re: [RFC PATCH 6/6] btrfs: zlib: add support for zlib-deflate
 through acomp
Message-ID: <20240429154129.GD2585@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20240426110941.5456-1-giovanni.cabiddu@intel.com>
 <20240426110941.5456-7-giovanni.cabiddu@intel.com>
 <20240429135645.GA3288472@perftesting>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240429135645.GA3288472@perftesting>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)

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.
> 
> 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'd skip the style and implementation details for now. The absence of
compression level support seems like the biggest problem, also in
combination with uncondtional use of the acomp interface. We'd have to
enhance the compression format specifier to make it configurable in the
sense: if accelerator is available use it, otherwise do CPU and
synchronous compression.

On the other hand, the compression levels are to trade off time and
space. If the QAT implementation with zlib level 9 is always better than
CPU compression then it's not that bad, not counting the possibly
misleading level to the users.

If QAT can also support ZSTD I'm not sure that lack of levels can work
there though, the memory overhead is bigger and it's a more complex
algorithm. Extending the acomp API with levels would be necessary.

Regarding the implementation, there are many allocations that set up the
async request. This is problematic as the compression is at the end of
the IO path and potentially called after memory pressure. We still do
some allocations there but also try not to fail due to ENOMEM, each
allocation is a new failure point. Anything that could be reused should
be in the workspace memory.