* [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 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 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
* 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 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
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.