diff mbox series

[09/11] btrfs: change set_level() to bound the level passed in

Message ID 20190128212437.11597-10-dennis@kernel.org (mailing list archive)
State New, archived
Headers show
Series btrfs: add zstd compression level support | expand

Commit Message

Dennis Zhou Jan. 28, 2019, 9:24 p.m. UTC 
Currently, the only user of set_level() is zlib which sets an internal
workspace parameter. As level is now plumbed into get_workspace(), this
can be handled there rather than separately.

This repurposes set_level() to bound the level passed in so it can be
used when setting the mounts compression level and as well as verifying
the level before getting a workspace. The other benefit is this divides
the meaning of compress(0) and get_workspace(0). The former means we
want to use the default compression level of the compression type. The
latter means we can use any workspace available.

Signed-off-by: Dennis Zhou <dennis@kernel.org>
---
 fs/btrfs/compression.c | 23 +++++++++++++++--------
 fs/btrfs/compression.h |  4 ++--
 fs/btrfs/lzo.c         |  3 ++-
 fs/btrfs/super.c       |  4 +++-
 fs/btrfs/zlib.c        | 18 +++++++++++-------
 fs/btrfs/zstd.c        |  3 ++-
 6 files changed, 35 insertions(+), 20 deletions(-)

Comments

