diff mbox series

[v4,10/13] ubifs: Use crypto_acomp interface

Message ID 349a78bc53d3620a29cc6105b55985db51aa0a11.1741954523.git.herbert@gondor.apana.org.au
State Superseded
Headers show
Series crypto: acomp - Add virtual address and folio support | expand

Commit Message

Herbert Xu March 14, 2025, 12:22 p.m. UTC 
Replace the legacy crypto compression interface with the new acomp
interface.

Remove the compression mutexes and the overallocation for memory
(the offender LZO has been fixed).

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 fs/ubifs/compress.c | 116 ++++++++++++++++++++++++++++----------------
 fs/ubifs/journal.c  |   2 +-
 fs/ubifs/ubifs.h    |  15 +-----
 3 files changed, 77 insertions(+), 56 deletions(-)

Comments

Herbert Xu March 15, 2025, 5:44 a.m. UTC | #1 
On Sat, Mar 15, 2025 at 01:15:09PM +0800, Zhihao Cheng wrote:
>
> Hi, Herbert. Can you show me which patch fixed the problem in LZO?

https://web.git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/commit/?id=cc47f07234f72cbd8e2c973cdbf2a6730660a463
 
> Does LZO guarantee the output data length smaller than input buffer length?
> Which commit fixed the issue?

The guarantee is that the algorithm will not write to the output
buffer beyond the specific buffer length.

For compression, you may specify a desired output length that is
smaller than the input buffer, automatically stopping the compression
if the input is incompressible.

Cheers,
Herbert Xu March 15, 2025, 9:02 a.m. UTC | #2 
On Sat, Mar 15, 2025 at 04:58:31PM +0800, Zhihao Cheng wrote:
>
> Hi, Herbert, I got some warning messages while running xfstests, it looks
> like the compressor returns error code.

Yes this is expected as incompressible data will now show up as
errors since we reduced the output buffer size due to LZO getting
fixed.  I'll silence that warning.

There are no reasons why compression should fail, other than the
data being incompressible.

Thanks,
Herbert Xu March 15, 2025, 9:12 a.m. UTC | #3 
On Sat, Mar 15, 2025 at 05:08:47PM +0800, Zhihao Cheng wrote:
>
> According to the warning message, current compressor is zstd. The output
> buffer size is limited only for LZO compressor by [1].

Any algorithm can and will produce output longer than the input,
if you give it enough output buffer.

Previously an output buffer length of 2x the input length was given
to all algorithms, meaning that they would all succeed no matter
whether the input can be compressed or not.

This has now been changed so that incompressible data is not
needlessly compressed all the way to the end.  In fact we should
reduce it further to eliminate the UBIFS_MIN_COMPRESS_DIFF check.

I will remove the warning on compression since failures are
expected and reduce the output buffer length further to remove
the post-compression length check.

Thanks,
Herbert Xu March 15, 2025, 9:32 a.m. UTC | #4 
On Sat, Mar 15, 2025 at 05:27:22PM +0800, Zhihao Cheng wrote:
>
> I think we should keep the warning, it would be better to distinguish the
> different error types.

Unfortunately that requires quite a bit of work.  If you would
like to restore the warning you will need to modify each algorithm
implementation to return a consistent error.  Right now it's all
over the place.

Thanks,
Herbert Xu March 15, 2025, 9:34 a.m. UTC | #5 
On Sat, Mar 15, 2025 at 05:32:27PM +0800, Herbert Xu wrote:
>
> Unfortunately that requires quite a bit of work.  If you would
> like to restore the warning you will need to modify each algorithm
> implementation to return a consistent error.  Right now it's all
> over the place.

Of course if you really want to keep the warning, then I can
just restore the existing worst-case output length so that
even incompressible data will succeed.

Cheers,
diff mbox series

Patch

diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
index 0b48cbab8a3d..9046e796876d 100644
--- a/fs/ubifs/compress.c
+++ b/fs/ubifs/compress.c
@@ -15,7 +15,7 @@ 
  * decompression.
  */
 
-#include <linux/crypto.h>
+#include <crypto/acompress.h>
 #include "ubifs.h"
 
 /* Fake description object for the "none" compressor */
@@ -26,11 +26,8 @@  static struct ubifs_compressor none_compr = {
 };
 
 #ifdef CONFIG_UBIFS_FS_LZO
-static DEFINE_MUTEX(lzo_mutex);
-
 static struct ubifs_compressor lzo_compr = {
 	.compr_type = UBIFS_COMPR_LZO,
-	.comp_mutex = &lzo_mutex,
 	.name = "lzo",
 	.capi_name = "lzo",
 };
