Received-SPF: pass (google.com: domain of linux-crypto+bounces-3930-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 16:21:46 +0100
From: "Cabiddu, Giovanni" <giovanni.cabiddu@intel.com>
To: Josef Bacik <josef@toxicpanda.com>, <herbert@gondor.apana.org.au>
CC: <clm@fb.com>, <dsterba@suse.com>, <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: <Zi+7CnWeF9+DUXpK@gcabiddu-mobl.ger.corp.intel.com>
References: <20240426110941.5456-1-giovanni.cabiddu@intel.com>
 <20240426110941.5456-7-giovanni.cabiddu@intel.com>
 <20240429135645.GA3288472@perftesting>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240429135645.GA3288472@perftesting>
Organization: Intel Research and Development Ireland Ltd - Co. Reg. #308263 -
 Collinstown Industrial Park, Leixlip, County Kildare - Ireland
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?zIJMhVqg9wugnbHP0w7ZN8JFDLDlPa08zKfuDTAdy63yh3wzuO7hMAaK9HJu?=
 =?us-ascii?Q?mts5bDGp5O9Oq9mXELTjaWmvCfxiMARps75QU447rHe0y4vhSO1cvCFIcnkO?=
 =?us-ascii?Q?nyMs2XAq4lsze17ZC4jXNUp6yXT4zgqgtaCad6yuq1pKHyi5hQDtz/EAYM9F?=
 =?us-ascii?Q?PpHsFY3f3JnzWtOnIW8X0qe5onEoewEVVUq3BZT0XIGvb5F9EY3wwmIqFGn5?=
 =?us-ascii?Q?jH/AZtcRVzLgGOWeFCGRbsTrhJopXPRfbGua210yzP31WmXJgT2rSfkB66Kq?=
 =?us-ascii?Q?XAlkbntxZa2InJxmxJKKG6ZOXzvh5W3ozKXXKMj6zaJ8W5EVx3WpopR+JveV?=
 =?us-ascii?Q?pEe3Q+Bu2mVpMXtz395ecO2LXchJDEyJGDNa8fyW4bU9rmQCG9fQktasAZOj?=
 =?us-ascii?Q?MVXwBvwmQStWZyKemiK++QhFwLRfgdhoSKXg7qUFYihI56c3d/isUNGhwdU3?=
 =?us-ascii?Q?jFfBC2/aZifF62aSALPmxJ+3hH5VbY3MY2HfKprT42VEyABxPsNdZ0Vv5qMU?=
 =?us-ascii?Q?zCXxS4wpOPuph5IUnHC4fC6wfpmo9kFKdmZ13NovK2medFHthN1x84a6KGqW?=
 =?us-ascii?Q?rJfnGiEyfbVgl5A3ZXf4/1RnH/VJqpkoFUwvNJ1YeLxbW4uLqsy3ElDvPCzZ?=
 =?us-ascii?Q?cOUmZsZ+I9z6qFluOHxOh0u/cYjbvwp/2Bq/R7otqODihN/20Od9HlY2/24O?=
 =?us-ascii?Q?kxEVC9mXPxTsariYVW1njb8pfur2s7XY2Pl4jFLa+VhAURCKQ0a09yO8fseN?=
 =?us-ascii?Q?dMtwKTwqVqcVRqZ1FZsORxwkCm2DR2bbyBSegmILg+dAZUPm2oFIDXeMEOEO?=
 =?us-ascii?Q?cX0a8uegUiqC2PKpo/zogTqD1FM4RPueW/kubiDwz8P4axvALtVYPw2guzHo?=
 =?us-ascii?Q?4mKI5/YlaBFDzP3epo/QERTQKF9dadLwhG0zWTqGbdzbqudLdlZFvzZO6lVp?=
 =?us-ascii?Q?v7gktHQaXilfqnUVnG6p61KREg0ZSB6f/N871KlCFBfws4mTonZC+VXFYLHH?=
 =?us-ascii?Q?iXC9JEwxhrzlDRs0FttpnniMAyOeOCzbYbNAPlPxULeEcp4K3k8bipkThFtg?=
 =?us-ascii?Q?eIVdG12BnyO2k6qxtILzDq03+dp3gblKrppMfuqysk5pLjam8HUB78avtYrN?=
 =?us-ascii?Q?uJkKzrxk+sSC1WceUlGzHy5yLxqjl1N3Ll6f4fYlSPbJuTLug4Y66CToYR7M?=
 =?us-ascii?Q?B9dPl/D+Dt1rmmYEP59d1auPfkA7LQR/bfMNgmjXynxYv9nveyD9Cl/q4Lyq?=
 =?us-ascii?Q?oqJTboESjuUcIDkCAmuEBgouQPk4WeC+YGOaYf1NczziCKhU3X/LGjR+qu6y?=
 =?us-ascii?Q?+dGVwLtTxVBliZCQzTfBALHeOg9mrhk5VdIanUvk4X85RPIpYcSksODjG9mA?=
 =?us-ascii?Q?ZRuU9ejK/XP8p8+qN3ILOdI1Or11r6cfdO3XEPBsoLX0RctFo39DwbQPPgn1?=
 =?us-ascii?Q?q0oczxpdM/Y2aK/3KYzlsFsoMwI0Hp3p6yKtl1sH9baAUPrTek2yLwlucK2M?=
 =?us-ascii?Q?uytpFHP3IO0atSbmpsJXNNmxsOHRPztAgIJyziI3uwzzN+3FQMKNDhyfsZs2?=
 =?us-ascii?Q?Ut5WubkrQ7nnrlcqBzvVt8cM4kIr+EeX1xNsbOZRjAL7XsLMTGygk9tCxx4M?=
 =?us-ascii?Q?Bg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0295e4e5-acfa-4b6d-76e3-08dc686019fe
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6366.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2024 15:21:54.4188
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jdN3aa8j+i+VZignCGoQ+SsTssuvF1x0svGkX/N2w9SNhv5noFBOPUA/gvUPVtZDzNwBswTKVqb8MUr9MB/FKcM9YUN+kAEpM0NH62W0e78=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7956
X-OriginatorOrg: 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