Nikolay Borisov Jan. 29, 2019, 8:14 a.m. UTC | #1 
On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> Currently, the only user of set_level() is zlib which sets an internal
> workspace parameter. As level is now plumbed into get_workspace(), this
> can be handled there rather than separately.
> 
> This repurposes set_level() to bound the level passed in so it can be
> used when setting the mounts compression level and as well as verifying
> the level before getting a workspace. The other benefit is this divides
> the meaning of compress(0) and get_workspace(0). The former means we
> want to use the default compression level of the compression type. The
> latter means we can use any workspace available.
> 
> Signed-off-by: Dennis Zhou <dennis@kernel.org>
> ---
>  fs/btrfs/compression.c | 23 +++++++++++++++--------
>  fs/btrfs/compression.h |  4 ++--
>  fs/btrfs/lzo.c         |  3 ++-
>  fs/btrfs/super.c       |  4 +++-
>  fs/btrfs/zlib.c        | 18 +++++++++++-------
>  fs/btrfs/zstd.c        |  3 ++-
>  6 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index e509071eaa69..a552c6f61e6d 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -1008,9 +1008,9 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
>  	struct list_head *workspace;
>  	int ret;
>  
> -	workspace = get_workspace(type, level);
> +	level = btrfs_compress_op[type]->set_level(level);
>  
> -	btrfs_compress_op[type]->set_level(workspace, level);
> +	workspace = get_workspace(type, level);
>  	ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
>  						      start, pages,
>  						      out_pages,
> @@ -1563,14 +1563,21 @@ int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end)
>  	return ret;
>  }
>  
> -unsigned int btrfs_compress_str2level(const char *str)
> +unsigned int btrfs_compress_str2level(unsigned int type, const char *str)
>  {
> -	if (strncmp(str, "zlib", 4) != 0)
> +	unsigned int level;
> +	int ret;
> +
> +	if (!type)
>  		return 0;
>  
> -	/* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */
> -	if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0)
> -		return str[5] - '0';
> +	if (str[0] == ':') {
> +		ret = kstrtouint(str + 1, 10, &level);
> +		if (ret)
> +			level = 0;
> +	}
> +
> +	level = btrfs_compress_op[type]->set_level(level);
>  
> -	return BTRFS_ZLIB_DEFAULT_LEVEL;
> +	return level;
>  }
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index e3627139bc5c..d607be40aa0e 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -90,7 +90,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
>  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  				 int mirror_num, unsigned long bio_flags);
>  
> -unsigned btrfs_compress_str2level(const char *str);
> +unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
>  
>  enum btrfs_compression_type {
>  	BTRFS_COMPRESS_NONE  = 0,
> @@ -149,7 +149,7 @@ struct btrfs_compress_op {
>  			  unsigned long start_byte,
>  			  size_t srclen, size_t destlen);
>  
> -	void (*set_level)(struct list_head *ws, unsigned int type);
> +	unsigned int (*set_level)(unsigned int level);

It might be good to document the return value since this is an
interface. AFAICS implementations are required to return the actual
level set irrespective of what level was passed in, no?

>  };
>  
>  extern const struct btrfs_compress_op btrfs_heuristic_compress;
> diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
> index f132af45a924..579d53ae256f 100644
> --- a/fs/btrfs/lzo.c
> +++ b/fs/btrfs/lzo.c
> @@ -507,8 +507,9 @@ static int lzo_decompress(struct list_head *ws, unsigned char *data_in,
>  	return ret;
>  }
>  
> -static void lzo_set_level(struct list_head *ws, unsigned int type)
> +static unsigned int lzo_set_level(unsigned int level)
>  {
> +	return 0;
>  }
>  
>  const struct btrfs_compress_op btrfs_lzo_compress = {
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index c5586ffd1426..b28dff207383 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -529,7 +529,9 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>  				if (token != Opt_compress &&
>  				    token != Opt_compress_force)
>  					info->compress_level =
> -					  btrfs_compress_str2level(args[0].from);
> +					  btrfs_compress_str2level(
> +							BTRFS_COMPRESS_ZLIB,
> +							args[0].from + 4);
>  				btrfs_set_opt(info->mount_opt, COMPRESS);
>  				btrfs_clear_opt(info->mount_opt, NODATACOW);
>  				btrfs_clear_opt(info->mount_opt, NODATASUM);
> diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
> index e2173d0c4fd3..388b1f000fca 100644
> --- a/fs/btrfs/zlib.c
> +++ b/fs/btrfs/zlib.c
> @@ -41,7 +41,12 @@ static void zlib_cleanup_workspace_manager(void)
>  
>  static struct list_head *zlib_get_workspace(unsigned int level)
>  {
> -	return btrfs_get_workspace(&wsm, level);
> +	struct list_head *ws = btrfs_get_workspace(&wsm, level);
> +	struct workspace *workspace = list_entry(ws, struct workspace, list);
> +
> +	workspace->level = level;
> +
> +	return ws;
>  }
>  
>  static void zlib_put_workspace(struct list_head *ws)
> @@ -413,15 +418,14 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
>  	return ret;
>  }
>  
> -static void zlib_set_level(struct list_head *ws, unsigned int type)
> +static unsigned int zlib_set_level(unsigned int level)
>  {
> -	struct workspace *workspace = list_entry(ws, struct workspace, list);
> -	unsigned int level = BTRFS_COMPRESS_LEVEL(type);
> -
> -	if (level > 9)
> +	if (!level)
> +		level = BTRFS_ZLIB_DEFAULT_LEVEL;
> +	else if (level > 9)
>  		level = 9;

nit:  This makes it a bit more obvious (IMO) that you are essentially
doing max:

if (!level)
   level = BTRFS_ZLIB_DEFAULT_LEVEL;
level = max(level, 9);

>  
> -	workspace->level = level > 0 ? level : 3;
> +	return level;
>  }
>  
>  const struct btrfs_compress_op btrfs_zlib_compress = {
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index 404101864220..43f3be755b8c 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -441,8 +441,9 @@ static int zstd_decompress(struct list_head *ws, unsigned char *data_in,
>  	return ret;
>  }
>  
> -static void zstd_set_level(struct list_head *ws, unsigned int type)
> +static unsigned int zstd_set_level(unsigned int level)
>  {
> +	return ZSTD_BTRFS_DEFAULT_LEVEL;
>  }
>  
>  const struct btrfs_compress_op btrfs_zstd_compress = {
>
Dennis Zhou Jan. 30, 2019, 10:06 p.m. UTC | #2 
On Tue, Jan 29, 2019 at 10:14:18AM +0200, Nikolay Borisov wrote:
> 
> 
> On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> > Currently, the only user of set_level() is zlib which sets an internal
> > workspace parameter. As level is now plumbed into get_workspace(), this
> > can be handled there rather than separately.
> > 
> > This repurposes set_level() to bound the level passed in so it can be
> > used when setting the mounts compression level and as well as verifying
> > the level before getting a workspace. The other benefit is this divides
> > the meaning of compress(0) and get_workspace(0). The former means we
> > want to use the default compression level of the compression type. The
> > latter means we can use any workspace available.
> > 
> > Signed-off-by: Dennis Zhou <dennis@kernel.org>
> > ---
 >  }
> > diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> > index e3627139bc5c..d607be40aa0e 100644
> > --- a/fs/btrfs/compression.h
> > +++ b/fs/btrfs/compression.h
> > @@ -90,7 +90,7 @@ blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
> >  blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
> >  				 int mirror_num, unsigned long bio_flags);
> >  
> > -unsigned btrfs_compress_str2level(const char *str);
> > +unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
> >  
> >  enum btrfs_compression_type {
> >  	BTRFS_COMPRESS_NONE  = 0,
> > @@ -149,7 +149,7 @@ struct btrfs_compress_op {
> >  			  unsigned long start_byte,
> >  			  size_t srclen, size_t destlen);
> >  
> > -	void (*set_level)(struct list_head *ws, unsigned int type);
> > +	unsigned int (*set_level)(unsigned int level);
> 
> It might be good to document the return value since this is an
> interface. AFAICS implementations are required to return the actual
> level set irrespective of what level was passed in, no?
> 
> >  };

