[PATCH 0/2] Compression level API cleanups

[PATCH 0/2] Compression level API cleanups

[David Sterba]

Remove the callback indirection.

David Sterba (2):
  btrfs: define compression levels statically
  btrfs: compression: replace set_level callbacks by a common helper

 fs/btrfs/compression.c | 20 ++++++++++++++++++--
 fs/btrfs/compression.h | 11 +++++------
 fs/btrfs/lzo.c         |  8 ++------
 fs/btrfs/zlib.c        | 11 ++---------
 fs/btrfs/zstd.c        | 11 ++---------
 5 files changed, 29 insertions(+), 32 deletions(-)

-- 
2.22.0

[PATCH 1/2] btrfs: define compression levels statically

[David Sterba]

The maximum and default levels do not change and can be defined
directly. The set_level callback was a temporary solution and will be
removed.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/compression.h | 4 ++++
 fs/btrfs/lzo.c         | 2 ++
 fs/btrfs/zlib.c        | 2 ++
 fs/btrfs/zstd.c        | 2 ++
 4 files changed, 10 insertions(+)

diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 2035b8eb1290..07b2009dc63f 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -162,6 +162,10 @@ struct btrfs_compress_op {
 	 * if the level is out of bounds or the default if 0 is passed in.
 	 */
 	unsigned int (*set_level)(unsigned int level);
+
+	/* Maximum level supported by the compression algorithm */
+	int max_level;
+	int default_level;
 };
 
 /* The heuristic workspaces are managed via the 0th workspace manager */
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 579d53ae256f..adac6cb30d65 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -523,4 +523,6 @@ const struct btrfs_compress_op btrfs_lzo_compress = {
 	.decompress_bio		= lzo_decompress_bio,
 	.decompress		= lzo_decompress,
 	.set_level		= lzo_set_level,
+	.max_level		= 1,
+	.default_level		= 1,
 };
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index b86b7ad6b900..03d6c3683bd9 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -437,4 +437,6 @@ const struct btrfs_compress_op btrfs_zlib_compress = {
 	.decompress_bio		= zlib_decompress_bio,
 	.decompress		= zlib_decompress,
 	.set_level              = zlib_set_level,
+	.max_level		= 9,
+	.default_level		= BTRFS_ZLIB_DEFAULT_LEVEL,
 };
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index 3837ca180d52..b2b23a6a497d 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -729,4 +729,6 @@ const struct btrfs_compress_op btrfs_zstd_compress = {
 	.decompress_bio = zstd_decompress_bio,
 	.decompress = zstd_decompress,
 	.set_level = zstd_set_level,
+	.max_level	= ZSTD_BTRFS_MAX_LEVEL,
+	.default_level	= ZSTD_BTRFS_DEFAULT_LEVEL,
 };
-- 
2.22.0

[PATCH 2/2] btrfs: compression: replace set_level callbacks by a common helper

[David Sterba]

The set_level callbacks do not do anything special and can be replaced
by a helper that uses the levels defined in the tables.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/compression.c | 20 ++++++++++++++++++--
 fs/btrfs/compression.h |  9 ++-------
 fs/btrfs/lzo.c         |  6 ------
 fs/btrfs/zlib.c        |  9 ---------
 fs/btrfs/zstd.c        |  9 ---------
 5 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 60c47b417a4b..9d08d56f2896 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1039,7 +1039,7 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
 	struct list_head *workspace;
 	int ret;
 
-	level = btrfs_compress_op[type]->set_level(level);
+	level = btrfs_compress_set_level(type, level);
 	workspace = get_workspace(type, level);
 	ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
 						      start, pages,
@@ -1611,7 +1611,23 @@ unsigned int btrfs_compress_str2level(unsigned int type, const char *str)
 			level = 0;
 	}
 
-	level = btrfs_compress_op[type]->set_level(level);
+	level = btrfs_compress_set_level(type, level);
+
+	return level;
+}
+
+/*
+ * Adjust @level according to the limits of the compression algorithm or
+ * fallback to default
+ */
+unsigned int btrfs_compress_get_level(int type, unsigned level)
+{
+	const struct btrfs_compress_op *ops = btrfs_compress_op[type];
+
+	if (level == 0)
+		level = ops->default_level;
+	else
+		level = min_t(unsigned int, level, ops->max_level);
 
 	return level;
 }
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 07b2009dc63f..f0fc6304dfae 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -156,13 +156,6 @@ struct btrfs_compress_op {
 			  unsigned long start_byte,
 			  size_t srclen, size_t destlen);
 
-	/*
-	 * This bounds the level set by the user to be within range of a
-	 * particular compression type.  It returns the level that will be used
-	 * if the level is out of bounds or the default if 0 is passed in.
-	 */
-	unsigned int (*set_level)(unsigned int level);
-
 	/* Maximum level supported by the compression algorithm */
 	int max_level;
 	int default_level;
