Re: [v5 PATCH 13/14] ubifs: Use crypto_acomp interface

Zhihao Cheng
2025-03-19T09:45:03Z

在 2025/3/15 18:30, Herbert Xu 写道:
> 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).
>
> Cap the output buffer length for compression to eliminate the
> post-compression check for UBIFS_MIN_COMPRESS_DIFF.
>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> ---
> fs/ubifs/compress.c | 106 ++++++++++++++++++++++++++------------------
> fs/ubifs/journal.c | 2 +-
> fs/ubifs/ubifs.h | 15 +------
> 3 files changed, 67 insertions(+), 56 deletions(-)
>

Tested-by: Zhihao Cheng <chengzhihao1@huawei.com> # For xfstests
Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> diff --git a/fs/ubifs/compress.c b/fs/ubifs/compress.c
> index 0b48cbab8a3d..a241ba01c9a8 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,30 @@ 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,
> + const char *compr_name)
> +{
> + struct crypto_wait wait;
> + int in_len = req->slen;
> + int dlen = *out_len;
> + int err;
> +
> + dlen = min(dlen, in_len - UBIFS_MIN_COMPRESS_DIFF);
> +
> + 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, dlen);
> + err = crypto_acomp_compress(req);
> + err = crypto_wait_req(err, &wait);
> + *out_len = req->dlen;
> + acomp_request_free(req);
> +
> + return err;
> +}
> +
> /**
> * ubifs_compress - compress data.
> * @c: UBIFS file-system description object
> @@ -112,23 +123,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, compr->name);
> }
>
> - /*
> - * 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 +141,31 @@ 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,
> + const char *compr_name)
> +{
> + 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, compr_name, err);
> +
> + acomp_request_free(req);
> +
> + return err;
> +}
> +
> /**
> * ubifs_decompress - decompress data.
> * @c: UBIFS file-system description object
> @@ -155,7 +182,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 +202,13 @@ 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,
> + compr->name);
> + }
> }
>
> /**
> @@ -199,7 +221,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 +240,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;
> };
>