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 03/11] btrfs: manage heuristic workspace as index 0
Date: Tue, 29 Jan 2019 09:53:33 +0200 [thread overview]
Message-ID: <4bc26773-da13-3730-afda-78d672f348bc@suse.com> (raw)
In-Reply-To: <20190128212437.11597-4-dennis@kernel.org>
On 28.01.19 г. 23:24 ч., Dennis Zhou wrote:
> While the heuristic workspaces aren't really compression workspaces,
> they use the same interface for managing them. So rather than branching,
> let's just handle them once again as the index 0 compression type.
>
> Signed-off-by: Dennis Zhou <dennis@kernel.org>
Reviewed-by: Nikolay Borisov <nborisov@suse.com> albeit one minor nit
below.
> ---
> fs/btrfs/compression.c | 107 +++++++++++-----------------------------
> fs/btrfs/compression.h | 3 +-
> fs/btrfs/ioctl.c | 2 +-
> fs/btrfs/tree-checker.c | 4 +-
> 4 files changed, 33 insertions(+), 83 deletions(-)
>
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index aced261984e2..bda7e8d2cbc7 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -37,6 +37,8 @@ const char* btrfs_compress_type2str(enum btrfs_compression_type type)
> case BTRFS_COMPRESS_ZSTD:
> case BTRFS_COMPRESS_NONE:
> return btrfs_compress_types[type];
> + default:
> + return NULL;
nit: With this change...
> }
>
> return NULL;
This becomes redundant. I doubt the compiler will issue a warning since
it should be clever enough to figure we will never exit the switch()
construct.
> @@ -769,6 +771,11 @@ static struct list_head *alloc_heuristic_ws(void)
> return ERR_PTR(-ENOMEM);
> }
>
> +const struct btrfs_compress_op btrfs_heuristic_compress = {
> + .alloc_workspace = alloc_heuristic_ws,
> + .free_workspace = free_heuristic_ws,
> +};
> +
> struct workspace_manager {
> struct list_head idle_ws;
> spinlock_t ws_lock;
> @@ -782,9 +789,8 @@ struct workspace_manager {
>
> static struct workspace_manager wsm[BTRFS_COMPRESS_TYPES];
>
> -static struct workspace_manager btrfs_heuristic_ws;
> -
> static const struct btrfs_compress_op * const btrfs_compress_op[] = {
> + &btrfs_heuristic_compress,
> &btrfs_zlib_compress,
> &btrfs_lzo_compress,
> &btrfs_zstd_compress,
> @@ -795,21 +801,6 @@ void __init btrfs_init_compress(void)
> struct list_head *workspace;
> int i;
>
> - INIT_LIST_HEAD(&btrfs_heuristic_ws.idle_ws);
> - spin_lock_init(&btrfs_heuristic_ws.ws_lock);
> - atomic_set(&btrfs_heuristic_ws.total_ws, 0);
> - init_waitqueue_head(&btrfs_heuristic_ws.ws_wait);
> -
> - workspace = alloc_heuristic_ws();
> - if (IS_ERR(workspace)) {
> - pr_warn(
> - "BTRFS: cannot preallocate heuristic workspace, will try later\n");
> - } else {
> - atomic_set(&btrfs_heuristic_ws.total_ws, 1);
> - btrfs_heuristic_ws.free_ws = 1;
> - list_add(workspace, &btrfs_heuristic_ws.idle_ws);
> - }
> -
> for (i = 0; i < BTRFS_COMPRESS_TYPES; i++) {
> INIT_LIST_HEAD(&wsm[i].idle_ws);
> spin_lock_init(&wsm[i].ws_lock);
> @@ -837,11 +828,10 @@ void __init btrfs_init_compress(void)
> * Preallocation makes a forward progress guarantees and we do not return
> * errors.
> */
> -static struct list_head *__find_workspace(int type, bool heuristic)
> +static struct list_head *find_workspace(int type)
> {
> struct list_head *workspace;
> int cpus = num_online_cpus();
> - int idx = type - 1;
> unsigned nofs_flag;
> struct list_head *idle_ws;
> spinlock_t *ws_lock;
> @@ -849,19 +839,11 @@ static struct list_head *__find_workspace(int type, bool heuristic)
> wait_queue_head_t *ws_wait;
> int *free_ws;
>
> - if (heuristic) {
> - idle_ws = &btrfs_heuristic_ws.idle_ws;
> - ws_lock = &btrfs_heuristic_ws.ws_lock;
> - total_ws = &btrfs_heuristic_ws.total_ws;
> - ws_wait = &btrfs_heuristic_ws.ws_wait;
> - free_ws = &btrfs_heuristic_ws.free_ws;
> - } else {
> - idle_ws = &wsm[idx].idle_ws;
> - ws_lock = &wsm[idx].ws_lock;
> - total_ws = &wsm[idx].total_ws;
> - ws_wait = &wsm[idx].ws_wait;
> - free_ws = &wsm[idx].free_ws;
> - }
> + idle_ws = &wsm[type].idle_ws;
> + ws_lock = &wsm[type].ws_lock;
> + total_ws = &wsm[type].total_ws;
> + ws_wait = &wsm[type].ws_wait;
> + free_ws = &wsm[type].free_ws;
>
> again:
> spin_lock(ws_lock);
> @@ -892,10 +874,7 @@ static struct list_head *__find_workspace(int type, bool heuristic)
> * context of btrfs_compress_bio/btrfs_compress_pages
> */
> nofs_flag = memalloc_nofs_save();
> - if (heuristic)
> - workspace = alloc_heuristic_ws();
> - else
> - workspace = btrfs_compress_op[idx]->alloc_workspace();
> + workspace = btrfs_compress_op[type]->alloc_workspace();
> memalloc_nofs_restore(nofs_flag);
>
> if (IS_ERR(workspace)) {
> @@ -926,38 +905,23 @@ static struct list_head *__find_workspace(int type, bool heuristic)
> return workspace;
> }
>
> -static struct list_head *find_workspace(int type)
> -{
> - return __find_workspace(type, false);
> -}
> -
> /*
> * put a workspace struct back on the list or free it if we have enough
> * idle ones sitting around
> */
> -static void __free_workspace(int type, struct list_head *workspace,
> - bool heuristic)
> +static void free_workspace(int type, struct list_head *workspace)
> {
> - int idx = type - 1;
> struct list_head *idle_ws;
> spinlock_t *ws_lock;
> atomic_t *total_ws;
> wait_queue_head_t *ws_wait;
> int *free_ws;
>
> - if (heuristic) {
> - idle_ws = &btrfs_heuristic_ws.idle_ws;
> - ws_lock = &btrfs_heuristic_ws.ws_lock;
> - total_ws = &btrfs_heuristic_ws.total_ws;
> - ws_wait = &btrfs_heuristic_ws.ws_wait;
> - free_ws = &btrfs_heuristic_ws.free_ws;
> - } else {
> - idle_ws = &wsm[idx].idle_ws;
> - ws_lock = &wsm[idx].ws_lock;
> - total_ws = &wsm[idx].total_ws;
> - ws_wait = &wsm[idx].ws_wait;
> - free_ws = &wsm[idx].free_ws;
> - }
> + idle_ws = &wsm[type].idle_ws;
> + ws_lock = &wsm[type].ws_lock;
> + total_ws = &wsm[type].total_ws;
> + ws_wait = &wsm[type].ws_wait;
> + free_ws = &wsm[type].free_ws;
>
> spin_lock(ws_lock);
> if (*free_ws <= num_online_cpus()) {
> @@ -968,20 +932,12 @@ static void __free_workspace(int type, struct list_head *workspace,
> }
> spin_unlock(ws_lock);
>
> - if (heuristic)
> - free_heuristic_ws(workspace);
> - else
> - btrfs_compress_op[idx]->free_workspace(workspace);
> + btrfs_compress_op[type]->free_workspace(workspace);
> atomic_dec(total_ws);
> wake:
> cond_wake_up(ws_wait);
> }
>
> -static void free_workspace(int type, struct list_head *ws)
> -{
> - return __free_workspace(type, ws, false);
> -}
> -
> /*
> * cleanup function for module exit
> */
> @@ -990,13 +946,6 @@ static void free_workspaces(void)
> struct list_head *workspace;
> int i;
>
> - while (!list_empty(&btrfs_heuristic_ws.idle_ws)) {
> - workspace = btrfs_heuristic_ws.idle_ws.next;
> - list_del(workspace);
> - free_heuristic_ws(workspace);
> - atomic_dec(&btrfs_heuristic_ws.total_ws);
> - }
> -
> for (i = 0; i < BTRFS_COMPRESS_TYPES; i++) {
> while (!list_empty(&wsm[i].idle_ws)) {
> workspace = wsm[i].idle_ws.next;
> @@ -1042,8 +991,8 @@ int btrfs_compress_pages(unsigned int type_level, struct address_space *mapping,
>
> workspace = find_workspace(type);
>
> - btrfs_compress_op[type - 1]->set_level(workspace, type_level);
> - ret = btrfs_compress_op[type-1]->compress_pages(workspace, mapping,
> + btrfs_compress_op[type]->set_level(workspace, type_level);
> + ret = btrfs_compress_op[type]->compress_pages(workspace, mapping,
> start, pages,
> out_pages,
> total_in, total_out);
> @@ -1072,7 +1021,7 @@ static int btrfs_decompress_bio(struct compressed_bio *cb)
> int type = cb->compress_type;
>
> workspace = find_workspace(type);
> - ret = btrfs_compress_op[type - 1]->decompress_bio(workspace, cb);
> + ret = btrfs_compress_op[type]->decompress_bio(workspace, cb);
> free_workspace(type, workspace);
>
> return ret;
> @@ -1091,7 +1040,7 @@ int btrfs_decompress(int type, unsigned char *data_in, struct page *dest_page,
>
> workspace = find_workspace(type);
>
> - ret = btrfs_compress_op[type-1]->decompress(workspace, data_in,
> + ret = btrfs_compress_op[type]->decompress(workspace, data_in,
> dest_page, start_byte,
> srclen, destlen);
>
> @@ -1512,7 +1461,7 @@ static void heuristic_collect_sample(struct inode *inode, u64 start, u64 end,
> */
> int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end)
> {
> - struct list_head *ws_list = __find_workspace(0, true);
> + struct list_head *ws_list = find_workspace(0);
> struct heuristic_ws *ws;
> u32 i;
> u8 byte;
> @@ -1581,7 +1530,7 @@ int btrfs_compress_heuristic(struct inode *inode, u64 start, u64 end)
> }
>
> out:
> - __free_workspace(0, ws_list, true);
> + free_workspace(0, ws_list);
> return ret;
> }
>
> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
> index 69a9197dadc3..53a8b9e93217 100644
> --- a/fs/btrfs/compression.h
> +++ b/fs/btrfs/compression.h
> @@ -97,7 +97,7 @@ enum btrfs_compression_type {
> BTRFS_COMPRESS_ZLIB = 1,
> BTRFS_COMPRESS_LZO = 2,
> BTRFS_COMPRESS_ZSTD = 3,
> - BTRFS_COMPRESS_TYPES = 3,
> + BTRFS_COMPRESS_TYPES = 4,
> };
>
> struct btrfs_compress_op {
> @@ -125,6 +125,7 @@ struct btrfs_compress_op {
> void (*set_level)(struct list_head *ws, unsigned int type);
> };
>
> +extern const struct btrfs_compress_op btrfs_heuristic_compress;
> extern const struct btrfs_compress_op btrfs_zlib_compress;
> extern const struct btrfs_compress_op btrfs_lzo_compress;
> extern const struct btrfs_compress_op btrfs_zstd_compress;
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9c8e1734429c..20081465a451 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1410,7 +1410,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
> return -EINVAL;
>
> if (do_compress) {
> - if (range->compress_type > BTRFS_COMPRESS_TYPES)
> + if (range->compress_type >= BTRFS_COMPRESS_TYPES)
> return -EINVAL;
> if (range->compress_type)
> compress_type = range->compress_type;
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index a62e1e837a89..c88e146d8e99 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -133,9 +133,9 @@ static int check_extent_data_item(struct btrfs_fs_info *fs_info,
> * Support for new compression/encryption must introduce incompat flag,
> * and must be caught in open_ctree().
> */
> - if (btrfs_file_extent_compression(leaf, fi) > BTRFS_COMPRESS_TYPES) {
> + if (btrfs_file_extent_compression(leaf, fi) >= BTRFS_COMPRESS_TYPES) {
> file_extent_err(fs_info, leaf, slot,
> - "invalid compression for file extent, have %u expect range [0, %u]",
> + "invalid compression for file extent, have %u expect range [0, %u)",
> btrfs_file_extent_compression(leaf, fi),
> BTRFS_COMPRESS_TYPES);
> return -EUCLEAN;
>
next prev parent reply other threads:[~2019-01-29 7:53 UTC|newest]
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 [this message]
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
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 publicly 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=4bc26773-da13-3730-afda-78d672f348bc@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.