* [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels
@ 2017-10-23 10:51 Nikolay Borisov
2017-10-23 10:51 ` [PATCH 2/2] btrfs: Move checks from btrfs_wq_run_delayed_node to btrfs_balance_delayed_items Nikolay Borisov
2017-10-23 11:43 ` [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Qu Wenruo
0 siblings, 2 replies; 5+ messages in thread
From: Nikolay Borisov @ 2017-10-23 10:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
Currently btrfs_async_run_delayed_root's implementation uses 3 goto labels to
mimic the functionality of a simple do {} while loop. Refactor the function
to use a do {} while construct, making intention clear and code easier to
follow. No functional changes
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/delayed-inode.c | 52 +++++++++++++++++++++++++-----------------------
1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 19e4ad2f3f2e..1bfdb90d7633 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1323,40 +1323,42 @@ static void btrfs_async_run_delayed_root(struct btrfs_work *work)
if (!path)
goto out;
-again:
- if (atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND / 2)
- goto free_path;
+ do {
+ if (atomic_read(&delayed_root->items) <
+ BTRFS_DELAYED_BACKGROUND / 2)
+ break;
- delayed_node = btrfs_first_prepared_delayed_node(delayed_root);
- if (!delayed_node)
- goto free_path;
+ delayed_node = btrfs_first_prepared_delayed_node(delayed_root);
+ if (!delayed_node)
+ break;
- path->leave_spinning = 1;
- root = delayed_node->root;
+ path->leave_spinning = 1;
+ root = delayed_node->root;
- trans = btrfs_join_transaction(root);
- if (IS_ERR(trans))
- goto release_path;
+ trans = btrfs_join_transaction(root);
+ if (IS_ERR(trans)) {
+ btrfs_release_path(path);
+ btrfs_release_prepared_delayed_node(delayed_node);
+ total_done++;
+ continue;
+ }
- block_rsv = trans->block_rsv;
- trans->block_rsv = &root->fs_info->delayed_block_rsv;
+ block_rsv = trans->block_rsv;
+ trans->block_rsv = &root->fs_info->delayed_block_rsv;
- __btrfs_commit_inode_delayed_items(trans, path, delayed_node);
+ __btrfs_commit_inode_delayed_items(trans, path, delayed_node);
- trans->block_rsv = block_rsv;
- btrfs_end_transaction(trans);
- btrfs_btree_balance_dirty_nodelay(root->fs_info);
+ trans->block_rsv = block_rsv;
+ btrfs_end_transaction(trans);
+ btrfs_btree_balance_dirty_nodelay(root->fs_info);
-release_path:
- btrfs_release_path(path);
- total_done++;
+ btrfs_release_path(path);
+ btrfs_release_prepared_delayed_node(delayed_node);
+ total_done++;
- btrfs_release_prepared_delayed_node(delayed_node);
- if ((async_work->nr == 0 && total_done < BTRFS_DELAYED_WRITEBACK) ||
- total_done < async_work->nr)
- goto again;
+ } while ((async_work->nr == 0 && total_done < BTRFS_DELAYED_WRITEBACK)
+ || total_done < async_work->nr);
-free_path:
btrfs_free_path(path);
out:
wake_up(&delayed_root->wait);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] btrfs: Move checks from btrfs_wq_run_delayed_node to btrfs_balance_delayed_items
2017-10-23 10:51 [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Nikolay Borisov
@ 2017-10-23 10:51 ` Nikolay Borisov
2017-10-23 11:51 ` Qu Wenruo
2017-10-23 11:43 ` [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Qu Wenruo
1 sibling, 1 reply; 5+ messages in thread
From: Nikolay Borisov @ 2017-10-23 10:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
btrfs_balance_delayed_items is the sole caller of btrfs_wq_run_delayed_node and
already includes one of the checks whether the delayed inodes should be run. On
the other hand btrfs_wq_run_delayed_node duplicates that check and performs an
additional one for wq congestion.
Let's remove the duplicate check and move the congestion one in
btrfs_balance_delayed_items, leaving btrfs_wq_run_delayed_node to only care
about setting up the wq run. No functional changes.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/delayed-inode.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 1bfdb90d7633..b7a0ec2c41e6 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1371,10 +1371,6 @@ static int btrfs_wq_run_delayed_node(struct btrfs_delayed_root *delayed_root,
{
struct btrfs_async_delayed_work *async_work;
- if (atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND ||
- btrfs_workqueue_normal_congested(fs_info->delayed_workers))
- return 0;
-
async_work = kmalloc(sizeof(*async_work), GFP_NOFS);
if (!async_work)
return -ENOMEM;
@@ -1410,7 +1406,8 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
{
struct btrfs_delayed_root *delayed_root = fs_info->delayed_root;
- if (atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND)
+ if ((atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND) ||
+ btrfs_workqueue_normal_congested(fs_info->delayed_workers))
return;
if (atomic_read(&delayed_root->items) >= BTRFS_DELAYED_WRITEBACK) {
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels
2017-10-23 10:51 [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Nikolay Borisov
2017-10-23 10:51 ` [PATCH 2/2] btrfs: Move checks from btrfs_wq_run_delayed_node to btrfs_balance_delayed_items Nikolay Borisov
@ 2017-10-23 11:43 ` Qu Wenruo
2017-10-30 16:16 ` David Sterba
1 sibling, 1 reply; 5+ messages in thread
From: Qu Wenruo @ 2017-10-23 11:43 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 2970 bytes --]
On 2017年10月23日 18:51, Nikolay Borisov wrote:
> Currently btrfs_async_run_delayed_root's implementation uses 3 goto labels to
> mimic the functionality of a simple do {} while loop. Refactor the function
> to use a do {} while construct, making intention clear and code easier to
> follow. No functional changes
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Looks good to me.
Reviewed-by: Qu Wenruo <wqu@suse.com>
> ---
> fs/btrfs/delayed-inode.c | 52 +++++++++++++++++++++++++-----------------------
> 1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 19e4ad2f3f2e..1bfdb90d7633 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1323,40 +1323,42 @@ static void btrfs_async_run_delayed_root(struct btrfs_work *work)
> if (!path)
> goto out;
>
> -again:
> - if (atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND / 2)
> - goto free_path;
> + do {
> + if (atomic_read(&delayed_root->items) <
> + BTRFS_DELAYED_BACKGROUND / 2)
> + break;
>
> - delayed_node = btrfs_first_prepared_delayed_node(delayed_root);
> - if (!delayed_node)
> - goto free_path;
> + delayed_node = btrfs_first_prepared_delayed_node(delayed_root);
> + if (!delayed_node)
> + break;
>
> - path->leave_spinning = 1;
> - root = delayed_node->root;
> + path->leave_spinning = 1;
> + root = delayed_node->root;
>
> - trans = btrfs_join_transaction(root);
> - if (IS_ERR(trans))
> - goto release_path;
> + trans = btrfs_join_transaction(root);
> + if (IS_ERR(trans)) {
> + btrfs_release_path(path);
> + btrfs_release_prepared_delayed_node(delayed_node);
> + total_done++;
> + continue;
> + }
>
> - block_rsv = trans->block_rsv;
> - trans->block_rsv = &root->fs_info->delayed_block_rsv;
> + block_rsv = trans->block_rsv;
> + trans->block_rsv = &root->fs_info->delayed_block_rsv;
>
> - __btrfs_commit_inode_delayed_items(trans, path, delayed_node);
> + __btrfs_commit_inode_delayed_items(trans, path, delayed_node);
>
> - trans->block_rsv = block_rsv;
> - btrfs_end_transaction(trans);
> - btrfs_btree_balance_dirty_nodelay(root->fs_info);
> + trans->block_rsv = block_rsv;
> + btrfs_end_transaction(trans);
> + btrfs_btree_balance_dirty_nodelay(root->fs_info);
>
> -release_path:
> - btrfs_release_path(path);
> - total_done++;
> + btrfs_release_path(path);
> + btrfs_release_prepared_delayed_node(delayed_node);
> + total_done++;
>
> - btrfs_release_prepared_delayed_node(delayed_node);
> - if ((async_work->nr == 0 && total_done < BTRFS_DELAYED_WRITEBACK) ||
> - total_done < async_work->nr)
> - goto again;
> + } while ((async_work->nr == 0 && total_done < BTRFS_DELAYED_WRITEBACK)
> + || total_done < async_work->nr);
>
> -free_path:
> btrfs_free_path(path);
> out:
> wake_up(&delayed_root->wait);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 504 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] btrfs: Move checks from btrfs_wq_run_delayed_node to btrfs_balance_delayed_items
2017-10-23 10:51 ` [PATCH 2/2] btrfs: Move checks from btrfs_wq_run_delayed_node to btrfs_balance_delayed_items Nikolay Borisov
@ 2017-10-23 11:51 ` Qu Wenruo
0 siblings, 0 replies; 5+ messages in thread
From: Qu Wenruo @ 2017-10-23 11:51 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 2156 bytes --]
On 2017年10月23日 18:51, Nikolay Borisov wrote:
> btrfs_balance_delayed_items is the sole caller of btrfs_wq_run_delayed_node and
> already includes one of the checks whether the delayed inodes should be run. On
> the other hand btrfs_wq_run_delayed_node duplicates that check and performs an
> additional one for wq congestion.
>
> Let's remove the duplicate check and move the congestion one in
> btrfs_balance_delayed_items, leaving btrfs_wq_run_delayed_node to only care
> about setting up the wq run. No functional changes.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
btrfs_workqueue_normal_congested() is moved to the caller and removed
duplicated atomic_read().
Unless delayed_root->items get modified in the period, it should be good.
But anyway, the original code has nothing to protect different
atomic_read(), so I don't think it will cause any new problem.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/delayed-inode.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 1bfdb90d7633..b7a0ec2c41e6 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1371,10 +1371,6 @@ static int btrfs_wq_run_delayed_node(struct btrfs_delayed_root *delayed_root,
> {
> struct btrfs_async_delayed_work *async_work;
>
> - if (atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND ||
> - btrfs_workqueue_normal_congested(fs_info->delayed_workers))
> - return 0;
> -
> async_work = kmalloc(sizeof(*async_work), GFP_NOFS);
> if (!async_work)
> return -ENOMEM;
> @@ -1410,7 +1406,8 @@ void btrfs_balance_delayed_items(struct btrfs_fs_info *fs_info)
> {
> struct btrfs_delayed_root *delayed_root = fs_info->delayed_root;
>
> - if (atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND)
> + if ((atomic_read(&delayed_root->items) < BTRFS_DELAYED_BACKGROUND) ||
> + btrfs_workqueue_normal_congested(fs_info->delayed_workers))
> return;
>
> if (atomic_read(&delayed_root->items) >= BTRFS_DELAYED_WRITEBACK) {
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels
2017-10-23 11:43 ` [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Qu Wenruo
@ 2017-10-30 16:16 ` David Sterba
0 siblings, 0 replies; 5+ messages in thread
From: David Sterba @ 2017-10-30 16:16 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Nikolay Borisov, linux-btrfs
On Mon, Oct 23, 2017 at 07:43:02PM +0800, Qu Wenruo wrote:
>
>
> On 2017年10月23日 18:51, Nikolay Borisov wrote:
> > Currently btrfs_async_run_delayed_root's implementation uses 3 goto labels to
> > mimic the functionality of a simple do {} while loop. Refactor the function
> > to use a do {} while construct, making intention clear and code easier to
> > follow. No functional changes
> >
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>
> Looks good to me.
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
1-2 added to next.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-30 16:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-23 10:51 [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Nikolay Borisov
2017-10-23 10:51 ` [PATCH 2/2] btrfs: Move checks from btrfs_wq_run_delayed_node to btrfs_balance_delayed_items Nikolay Borisov
2017-10-23 11:51 ` Qu Wenruo
2017-10-23 11:43 ` [PATCH 1/2] btrfs: Make btrfs_async_run_delayed_root use a loop rather than multiple labels Qu Wenruo
2017-10-30 16:16 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).