@@ -179,6 +172,8 @@ extern const struct btrfs_compress_op btrfs_zstd_compress;
 const char* btrfs_compress_type2str(enum btrfs_compression_type type);
 bool btrfs_compress_is_valid_type(const char *str, size_t len);
 
+unsigned int btrfs_compress_set_level(int type, unsigned level);
+
 int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end);
 
 #endif
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index adac6cb30d65..acad4174f68d 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -507,11 +507,6 @@ static int lzo_decompress(struct list_head *ws, unsigned char *data_in,
 	return ret;
 }
 
-static unsigned int lzo_set_level(unsigned int level)
-{
-	return 0;
-}
-
 const struct btrfs_compress_op btrfs_lzo_compress = {
 	.init_workspace_manager	= lzo_init_workspace_manager,
 	.cleanup_workspace_manager = lzo_cleanup_workspace_manager,
@@ -522,7 +517,6 @@ const struct btrfs_compress_op btrfs_lzo_compress = {
 	.compress_pages		= lzo_compress_pages,
 	.decompress_bio		= lzo_decompress_bio,
 	.decompress		= lzo_decompress,
-	.set_level		= lzo_set_level,
 	.max_level		= 1,
 	.default_level		= 1,
 };
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index 03d6c3683bd9..df1aace5df50 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -418,14 +418,6 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 	return ret;
 }
 
-static unsigned int zlib_set_level(unsigned int level)
-{
-	if (!level)
-		return BTRFS_ZLIB_DEFAULT_LEVEL;
-
-	return min_t(unsigned int, level, 9);
-}
-
 const struct btrfs_compress_op btrfs_zlib_compress = {
 	.init_workspace_manager	= zlib_init_workspace_manager,
 	.cleanup_workspace_manager = zlib_cleanup_workspace_manager,
@@ -436,7 +428,6 @@ const struct btrfs_compress_op btrfs_zlib_compress = {
 	.compress_pages		= zlib_compress_pages,
 	.decompress_bio		= zlib_decompress_bio,
 	.decompress		= zlib_decompress,
-	.set_level              = zlib_set_level,
 	.max_level		= 9,
 	.default_level		= BTRFS_ZLIB_DEFAULT_LEVEL,
 };
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index b2b23a6a497d..0af4a5cd4313 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -710,14 +710,6 @@ static int zstd_decompress(struct list_head *ws, unsigned char *data_in,
 	return ret;
 }
 
-static unsigned int zstd_set_level(unsigned int level)
-{
-	if (!level)
-		return ZSTD_BTRFS_DEFAULT_LEVEL;
-
-	return min_t(unsigned int, level, ZSTD_BTRFS_MAX_LEVEL);
-}
-
 const struct btrfs_compress_op btrfs_zstd_compress = {
 	.init_workspace_manager = zstd_init_workspace_manager,
 	.cleanup_workspace_manager = zstd_cleanup_workspace_manager,
@@ -728,7 +720,6 @@ const struct btrfs_compress_op btrfs_zstd_compress = {
 	.compress_pages = zstd_compress_pages,
 	.decompress_bio = zstd_decompress_bio,
 	.decompress = zstd_decompress,
-	.set_level = zstd_set_level,
 	.max_level	= ZSTD_BTRFS_MAX_LEVEL,
 	.default_level	= ZSTD_BTRFS_DEFAULT_LEVEL,
 };
-- 
2.22.0

Re: [PATCH 2/2] btrfs: compression: replace set_level callbacks by a common helper

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/Compression-level-API-cleanups/20190811-193645
config: x86_64-randconfig-h003-201932 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> ERROR: "btrfs_compress_set_level" [fs/btrfs/btrfs.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36610 bytes --]

Re: [PATCH 2/2] btrfs: compression: replace set_level callbacks by a common helper

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3027 bytes --]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3-rc3 next-20190809]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/Compression-level-API-cleanups/20190811-193645
config: x86_64-randconfig-h002-201932 (attached as .config)
compiler: gcc-7 (Debian 7.4.0-10) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: fs/btrfs/compression.o: in function `btrfs_compress_pages':
>> fs/btrfs/compression.c:1042: undefined reference to `btrfs_compress_set_level'
   ld: fs/btrfs/compression.o: in function `btrfs_compress_str2level':
   fs/btrfs/compression.c:1614: undefined reference to `btrfs_compress_set_level'