Yeah that makes sense. I've added a comment about it in
btrfs_compress_op.

> >  
> >  static void zlib_put_workspace(struct list_head *ws)
> > @@ -413,15 +418,14 @@ static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
> >  	return ret;
> >  }
> >  
> > -static void zlib_set_level(struct list_head *ws, unsigned int type)
> > +static unsigned int zlib_set_level(unsigned int level)
> >  {
> > -	struct workspace *workspace = list_entry(ws, struct workspace, list);
> > -	unsigned int level = BTRFS_COMPRESS_LEVEL(type);
> > -
> > -	if (level > 9)
> > +	if (!level)
> > +		level = BTRFS_ZLIB_DEFAULT_LEVEL;
> > +	else if (level > 9)
> >  		level = 9;
> 
> nit:  This makes it a bit more obvious (IMO) that you are essentially
> doing max:
> 
> if (!level)
>    level = BTRFS_ZLIB_DEFAULT_LEVEL;
> level = max(level, 9);
> 

I agree. I've updated it in both places. It should be min instead of max
though.

Thanks,
Dennis
diff mbox series

Patch

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index e509071eaa69..a552c6f61e6d 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -1008,9 +1008,9 @@  int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
 	struct list_head *workspace;
 	int ret;
 
-	workspace = get_workspace(type, level);
+	level = btrfs_compress_op[type]->set_level(level);
 
-	btrfs_compress_op[type]->set_level(workspace, level);
+	workspace = get_workspace(type, level);
 	ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
 						      start, pages,
 						      out_pages,
@@ -1563,14 +1563,21 @@  int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end)
 	return ret;
 }
 
-unsigned int btrfs_compress_str2level(const char *str)
+unsigned int btrfs_compress_str2level(unsigned int type, const char *str)
 {
-	if (strncmp(str, "zlib", 4) != 0)
+	unsigned int level;
+	int ret;
+
+	if (!type)
 		return 0;
 
-	/* Accepted form: zlib:1 up to zlib:9 and nothing left after the number */
-	if (str[4] == ':' && '1' <= str[5] && str[5] <= '9' && str[6] == 0)
-		return str[5] - '0';
+	if (str[0] == ':') {
+		ret = kstrtouint(str + 1, 10, &level);
+		if (ret)
+			level = 0;
+	}
+
+	level = btrfs_compress_op[type]->set_level(level);
 
-	return BTRFS_ZLIB_DEFAULT_LEVEL;
+	return level;
 }
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index e3627139bc5c..d607be40aa0e 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -90,7 +90,7 @@  blk_status_t btrfs_submit_compressed_write(struct inode *inode, u64 start,
 blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 				 int mirror_num, unsigned long bio_flags);
 
