[PATCH 1/4] f2fs: fix to check lz4hc compression when CONFIG_F2FS_FS_LZ4HC is not enabled

[PATCH 1/4] f2fs: fix to check lz4hc compression when CONFIG_F2FS_FS_LZ4HC is not enabled

[Yangtao Li]

f2fs supports lz4 compression algorithm and lz4hc compression algorithm,
which the level parameter needs to be passed in. When CONFIG_F2FS_FS_LZ4HC
is not enabled, even if there is no problem with the level parameter, add
the level parameter to the lz4 algorithm will cause the mount to fail.

Let's change it to be the same as other compression algorithms. When the
kernel does not enable the algorithm, ignore this parameter and print msg.

Fixes: 3fde13f817e2 ("f2fs: compress: support compress level")
Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/super.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index d8a65645ee48..ad5df4d5c39a 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -588,19 +588,11 @@ static int f2fs_test_compress_extension(struct f2fs_sb_info *sbi)
 	return 0;
 }
 
-#ifdef CONFIG_F2FS_FS_LZ4
+#ifdef CONFIG_F2FS_FS_LZ4HC
 static int f2fs_set_lz4hc_level(struct f2fs_sb_info *sbi, const char *str)
 {
-#ifdef CONFIG_F2FS_FS_LZ4HC
 	unsigned int level;
-#endif
 
-	if (strlen(str) == 3) {
-		F2FS_OPTION(sbi).compress_level = 0;
-		return 0;
-	}
-
-#ifdef CONFIG_F2FS_FS_LZ4HC
 	str += 3;
 
 	if (str[0] != ':') {
@@ -617,10 +609,6 @@ static int f2fs_set_lz4hc_level(struct f2fs_sb_info *sbi, const char *str)
 
 	F2FS_OPTION(sbi).compress_level = level;
 	return 0;
-#else
-	f2fs_info(sbi, "kernel doesn't support lz4hc compression");
-	return -EINVAL;
-#endif
 }
 #endif
 
@@ -1085,10 +1073,19 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 #endif
 			} else if (!strncmp(name, "lz4", 3)) {
 #ifdef CONFIG_F2FS_FS_LZ4
-				ret = f2fs_set_lz4hc_level(sbi, name);
-				if (ret) {
-					kfree(name);
-					return -EINVAL;
+				if (strlen(name) == 3) {
+					F2FS_OPTION(sbi).compress_level = 0;
+				} else {
+#ifdef CONFIG_F2FS_FS_LZ4HC
+					ret = f2fs_set_lz4hc_level(sbi, name);
+					if (ret) {
+						kfree(name);
+						return -EINVAL;
+					}
+#else
+					f2fs_info(sbi, "kernel doesn't support lz4hc compression");
+					break;
+#endif
 				}
 				F2FS_OPTION(sbi).compress_algorithm =
 								COMPRESS_LZ4;
-- 
2.25.1

[PATCH 2/4] f2fs: introduce f2fs_set_compress_level()

[Yangtao Li]

f2fs_set_lz4hc_level() and f2fs_set_zstd_level() have common code,
let's introduce f2fs_set_compress_level() to do sanity compress level
check.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/super.c | 57 +++++++++++++++----------------------------------
 1 file changed, 17 insertions(+), 40 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index ad5df4d5c39a..b5fbe9939390 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -588,40 +588,12 @@ static int f2fs_test_compress_extension(struct f2fs_sb_info *sbi)
 	return 0;
 }
 
-#ifdef CONFIG_F2FS_FS_LZ4HC
-static int f2fs_set_lz4hc_level(struct f2fs_sb_info *sbi, const char *str)
+static int __maybe_unused f2fs_set_compress_level(struct f2fs_sb_info *sbi,
+					const char *str, const char *alg_name,
+					unsigned int min, unsigned int max)
 {
 	unsigned int level;
-
-	str += 3;
-
-	if (str[0] != ':') {
-		f2fs_info(sbi, "wrong format, e.g. <alg_name>:<compr_level>");
-		return -EINVAL;
-	}
-	if (kstrtouint(str + 1, 10, &level))
-		return -EINVAL;
-
-	if (level < LZ4HC_MIN_CLEVEL || level > LZ4HC_MAX_CLEVEL) {
-		f2fs_info(sbi, "invalid lz4hc compress level: %d", level);
-		return -EINVAL;
-	}
-
-	F2FS_OPTION(sbi).compress_level = level;
-	return 0;
-}
-#endif
-
-#ifdef CONFIG_F2FS_FS_ZSTD
-static int f2fs_set_zstd_level(struct f2fs_sb_info *sbi, const char *str)
-{
-	unsigned int level;
-	int len = 4;
-
-	if (strlen(str) == len) {
-		F2FS_OPTION(sbi).compress_level = 0;
-		return 0;
-	}
+	int len = strlen(alg_name);
 
 	str += len;
 
@@ -632,8 +604,8 @@ static int f2fs_set_zstd_level(struct f2fs_sb_info *sbi, const char *str)
 	if (kstrtouint(str + 1, 10, &level))
 		return -EINVAL;
 
-	if (!level || level > zstd_max_clevel()) {
-		f2fs_info(sbi, "invalid zstd compress level: %d", level);
+	if (level < min || level > max) {
+		f2fs_info(sbi, "invalid %s compress level: %d", alg_name, level);
 		return -EINVAL;
 	}
 
@@ -641,7 +613,6 @@ static int f2fs_set_zstd_level(struct f2fs_sb_info *sbi, const char *str)
 	return 0;
 }
 #endif
-#endif
 
 static int parse_options(struct super_block *sb, char *options, bool is_remount)
 {
@@ -1077,7 +1048,8 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 					F2FS_OPTION(sbi).compress_level = 0;
 				} else {
 #ifdef CONFIG_F2FS_FS_LZ4HC
-					ret = f2fs_set_lz4hc_level(sbi, name);
+					ret = f2fs_set_compress_level(sbi, name, "lz4",
+						LZ4HC_MIN_CLEVEL, LZ4HC_MAX_CLEVEL);
 					if (ret) {
 						kfree(name);
 						return -EINVAL;
@@ -1094,10 +1066,15 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
 #endif
 			} else if (!strncmp(name, "zstd", 4)) {
 #ifdef CONFIG_F2FS_FS_ZSTD
-				ret = f2fs_set_zstd_level(sbi, name);
-				if (ret) {
-					kfree(name);
-					return -EINVAL;
+				if (strlen(name) == 4) {
+					F2FS_OPTION(sbi).compress_level = 0;
+				} else {
+					ret = f2fs_set_compress_level(sbi, name, "zstd", 1,
+						zstd_max_clevel());
+					if (ret) {
+						kfree(name);
+						return -EINVAL;
+					}
 				}
 				F2FS_OPTION(sbi).compress_algorithm =
 								COMPRESS_ZSTD;
-- 
2.25.1

[PATCH 3/4] f2fs: set default compress option only when sb_has_compression

[Yangtao Li]

If the compress feature is not enabled, there is no need to set
compress-related parameters.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/super.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index b5fbe9939390..68ccc9f54c2d 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2039,10 +2039,12 @@ static void default_options(struct f2fs_sb_info *sbi)
 	F2FS_OPTION(sbi).fsync_mode = FSYNC_MODE_POSIX;
 	F2FS_OPTION(sbi).s_resuid = make_kuid(&init_user_ns, F2FS_DEF_RESUID);
 	F2FS_OPTION(sbi).s_resgid = make_kgid(&init_user_ns, F2FS_DEF_RESGID);
-	F2FS_OPTION(sbi).compress_algorithm = COMPRESS_LZ4;
-	F2FS_OPTION(sbi).compress_log_size = MIN_COMPRESS_LOG_SIZE;
-	F2FS_OPTION(sbi).compress_ext_cnt = 0;
-	F2FS_OPTION(sbi).compress_mode = COMPR_MODE_FS;
+	if (f2fs_sb_has_compression(sbi)) {
+		F2FS_OPTION(sbi).compress_algorithm = COMPRESS_LZ4;
+		F2FS_OPTION(sbi).compress_log_size = MIN_COMPRESS_LOG_SIZE;
+		F2FS_OPTION(sbi).compress_ext_cnt = 0;
+		F2FS_OPTION(sbi).compress_mode = COMPR_MODE_FS;
+	}
 	F2FS_OPTION(sbi).bggc_mode = BGGC_MODE_ON;
 	F2FS_OPTION(sbi).memory_mode = MEMORY_MODE_NORMAL;
 
-- 
2.25.1

[PATCH 4/4] f2fs: merge lz4hc_compress_pages() to lz4_compress_pages()

[Yangtao Li]

Remove unnecessary lz4hc_compress_pages().

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 fs/f2fs/compress.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index b196b881f3bb..112f0e208b4e 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -264,36 +264,21 @@ static void lz4_destroy_compress_ctx(struct compress_ctx *cc)
 	cc->private = NULL;
 }
 
-#ifdef CONFIG_F2FS_FS_LZ4HC
-static int lz4hc_compress_pages(struct compress_ctx *cc)
+static int lz4_compress_pages(struct compress_ctx *cc)
 {
+	int len;
+#ifdef CONFIG_F2FS_FS_LZ4HC
 	unsigned char level = F2FS_I(cc->inode)->i_compress_flag >>
 						COMPRESS_LEVEL_OFFSET;
-	int len;
 
 	if (level)
 		len = LZ4_compress_HC(cc->rbuf, cc->cbuf->cdata, cc->rlen,
 					cc->clen, level, cc->private);
 	else
+#endif
 		len = LZ4_compress_default(cc->rbuf, cc->cbuf->cdata, cc->rlen,
 						cc->clen, cc->private);
-	if (!len)
-		return -EAGAIN;
-
-	cc->clen = len;
-	return 0;
-}
-#endif
-
-static int lz4_compress_pages(struct compress_ctx *cc)
-{
-	int len;
 
-#ifdef CONFIG_F2FS_FS_LZ4HC
-	return lz4hc_compress_pages(cc);
-#endif
-	len = LZ4_compress_default(cc->rbuf, cc->cbuf->cdata, cc->rlen,
-						cc->clen, cc->private);
 	if (!len)
 		return -EAGAIN;
 
-- 
2.25.1

Re: [PATCH 1/4] f2fs: fix to check lz4hc compression when CONFIG_F2FS_FS_LZ4HC is not enabled

[Chao Yu]

On 2023/1/24 23:33, Yangtao Li wrote:
> f2fs supports lz4 compression algorithm and lz4hc compression algorithm,
> which the level parameter needs to be passed in. When CONFIG_F2FS_FS_LZ4HC
> is not enabled, even if there is no problem with the level parameter, add
> the level parameter to the lz4 algorithm will cause the mount to fail.
> 
> Let's change it to be the same as other compression algorithms. When the
> kernel does not enable the algorithm, ignore this parameter and print msg.
> 
> Fixes: 3fde13f817e2 ("f2fs: compress: support compress level")
> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>   fs/f2fs/super.c | 31 ++++++++++++++-----------------
>   1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index d8a65645ee48..ad5df4d5c39a 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -588,19 +588,11 @@ static int f2fs_test_compress_extension(struct f2fs_sb_info *sbi)
>   	return 0;
>   }
>   
> -#ifdef CONFIG_F2FS_FS_LZ4
> +#ifdef CONFIG_F2FS_FS_LZ4HC
>   static int f2fs_set_lz4hc_level(struct f2fs_sb_info *sbi, const char *str)
>   {
> -#ifdef CONFIG_F2FS_FS_LZ4HC
>   	unsigned int level;
> -#endif
>   
> -	if (strlen(str) == 3) {
> -		F2FS_OPTION(sbi).compress_level = 0;
> -		return 0;
> -	}
> -
> -#ifdef CONFIG_F2FS_FS_LZ4HC
>   	str += 3;
>   
>   	if (str[0] != ':') {
> @@ -617,10 +609,6 @@ static int f2fs_set_lz4hc_level(struct f2fs_sb_info *sbi, const char *str)
>   
>   	F2FS_OPTION(sbi).compress_level = level;
>   	return 0;
> -#else
> -	f2fs_info(sbi, "kernel doesn't support lz4hc compression");
> -	return -EINVAL;
> -#endif
>   }
>   #endif
>   
> @@ -1085,10 +1073,19 @@ static int parse_options(struct super_block *sb, char *options, bool is_remount)
>   #endif
>   			} else if (!strncmp(name, "lz4", 3)) {
>   #ifdef CONFIG_F2FS_FS_LZ4
> -				ret = f2fs_set_lz4hc_level(sbi, name);
> -				if (ret) {
> -					kfree(name);
> -					return -EINVAL;
> +				if (strlen(name) == 3) {
> +					F2FS_OPTION(sbi).compress_level = 0;
> +				} else {
> +#ifdef CONFIG_F2FS_FS_LZ4HC
> +					ret = f2fs_set_lz4hc_level(sbi, name);
> +					if (ret) {
> +						kfree(name);
> +						return -EINVAL;
> +					}
> +#else
> +					f2fs_info(sbi, "kernel doesn't support lz4hc compression");

It needs to check <alg_name>:<compr_level> format to make sure user wants to
enable lz4hc w/ specified level, otherwise if parameter is lz4xx, it doesn't
make sense to print:

"kernel doesn't support lz4hc compression"

> +					break;

It will cause memory leak for name.

Thanks,

> +#endif
>   				}
>   				F2FS_OPTION(sbi).compress_algorithm =
>   								COMPRESS_LZ4;

Re: [PATCH 1/4] f2fs: fix to check lz4hc compression when CONFIG_F2FS_FS_LZ4HC is not enabled

[Eric Biggers]

On Sun, Jan 29, 2023 at 06:21:17PM +0800, Chao Yu wrote:
> On 2023/1/24 23:33, Yangtao Li wrote:
> > f2fs supports lz4 compression algorithm and lz4hc compression algorithm,
> > which the level parameter needs to be passed in. When CONFIG_F2FS_FS_LZ4HC
> > is not enabled, even if there is no problem with the level parameter, add

lz4hc is not a different compression algorithm, but rather just a higher
compression level for lz4.

- Eric

Re: [f2fs-dev] [PATCH 1/4] f2fs: fix to check lz4hc compression when CONFIG_F2FS_FS_LZ4HC is not enabled

[patchwork-bot+f2fs]

Hello:

This series was applied to jaegeuk/f2fs.git (dev)
by Jaegeuk Kim <jaegeuk@kernel.org>:

On Tue, 24 Jan 2023 23:33:43 +0800 you wrote:
> f2fs supports lz4 compression algorithm and lz4hc compression algorithm,
> which the level parameter needs to be passed in. When CONFIG_F2FS_FS_LZ4HC
> is not enabled, even if there is no problem with the level parameter, add
> the level parameter to the lz4 algorithm will cause the mount to fail.
> 
> Let's change it to be the same as other compression algorithms. When the
> kernel does not enable the algorithm, ignore this parameter and print msg.
> 
> [...]

Here is the summary with links:
  - [f2fs-dev,1/4] f2fs: fix to check lz4hc compression when CONFIG_F2FS_FS_LZ4HC is not enabled
    (no matching commit)
  - [f2fs-dev,2/4] f2fs: introduce f2fs_set_compress_level()
    (no matching commit)
  - [f2fs-dev,3/4] f2fs: set default compress option only when sb_has_compression
    https://git.kernel.org/jaegeuk/f2fs/c/338abb312bb2
  - [f2fs-dev,4/4] f2fs: merge lz4hc_compress_pages() to lz4_compress_pages()
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html