* [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root
@ 2020-02-21 13:11 Nikolay Borisov
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Nikolay Borisov @ 2020-02-21 13:11 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
__del_reloc_root is called from:
a) Transaction commit via:
btrfs_transaction_commit
commit_fs_roots
btrfs_update_reloc_root
__del_reloc_root
b) From the relocation thread with the following call chains:
relocate_block_group
merge_reloc_roots
insert_dirty_subvol
btrfs_update_reloc_root
__del_reloc_root
c) merge_reloc_roots
free_reloc_roots
__del_reloc_roots
(The above call chain can called from btrfs_recover_relocation as well
but for the purpose of this fix this is irrelevant).
The commont data structure that needs protecting is
fs_info->reloc_ctl->reloc_list as reloc roots are anchored at this list.
Turns out it's no needed to hold the trans_lock in __del_reloc_root
since consistency is already guaranteed by call chain b) above holding
a transaction while calling insert_dirty_subvol, meaning we cannot have
a concurrent transaction commit. For call chain c) above a snapshot of
the fs_info->reloc_ctl->reloc_list is taken with reloc_mutex held and
free_reloc_roots is called on this local snapshot.
Those invariants are sufficient to prevent racing calls to
__del_reloc_root alongside other users of the list, as such it's fine
to drop the lock acquisition.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/relocation.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8076c340749f..e5cb64409f7c 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1381,9 +1381,7 @@ static void __del_reloc_root(struct btrfs_root *root)
BUG_ON((struct btrfs_root *)node->data != root);
}
- spin_lock(&fs_info->trans_lock);
list_del_init(&root->root_list);
- spin_unlock(&fs_info->trans_lock);
kfree(node);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots
2020-02-21 13:11 [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Nikolay Borisov
@ 2020-02-21 13:11 ` Nikolay Borisov
2020-02-21 13:17 ` [PATCH v2 " Nikolay Borisov
` (2 more replies)
2020-02-21 13:22 ` [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Qu Wenruo
2020-04-10 16:29 ` David Sterba
2 siblings, 3 replies; 7+ messages in thread
From: Nikolay Borisov @ 2020-02-21 13:11 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
The function always works on a local copy of the reloc root list, which
cannot be modified outside of it so using list_for_each_entry is fine.
Additionally the macro handles empty lists so drop list_empty checks of
callers. No semantic changes.
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
fs/btrfs/relocation.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index e5cb64409f7c..f13b79adf6b0 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2523,13 +2523,10 @@ int prepare_to_merge(struct reloc_control *rc, int err)
static noinline_for_stack
void free_reloc_roots(struct list_head *list)
{
- struct btrfs_root *reloc_root;
+ struct btrfs_root *reloc_root, *tmp;
- while (!list_empty(list)) {
- reloc_root = list_entry(list->next, struct btrfs_root,
- root_list);
+ list_for_each_entry_safe(reloc_root, tmp, list, root_list)
__del_reloc_root(reloc_root);
- }
}
static noinline_for_stack
@@ -2588,15 +2585,13 @@ void merge_reloc_roots(struct reloc_control *rc)
out:
if (ret) {
btrfs_handle_fs_error(fs_info, ret, NULL);
- if (!list_empty(&reloc_roots))
- free_reloc_roots(&reloc_roots);
+ free_reloc_roots(&reloc_roots);
/* new reloc root may be added */
mutex_lock(&fs_info->reloc_mutex);
list_splice_init(&rc->reloc_roots, &reloc_roots);
mutex_unlock(&fs_info->reloc_mutex);
- if (!list_empty(&reloc_roots))
- free_reloc_roots(&reloc_roots);
+ free_reloc_roots(&reloc_roots);
}
BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
@@ -4689,8 +4684,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
out_free:
kfree(rc);
out:
- if (!list_empty(&reloc_roots))
- free_reloc_roots(&reloc_roots);
+ free_reloc_roots(&reloc_roots);
btrfs_free_path(path);
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
@ 2020-02-21 13:17 ` Nikolay Borisov
2020-02-21 13:23 ` [PATCH " Qu Wenruo
2020-04-10 16:33 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: Nikolay Borisov @ 2020-02-21 13:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: Nikolay Borisov
The function always works on a local copy of the reloc root list, which
cannot be modified outside of it so using list_for_each_entry is fine.
Additionally the macro handles empty lists so drop list_empty checks of
callers. No semantic changes.
---
V2: Base the patch on current misc-next rather than on a branch with Josef's
subvol cleanups part 2.
fs/btrfs/relocation.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 4b8c80de925b..a8898894f443 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2523,11 +2523,9 @@ int prepare_to_merge(struct reloc_control *rc, int err)
static noinline_for_stack
void free_reloc_roots(struct list_head *list)
{
- struct btrfs_root *reloc_root;
+ struct btrfs_root *reloc_root, *tmp;
- while (!list_empty(list)) {
- reloc_root = list_entry(list->next, struct btrfs_root,
- root_list);
+ list_for_each_entry_safe(reloc_root, tmp, list, root_list) {
__del_reloc_root(reloc_root);
free_extent_buffer(reloc_root->node);
free_extent_buffer(reloc_root->commit_root);
@@ -2592,15 +2590,13 @@ void merge_reloc_roots(struct reloc_control *rc)
out:
if (ret) {
btrfs_handle_fs_error(fs_info, ret, NULL);
- if (!list_empty(&reloc_roots))
- free_reloc_roots(&reloc_roots);
+ free_reloc_roots(&reloc_roots);
/* new reloc root may be added */
mutex_lock(&fs_info->reloc_mutex);
list_splice_init(&rc->reloc_roots, &reloc_roots);
mutex_unlock(&fs_info->reloc_mutex);
- if (!list_empty(&reloc_roots))
- free_reloc_roots(&reloc_roots);
+ free_reloc_roots(&reloc_roots);
}
BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
@@ -4693,8 +4689,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
out_free:
kfree(rc);
out:
- if (!list_empty(&reloc_roots))
- free_reloc_roots(&reloc_roots);
+ free_reloc_roots(&reloc_roots);
btrfs_free_path(path);
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root
2020-02-21 13:11 [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Nikolay Borisov
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
@ 2020-02-21 13:22 ` Qu Wenruo
2020-04-10 16:29 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2020-02-21 13:22 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2020/2/21 下午9:11, Nikolay Borisov wrote:
> __del_reloc_root is called from:
>
> a) Transaction commit via:
> btrfs_transaction_commit
> commit_fs_roots
> btrfs_update_reloc_root
> __del_reloc_root
>
> b) From the relocation thread with the following call chains:
>
> relocate_block_group
> merge_reloc_roots
> insert_dirty_subvol
> btrfs_update_reloc_root
> __del_reloc_root
>
> c) merge_reloc_roots
> free_reloc_roots
> __del_reloc_roots
>
> (The above call chain can called from btrfs_recover_relocation as well
> but for the purpose of this fix this is irrelevant).
>
> The commont data structure that needs protecting is
> fs_info->reloc_ctl->reloc_list as reloc roots are anchored at this list.
> Turns out it's no needed to hold the trans_lock in __del_reloc_root
> since consistency is already guaranteed by call chain b) above holding
> a transaction while calling insert_dirty_subvol, meaning we cannot have
> a concurrent transaction commit. For call chain c) above a snapshot of
> the fs_info->reloc_ctl->reloc_list is taken with reloc_mutex held and
> free_reloc_roots is called on this local snapshot.
>
> Those invariants are sufficient to prevent racing calls to
> __del_reloc_root alongside other users of the list, as such it's fine
> to drop the lock acquisition.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/relocation.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 8076c340749f..e5cb64409f7c 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1381,9 +1381,7 @@ static void __del_reloc_root(struct btrfs_root *root)
> BUG_ON((struct btrfs_root *)node->data != root);
> }
>
> - spin_lock(&fs_info->trans_lock);
> list_del_init(&root->root_list);
> - spin_unlock(&fs_info->trans_lock);
> kfree(node);
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
2020-02-21 13:17 ` [PATCH v2 " Nikolay Borisov
@ 2020-02-21 13:23 ` Qu Wenruo
2020-04-10 16:33 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2020-02-21 13:23 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
On 2020/2/21 下午9:11, Nikolay Borisov wrote:
> The function always works on a local copy of the reloc root list, which
> cannot be modified outside of it so using list_for_each_entry is fine.
> Additionally the macro handles empty lists so drop list_empty checks of
> callers. No semantic changes.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: Qu Wenruo <wqu@suse.com>
Thanks,
Qu
> ---
> fs/btrfs/relocation.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index e5cb64409f7c..f13b79adf6b0 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2523,13 +2523,10 @@ int prepare_to_merge(struct reloc_control *rc, int err)
> static noinline_for_stack
> void free_reloc_roots(struct list_head *list)
> {
> - struct btrfs_root *reloc_root;
> + struct btrfs_root *reloc_root, *tmp;
>
> - while (!list_empty(list)) {
> - reloc_root = list_entry(list->next, struct btrfs_root,
> - root_list);
> + list_for_each_entry_safe(reloc_root, tmp, list, root_list)
> __del_reloc_root(reloc_root);
> - }
> }
>
> static noinline_for_stack
> @@ -2588,15 +2585,13 @@ void merge_reloc_roots(struct reloc_control *rc)
> out:
> if (ret) {
> btrfs_handle_fs_error(fs_info, ret, NULL);
> - if (!list_empty(&reloc_roots))
> - free_reloc_roots(&reloc_roots);
> + free_reloc_roots(&reloc_roots);
>
> /* new reloc root may be added */
> mutex_lock(&fs_info->reloc_mutex);
> list_splice_init(&rc->reloc_roots, &reloc_roots);
> mutex_unlock(&fs_info->reloc_mutex);
> - if (!list_empty(&reloc_roots))
> - free_reloc_roots(&reloc_roots);
> + free_reloc_roots(&reloc_roots);
> }
>
> BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
> @@ -4689,8 +4684,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
> out_free:
> kfree(rc);
> out:
> - if (!list_empty(&reloc_roots))
> - free_reloc_roots(&reloc_roots);
> + free_reloc_roots(&reloc_roots);
>
> btrfs_free_path(path);
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root
2020-02-21 13:11 [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Nikolay Borisov
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
2020-02-21 13:22 ` [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Qu Wenruo
@ 2020-04-10 16:29 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2020-04-10 16:29 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
On Fri, Feb 21, 2020 at 03:11:23PM +0200, Nikolay Borisov wrote:
> __del_reloc_root is called from:
>
> a) Transaction commit via:
> btrfs_transaction_commit
> commit_fs_roots
> btrfs_update_reloc_root
> __del_reloc_root
>
> b) From the relocation thread with the following call chains:
>
> relocate_block_group
> merge_reloc_roots
> insert_dirty_subvol
> btrfs_update_reloc_root
> __del_reloc_root
>
> c) merge_reloc_roots
> free_reloc_roots
> __del_reloc_roots
>
> (The above call chain can called from btrfs_recover_relocation as well
> but for the purpose of this fix this is irrelevant).
>
> The commont data structure that needs protecting is
> fs_info->reloc_ctl->reloc_list as reloc roots are anchored at this list.
> Turns out it's no needed to hold the trans_lock in __del_reloc_root
> since consistency is already guaranteed by call chain b) above holding
> a transaction while calling insert_dirty_subvol, meaning we cannot have
> a concurrent transaction commit. For call chain c) above a snapshot of
> the fs_info->reloc_ctl->reloc_list is taken with reloc_mutex held and
> free_reloc_roots is called on this local snapshot.
>
> Those invariants are sufficient to prevent racing calls to
> __del_reloc_root alongside other users of the list, as such it's fine
> to drop the lock acquisition.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/relocation.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 8076c340749f..e5cb64409f7c 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1381,9 +1381,7 @@ static void __del_reloc_root(struct btrfs_root *root)
> BUG_ON((struct btrfs_root *)node->data != root);
> }
>
> - spin_lock(&fs_info->trans_lock);
> list_del_init(&root->root_list);
> - spin_unlock(&fs_info->trans_lock);
This has been obsoleted by patch f44deb7442edf42abee6 ("btrfs: hold a
ref on the root->reloc_root"), I think the locks are still needed but
you may want take a look.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
2020-02-21 13:17 ` [PATCH v2 " Nikolay Borisov
2020-02-21 13:23 ` [PATCH " Qu Wenruo
@ 2020-04-10 16:33 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2020-04-10 16:33 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
On Fri, Feb 21, 2020 at 03:11:24PM +0200, Nikolay Borisov wrote:
> The function always works on a local copy of the reloc root list, which
> cannot be modified outside of it so using list_for_each_entry is fine.
> Additionally the macro handles empty lists so drop list_empty checks of
> callers. No semantic changes.
>
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
This is still relevant for misc-next, so I'm applying it to misc-next.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-10 16:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-21 13:11 [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Nikolay Borisov
2020-02-21 13:11 ` [PATCH 2/2] btrfs: Use list_for_each_entry_safe in free_reloc_roots Nikolay Borisov
2020-02-21 13:17 ` [PATCH v2 " Nikolay Borisov
2020-02-21 13:23 ` [PATCH " Qu Wenruo
2020-04-10 16:33 ` David Sterba
2020-02-21 13:22 ` [PATCH 1/2] btrfs: Remove superflous lock acquisition in __del_reloc_root Qu Wenruo
2020-04-10 16:29 ` David Sterba
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.