vim +1042 fs/btrfs/compression.c

  1007	
  1008	/*
  1009	 * Given an address space and start and length, compress the bytes into @pages
  1010	 * that are allocated on demand.
  1011	 *
  1012	 * @type_level is encoded algorithm and level, where level 0 means whatever
  1013	 * default the algorithm chooses and is opaque here;
  1014	 * - compression algo are 0-3
  1015	 * - the level are bits 4-7
  1016	 *
  1017	 * @out_pages is an in/out parameter, holds maximum number of pages to allocate
  1018	 * and returns number of actually allocated pages
  1019	 *
  1020	 * @total_in is used to return the number of bytes actually read.  It
  1021	 * may be smaller than the input length if we had to exit early because we
  1022	 * ran out of room in the pages array or because we cross the
  1023	 * max_out threshold.
  1024	 *
  1025	 * @total_out is an in/out parameter, must be set to the input length and will
  1026	 * be also used to return the total number of compressed bytes
  1027	 *
  1028	 * @max_out tells us the max number of bytes that we're allowed to
  1029	 * stuff into pages
  1030	 */
  1031	int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
  1032				 u64 start, struct page **pages,
  1033				 unsigned long *out_pages,
  1034				 unsigned long *total_in,
  1035				 unsigned long *total_out)
  1036	{
  1037		int type = btrfs_compress_type(type_level);
  1038		int level = btrfs_compress_level(type_level);
  1039		struct list_head *workspace;
  1040		int ret;
  1041	
> 1042		level = btrfs_compress_set_level(type, level);
  1043		workspace = get_workspace(type, level);
  1044		ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
  1045							      start, pages,
  1046							      out_pages,
  1047							      total_in, total_out);
  1048		put_workspace(type, workspace);
  1049		return ret;
  1050	}
  1051	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30830 bytes --]

Re: [PATCH 1/2] btrfs: define compression levels statically

[Nikolay Borisov]

On 9.08.19 г. 17:55 ч., David Sterba wrote:
> The maximum and default levels do not change and can be defined
> directly. The set_level callback was a temporary solution and will be
> removed.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/compression.h | 4 ++++
>  fs/btrfs/lzo.c         | 2 ++
>  fs/btrfs/zlib.c        | 2 ++
>  fs/btrfs/zstd.c        | 2 ++
>  4 files changed, 10 insertions(+)
> 
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 2035b8eb1290..07b2009dc63f 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -162,6 +162,10 @@ struct btrfs_compress_op {
>  	 * if the level is out of bounds or the default if 0 is passed in.
>  	 */
>  	unsigned int (*set_level)(unsigned int level);
> +
> +	/* Maximum level supported by the compression algorithm */
> +	int max_level;
> +	int default_level;

can levels be negative? If not just define those as unsigned ints and in
the next patch it won't be necessary to use min_t but plain min.

>  };
>  
>  /* The heuristic workspaces are managed via the 0th workspace manager */
> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
> index 579d53ae256f..adac6cb30d65 100644
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -523,4 +523,6 @@ const struct btrfs_compress_op btrfs_lzo_compress = {
>  	.decompress_bio		= lzo_decompress_bio,
>  	.decompress		= lzo_decompress,
>  	.set_level		= lzo_set_level,
> +	.max_level		= 1,
> +	.default_level		= 1,
>  };
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index b86b7ad6b900..03d6c3683bd9 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -437,4 +437,6 @@ const struct btrfs_compress_op btrfs_zlib_compress = {
>  	.decompress_bio		= zlib_decompress_bio,
>  	.decompress		= zlib_decompress,
>  	.set_level              = zlib_set_level,
> +	.max_level		= 9,
> +	.default_level		= BTRFS_ZLIB_DEFAULT_LEVEL,
>  };
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index 3837ca180d52..b2b23a6a497d 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -729,4 +729,6 @@ const struct btrfs_compress_op btrfs_zstd_compress = {
>  	.decompress_bio = zstd_decompress_bio,
>  	.decompress = zstd_decompress,
>  	.set_level = zstd_set_level,
> +	.max_level	= ZSTD_BTRFS_MAX_LEVEL,
> +	.default_level	= ZSTD_BTRFS_DEFAULT_LEVEL,
>  };
> 

Re: [PATCH 1/2] btrfs: define compression levels statically

[David Sterba]

On Mon, Aug 12, 2019 at 11:30:42AM +0300, Nikolay Borisov wrote:
> 
> 
> On 9.08.19 г. 17:55 ч., David Sterba wrote:
> > The maximum and default levels do not change and can be defined
> > directly. The set_level callback was a temporary solution and will be
> > removed.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/compression.h | 4 ++++
> >  fs/btrfs/lzo.c         | 2 ++
> >  fs/btrfs/zlib.c        | 2 ++
> >  fs/btrfs/zstd.c        | 2 ++
> >  4 files changed, 10 insertions(+)
> > 
> > diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> > index 2035b8eb1290..07b2009dc63f 100644
> > --- a/fs/btrfs/compression.h
> > +++ b/fs/btrfs/compression.h
> > @@ -162,6 +162,10 @@ struct btrfs_compress_op {
> >  	 * if the level is out of bounds or the default if 0 is passed in.
> >  	 */
> >  	unsigned int (*set_level)(unsigned int level);
> > +
> > +	/* Maximum level supported by the compression algorithm */
> > +	int max_level;
> > +	int default_level;
> 
> can levels be negative? If not just define those as unsigned ints and in
> the next patch it won't be necessary to use min_t but plain min.

No, levels should be >= 0.