@@ -42,13 +39,8 @@  static struct ubifs_compressor lzo_compr = {
 #endif
 
 #ifdef CONFIG_UBIFS_FS_ZLIB
-static DEFINE_MUTEX(deflate_mutex);
-static DEFINE_MUTEX(inflate_mutex);
-
 static struct ubifs_compressor zlib_compr = {
 	.compr_type = UBIFS_COMPR_ZLIB,
-	.comp_mutex = &deflate_mutex,
-	.decomp_mutex = &inflate_mutex,
 	.name = "zlib",
 	.capi_name = "deflate",
 };
@@ -60,13 +52,8 @@  static struct ubifs_compressor zlib_compr = {
 #endif
 
 #ifdef CONFIG_UBIFS_FS_ZSTD
-static DEFINE_MUTEX(zstd_enc_mutex);
-static DEFINE_MUTEX(zstd_dec_mutex);
-
 static struct ubifs_compressor zstd_compr = {
 	.compr_type = UBIFS_COMPR_ZSTD,
-	.comp_mutex = &zstd_enc_mutex,
-	.decomp_mutex = &zstd_dec_mutex,
 	.name = "zstd",
 	.capi_name = "zstd",
 };
@@ -80,6 +67,40 @@  static struct ubifs_compressor zstd_compr = {
 /* All UBIFS compressors */
 struct ubifs_compressor *ubifs_compressors[UBIFS_COMPR_TYPES_CNT];
 
+static int ubifs_compress_req(const struct ubifs_info *c,
+			      struct acomp_req *req,
+			      void *out_buf, int *out_len)
+{
+	struct crypto_wait wait;
+	int in_len = req->slen;
+	int err;
+
+	crypto_init_wait(&wait);
+	acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+				   crypto_req_done, &wait);
+	acomp_request_set_dst_dma(req, out_buf, *out_len);
+	err = crypto_acomp_compress(req);
+	err = crypto_wait_req(err, &wait);
+	*out_len = req->dlen;
+
+	if (unlikely(err)) {
+		ubifs_warn(c, "cannot compress %d bytes, compressor %s, error %d, leave data uncompressed",
+			   in_len,
+			   crypto_acomp_alg_name(crypto_acomp_reqtfm(req)),
+			   err);
+	} else if (in_len - *out_len < UBIFS_MIN_COMPRESS_DIFF) {
+		/*
+		 * If the data compressed only slightly, it is better
+		 * to leave it uncompressed to improve read speed.
+		 */
+		err = -E2BIG;
+	}
+
+	acomp_request_free(req);
+
+	return err;
+}
+
 /**
  * ubifs_compress - compress data.
  * @c: UBIFS file-system description object
@@ -112,23 +133,14 @@  void ubifs_compress(const struct ubifs_info *c, const void *in_buf,
 	if (in_len < UBIFS_MIN_COMPR_LEN)
 		goto no_compr;
 
-	if (compr->comp_mutex)
-		mutex_lock(compr->comp_mutex);
-	err = crypto_comp_compress(compr->cc, in_buf, in_len, out_buf,
-				   (unsigned int *)out_len);
-	if (compr->comp_mutex)
-		mutex_unlock(compr->comp_mutex);
-	if (unlikely(err)) {
-		ubifs_warn(c, "cannot compress %d bytes, compressor %s, error %d, leave data uncompressed",
-			   in_len, compr->name, err);
-		goto no_compr;
+	{
+		ACOMP_REQUEST_ALLOC(req, compr->cc, GFP_NOFS | __GFP_NOWARN);
+
+		acomp_request_set_src_nondma(req, in_buf, in_len);
+		err = ubifs_compress_req(c, req, out_buf, out_len);
 	}
 
-	/*
-	 * If the data compressed only slightly, it is better to leave it
-	 * uncompressed to improve read speed.
-	 */
-	if (in_len - *out_len < UBIFS_MIN_COMPRESS_DIFF)
+	if (err)
 		goto no_compr;
 
 	return;
@@ -139,6 +151,32 @@  void ubifs_compress(const struct ubifs_info *c, const void *in_buf,
 	*compr_type = UBIFS_COMPR_NONE;
 }
 
+static int ubifs_decompress_req(const struct ubifs_info *c,
+				struct acomp_req *req,
+				const void *in_buf, int in_len, int *out_len)
+{
+	struct crypto_wait wait;
+	int err;
+
+	crypto_init_wait(&wait);
+	acomp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
+				   crypto_req_done, &wait);
+	acomp_request_set_src_dma(req, in_buf, in_len);
+	err = crypto_acomp_decompress(req);
+	err = crypto_wait_req(err, &wait);
+	*out_len = req->dlen;
+
+	if (err)
+		ubifs_err(c, "cannot decompress %d bytes, compressor %s, error %d",
+			  in_len,
+			  crypto_acomp_alg_name(crypto_acomp_reqtfm(req)),
+			  err);
+
+	acomp_request_free(req);
+
+	return err;
+}
+
 /**
  * ubifs_decompress - decompress data.
  * @c: UBIFS file-system description object
@@ -155,7 +193,6 @@  void ubifs_compress(const struct ubifs_info *c, const void *in_buf,
 int ubifs_decompress(const struct ubifs_info *c, const void *in_buf,
 		     int in_len, void *out_buf, int *out_len, int compr_type)
 {
-	int err;
 	struct ubifs_compressor *compr;
 
 	if (unlikely(compr_type < 0 || compr_type >= UBIFS_COMPR_TYPES_CNT)) {
@@ -176,17 +213,12 @@  int ubifs_decompress(const struct ubifs_info *c, const void *in_buf,
 		return 0;
 	}
 
-	if (compr->decomp_mutex)
-		mutex_lock(compr->decomp_mutex);
-	err = crypto_comp_decompress(compr->cc, in_buf, in_len, out_buf,
-				     (unsigned int *)out_len);
-	if (compr->decomp_mutex)
-		mutex_unlock(compr->decomp_mutex);
-	if (err)
-		ubifs_err(c, "cannot decompress %d bytes, compressor %s, error %d",
-			  in_len, compr->name, err);
+	{
+		ACOMP_REQUEST_ALLOC(req, compr->cc, GFP_NOFS | __GFP_NOWARN);
 
-	return err;
+		acomp_request_set_dst_nondma(req, out_buf, *out_len);
+		return ubifs_decompress_req(c, req, in_buf, in_len, out_len);
+	}
 }
 
 /**
@@ -199,7 +231,7 @@  int ubifs_decompress(const struct ubifs_info *c, const void *in_buf,
 static int __init compr_init(struct ubifs_compressor *compr)
 {
 	if (compr->capi_name) {
-		compr->cc = crypto_alloc_comp(compr->capi_name, 0, 0);
+		compr->cc = crypto_alloc_acomp(compr->capi_name, 0, 0);
 		if (IS_ERR(compr->cc)) {
 			pr_err("UBIFS error (pid %d): cannot initialize compressor %s, error %ld",
 			       current->pid, compr->name, PTR_ERR(compr->cc));
@@ -218,7 +250,7 @@  static int __init compr_init(struct ubifs_compressor *compr)
 static void compr_exit(struct ubifs_compressor *compr)
 {
 	if (compr->capi_name)
-		crypto_free_comp(compr->cc);
+		crypto_free_acomp(compr->cc);
 }
 
 /**
diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 36ba79fbd2ff..7629ca9ecfe8 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -1625,7 +1625,7 @@  static int truncate_data_node(const struct ubifs_info *c, const struct inode *in
 	int err, dlen, compr_type, out_len, data_size;
 
 	out_len = le32_to_cpu(dn->size);
-	buf = kmalloc_array(out_len, WORST_COMPR_FACTOR, GFP_NOFS);
+	buf = kmalloc(out_len, GFP_NOFS);
 	if (!buf)
 		return -ENOMEM;
 
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
index 3375bbe0508c..7d0aaf5d2e23 100644
--- a/fs/ubifs/ubifs.h
+++ b/fs/ubifs/ubifs.h
@@ -124,13 +124,6 @@ 
 #define OLD_ZNODE_AGE 20
 #define YOUNG_ZNODE_AGE 5
 
-/*
- * Some compressors, like LZO, may end up with more data then the input buffer.
- * So UBIFS always allocates larger output buffer, to be sure the compressor
- * will not corrupt memory in case of worst case compression.
- */
-#define WORST_COMPR_FACTOR 2
-
 #ifdef CONFIG_FS_ENCRYPTION
 #define UBIFS_CIPHER_BLOCK_SIZE FSCRYPT_CONTENTS_ALIGNMENT
 #else
@@ -141,7 +134,7 @@ 
  * How much memory is needed for a buffer where we compress a data node.
  */
 #define COMPRESSED_DATA_NODE_BUF_SZ \
-	(UBIFS_DATA_NODE_SZ + UBIFS_BLOCK_SIZE * WORST_COMPR_FACTOR)
+	(UBIFS_DATA_NODE_SZ + UBIFS_BLOCK_SIZE)
 
 /* Maximum expected tree height for use by bottom_up_buf */
 #define BOTTOM_UP_HEIGHT 64
@@ -835,16 +828,12 @@  struct ubifs_node_range {
  * struct ubifs_compressor - UBIFS compressor description structure.
  * @compr_type: compressor type (%UBIFS_COMPR_LZO, etc)
  * @cc: cryptoapi compressor handle
- * @comp_mutex: mutex used during compression
- * @decomp_mutex: mutex used during decompression
  * @name: compressor name
  * @capi_name: cryptoapi compressor name
  */
 struct ubifs_compressor {
 	int compr_type;
-	struct crypto_comp *cc;
-	struct mutex *comp_mutex;
-	struct mutex *decomp_mutex;
+	struct crypto_acomp *cc;
 	const char *name;
 	const char *capi_name;
 };