Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Nikolay Borisov <nborisov@suse.com>
To: Dennis Zhou <dennis@kernel.org>, David Sterba <dsterba@suse.com>,
	Josef Bacik <josef@toxicpanda.com>, Chris Mason <clm@fb.com>,
	Omar Sandoval <osandov@osandov.com>,
	Nick Terrell <terrelln@fb.com>
Cc: kernel-team@fb.com, linux-btrfs@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 09/11] btrfs: change set_level() to bound the level passed in
Date: Tue, 29 Jan 2019 10:14:18 +0200
Message-ID: <372d47b8-457d-db5d-61f4-de7b901c5e78@suse.com> (raw)
In-Reply-To: <20190128212437.11597-10-dennis@kernel.org>



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 = {
> 

  reply index

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-28 21:24 [PATCH 00/11] btrfs: add zstd compression level support Dennis Zhou
2019-01-28 21:24 ` [PATCH 01/11] btrfs: add macros for compression type and level Dennis Zhou
2019-01-29  7:26   ` Nikolay Borisov
2019-01-29 17:57   ` Josef Bacik
2019-01-29 22:30   ` Omar Sandoval
2019-01-31 16:00   ` David Sterba
2019-01-31 16:17     ` Dennis Zhou
2019-01-28 21:24 ` [PATCH 02/11] btrfs: rename workspaces_list to workspace_manager Dennis Zhou
2019-01-29  7:27   ` Nikolay Borisov
2019-01-29 17:58   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 03/11] btrfs: manage heuristic workspace as index 0 Dennis Zhou
2019-01-29  7:53   ` Nikolay Borisov
2019-01-29 22:43     ` Dennis Zhou
2019-01-29 18:02   ` Josef Bacik
2019-01-31 16:10   ` David Sterba
2019-01-28 21:24 ` [PATCH 04/11] btrfs: unify compression ops with workspace_manager Dennis Zhou
2019-01-29  7:54   ` Nikolay Borisov
2019-01-29 18:03   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 05/11] btrfs: add helper methods for workspace manager init and cleanup Dennis Zhou
2019-01-29  7:58   ` Nikolay Borisov
2019-01-29 18:04   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 06/11] btrfs: add compression interface in (get/put)_workspace() Dennis Zhou
2019-01-29 18:06   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 07/11] btrfs: move to fn pointers for get/put workspaces Dennis Zhou
2019-01-29  8:22   ` Nikolay Borisov
2019-01-29 23:35     ` Dennis Zhou
2019-01-29 18:17   ` Josef Bacik
2019-01-29 18:44     ` Josef Bacik
2019-01-28 21:24 ` [PATCH 08/11] btrfs: plumb level through the compression interface Dennis Zhou
2019-01-29  8:08   ` Nikolay Borisov
2019-01-29 18:17   ` Josef Bacik
2019-01-28 21:24 ` [PATCH 09/11] btrfs: change set_level() to bound the level passed in Dennis Zhou
2019-01-29  8:14   ` Nikolay Borisov [this message]
2019-01-30 22:06     ` Dennis Zhou
2019-01-28 21:24 ` [PATCH 10/11] btrfs: zstd use the passed through level instead of default Dennis Zhou
2019-01-29  8:15   ` Nikolay Borisov
2019-01-28 21:24 ` [PATCH 11/11] btrfs: add zstd compression level support Dennis Zhou
2019-01-29  7:25   ` Nikolay Borisov
2019-01-29 22:50     ` Dennis Zhou
2019-01-31 18:10   ` David Sterba
2019-01-31 18:13   ` David Sterba
2019-01-29 17:18 ` [PATCH 00/11] " David Sterba
2019-01-29 21:12   ` Nick Terrell
2019-01-30 17:40   ` Dennis Zhou
2019-01-31 14:04     ` David Sterba
2019-01-31 15:56       ` Dennis Zhou

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=372d47b8-457d-db5d-61f4-de7b901c5e78@suse.com \
    --to=nborisov@suse.com \
    --cc=clm@fb.com \
    --cc=dennis@kernel.org \
    --cc=dsterba@suse.com \
    --cc=josef@toxicpanda.com \
    --cc=kernel-team@fb.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=osandov@osandov.com \
    --cc=terrelln@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox