Re: [PATCH 1/3] erofs-utils: mkfs: merge erofs_compressor_setlevel() into erofs_compressor_init()

[Gao Xiang <xiang@kernel.org>]

Hi Yifan,

On Sun, Jan 07, 2024 at 02:10:40AM +0800, Yifan Zhao wrote:
> Currently erofs_compressor_setlevel() is only called once just after
> erofs_compressor_init() while initializing compressors. Let's just hide
> this interface and set the compression level in erofs_compressor_init().
> 
> BTW, if the user gives a compression level to an algorithm which doesn't
> support it, let's report a warning rather than early aborting. Besides,

Really, we should error out invalid standard compression levels
instead of just ignoring.

Thanks,
Gao Xiang

> we do not need to assign the {default,best}_level for such an algorithm
> in erofs_compressor struct.
> 
> Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
> ---
>  lib/compress.c       |  6 +-----
>  lib/compressor.c     | 28 ++++++++++++++--------------
>  lib/compressor.h     |  5 ++---
>  lib/compressor_lz4.c |  2 --
>  4 files changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/compress.c b/lib/compress.c
> index 8f61f92..216aeb4 100644
> --- a/lib/compress.c
> +++ b/lib/compress.c
> @@ -1199,11 +1199,7 @@ int z_erofs_compress_init(struct erofs_sb_info *sbi, struct erofs_buffer_head *s
>  	for (i = 0; cfg.c_compr_alg[i]; ++i) {
>  		struct erofs_compress *c = &erofs_ccfg[i].handle;
>  
> -		ret = erofs_compressor_init(sbi, c, cfg.c_compr_alg[i]);
> -		if (ret)
> -			return ret;
> -
> -		ret = erofs_compressor_setlevel(c, cfg.c_compr_level[i]);
> +		ret = erofs_compressor_init(sbi, c, cfg.c_compr_alg[i], cfg.c_compr_level[i]);
>  		if (ret)
>  			return ret;
>  
> diff --git a/lib/compressor.c b/lib/compressor.c
> index a71436a..7ec51c2 100644
> --- a/lib/compressor.c
> +++ b/lib/compressor.c
> @@ -77,20 +77,8 @@ int erofs_compress_destsize(const struct erofs_compress *c,
>  	return c->alg->c->compress_destsize(c, src, srcsize, dst, dstsize);
>  }
>  
> -int erofs_compressor_setlevel(struct erofs_compress *c, int compression_level)
> -{
> -	DBG_BUGON(!c->alg);
> -	if (c->alg->c->setlevel)
> -		return c->alg->c->setlevel(c, compression_level);
> -
> -	if (compression_level >= 0)
> -		return -EINVAL;
> -	c->compression_level = 0;
> -	return 0;
> -}
> -
> -int erofs_compressor_init(struct erofs_sb_info *sbi,
> -			  struct erofs_compress *c, char *alg_name)
> +int erofs_compressor_init(struct erofs_sb_info *sbi, struct erofs_compress *c,
> +			  char *alg_name, int compression_level)
>  {
>  	int ret, i;
>  
> @@ -113,6 +101,18 @@ int erofs_compressor_init(struct erofs_sb_info *sbi,
>  			continue;
>  
>  		ret = erofs_algs[i].c->init(c);
> +		if (ret)
> +			return ret;
> +
> +		if (erofs_algs[i].c->setlevel) {
> +			ret = erofs_algs[i].c->setlevel(c, compression_level);
> +		} else {
> +			if (compression_level >= 0)
> +				erofs_warn(
> +					"compression level %d is ignored for %s",
> +					compression_level, alg_name);
> +			c->compression_level = 0;
> +		}
>  		if (!ret) {
>  			c->alg = &erofs_algs[i];
>  			return 0;
> diff --git a/lib/compressor.h b/lib/compressor.h
> index 6875cf1..ec5485d 100644
> --- a/lib/compressor.h
> +++ b/lib/compressor.h
> @@ -55,9 +55,8 @@ int erofs_compress_destsize(const struct erofs_compress *c,
>  			    const void *src, unsigned int *srcsize,
>  			    void *dst, unsigned int dstsize);
>  
> -int erofs_compressor_setlevel(struct erofs_compress *c, int compression_level);
> -int erofs_compressor_init(struct erofs_sb_info *sbi,
> -		struct erofs_compress *c, char *alg_name);
> +int erofs_compressor_init(struct erofs_sb_info *sbi, struct erofs_compress *c,
> +			  char *alg_name, int compression_level);
>  int erofs_compressor_exit(struct erofs_compress *c);
>  
>  #endif
> diff --git a/lib/compressor_lz4.c b/lib/compressor_lz4.c
> index 6677693..f4e72c3 100644
> --- a/lib/compressor_lz4.c
> +++ b/lib/compressor_lz4.c
> @@ -37,8 +37,6 @@ static int compressor_lz4_init(struct erofs_compress *c)
>  }
>  
>  const struct erofs_compressor erofs_compressor_lz4 = {
> -	.default_level = 0,
> -	.best_level = 0,
>  	.init = compressor_lz4_init,
>  	.exit = compressor_lz4_exit,
>  	.compress_destsize = lz4_compress_destsize,
> -- 
> 2.43.0
>