On 3/2/20 8:12 PM, Qu Wenruo wrote:
On 2020/3/3 上午2:47, Josef Bacik wrote:
We previously were relying on root->reloc_root to be cleaned up by the
drop snapshot, or the error handling. However if btrfs_drop_snapshot()
failed it wouldn't drop the ref for the root. Also we sort of depend on
the right thing to happen with moving reloc roots between lists and the
fs root they belong to, which makes it hard to figure out who owns the
reference.
Fix this by explicitly holding a reference on the reloc root for
roo->reloc_root. This means that we hold two references on reloc roots,
one for whichever reloc_roots list it's attached to, and the
root->reloc_root we're on.
This makes it easier to reason out who owns a reference on the root, and
when it needs to be dropped.
Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
A small question inlined below, despite that,
Reviewed-by: Qu Wenruo <wqu@xxxxxxxx>
---
fs/btrfs/relocation.c | 44 ++++++++++++++++++++++++++++++++-----------
1 file changed, 33 insertions(+), 11 deletions(-)
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index acd21c156378..c8ff28930677 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1384,6 +1384,7 @@ static void __del_reloc_root(struct btrfs_root *root)
struct rb_node *rb_node;
struct mapping_node *node = NULL;
struct reloc_control *rc = fs_info->reloc_ctl;
+ bool put_ref = false;
if (rc && root->node) {
spin_lock(&rc->reloc_root_tree.lock);
@@ -1400,8 +1401,13 @@ static void __del_reloc_root(struct btrfs_root *root)
}
spin_lock(&fs_info->trans_lock);
- list_del_init(&root->root_list);
+ if (!list_empty(&root->root_list)) {
Can we make the ref of reloc root completely free from the list operation?
It still looks like a compromise between fully ref counted reloc root
and original non-ref counted one.
No because we have things like prepare_to_merge() which does
list_del_init(&reloc_root->list);
btrfs_update_reloc_root() <- which can call __del_reloc_root
list_add_tail(&reloc_root->list, &rc->reloc_roots);
so we don't want to drop the ref in __del_reloc_root there, as we're still
technically going to be holding the ref for the list to be cleared later. Thanks,
Josef