-unsigned btrfs_compress_str2level(const char *str);
+unsigned int btrfs_compress_str2level(unsigned int type, const char *str);
 
 enum btrfs_compression_type {
 	BTRFS_COMPRESS_NONE  = 0,
@@ -149,7 +149,7 @@  struct btrfs_compress_op {
 			  unsigned long start_byte,
 			  size_t srclen, size_t destlen);
 
-	void (*set_level)(struct list_head *ws, unsigned int type);
+	unsigned int (*set_level)(unsigned int level);
 };
 
 extern const struct btrfs_compress_op btrfs_heuristic_compress;
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index f132af45a924..579d53ae256f 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -507,8 +507,9 @@  static int lzo_decompress(struct list_head *ws, unsigned char *data_in,
 	return ret;
 }
 
-static void lzo_set_level(struct list_head *ws, unsigned int type)
+static unsigned int lzo_set_level(unsigned int level)
 {
+	return 0;
 }
 
 const struct btrfs_compress_op btrfs_lzo_compress = {
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index c5586ffd1426..b28dff207383 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -529,7 +529,9 @@  int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 				if (token != Opt_compress &&
 				    token != Opt_compress_force)
 					info->compress_level =
-					  btrfs_compress_str2level(args[0].from);
+					  btrfs_compress_str2level(
+							BTRFS_COMPRESS_ZLIB,
+							args[0].from + 4);
 				btrfs_set_opt(info->mount_opt, COMPRESS);
 				btrfs_clear_opt(info->mount_opt, NODATACOW);
 				btrfs_clear_opt(info->mount_opt, NODATASUM);
diff --git a/fs/btrfs/zlib.c b/fs/btrfs/zlib.c
index e2173d0c4fd3..388b1f000fca 100644
--- a/fs/btrfs/zlib.c
+++ b/fs/btrfs/zlib.c
@@ -41,7 +41,12 @@  static void zlib_cleanup_workspace_manager(void)
 
 static struct list_head *zlib_get_workspace(unsigned int level)
 {
-	return btrfs_get_workspace(&wsm, level);
+	struct list_head *ws = btrfs_get_workspace(&wsm, level);
+	struct workspace *workspace = list_entry(ws, struct workspace, list);
+
+	workspace->level = level;
+
+	return ws;
 }
 
 static void zlib_put_workspace(struct list_head *ws)
@@ -413,15 +418,14 @@  static int zlib_decompress(struct list_head *ws, unsigned char *data_in,
 	return ret;
 }
 
-static void zlib_set_level(struct list_head *ws, unsigned int type)
+static unsigned int zlib_set_level(unsigned int level)
 {
-	struct workspace *workspace = list_entry(ws, struct workspace, list);
-	unsigned int level = BTRFS_COMPRESS_LEVEL(type);
-
-	if (level > 9)
+	if (!level)
+		level = BTRFS_ZLIB_DEFAULT_LEVEL;
+	else if (level > 9)
 		level = 9;
 
-	workspace->level = level > 0 ? level : 3;
+	return level;
 }
 
 const struct btrfs_compress_op btrfs_zlib_compress = {
diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index 404101864220..43f3be755b8c 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -441,8 +441,9 @@  static int zstd_decompress(struct list_head *ws, unsigned char *data_in,
 	return ret;
 }
 
-static void zstd_set_level(struct list_head *ws, unsigned int type)
+static unsigned int zstd_set_level(unsigned int level)
 {
+	return ZSTD_BTRFS_DEFAULT_LEVEL;
 }
 
 const struct btrfs_compress_op btrfs_zstd_compress = {