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.
Thanks,
Qu
> + put_ref = true;
> + list_del_init(&root->root_list);
> + }
> spin_unlock(&fs_info->trans_lock);
> + if (put_ref)
> + btrfs_put_root(root);
> kfree(node);
> }
>
> @@ -1555,7 +1561,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>
> ret = __add_reloc_root(reloc_root);
> BUG_ON(ret < 0);
> - root->reloc_root = reloc_root;
> + root->reloc_root = btrfs_grab_root(reloc_root);
> return 0;
> }
>
> @@ -1576,6 +1582,13 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
> reloc_root = root->reloc_root;
> root_item = &reloc_root->root_item;
>
> + /*
> + * We are probably ok here, but __del_reloc_root() will drop its ref of
> + * the root. We have the ref fro root->reloc_root, but just in case
s/fro/for/
> + * hold it while we update the reloc root.
> + */
> + btrfs_grab_root(reloc_root);
> +
> /* root->reloc_root will stay until current relocation finished */
> if (fs_info->reloc_ctl->merge_reloc_tree &&
> btrfs_root_refs(root_item) == 0) {
> @@ -1597,7 +1610,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
> ret = btrfs_update_root(trans, fs_info->tree_root,
> &reloc_root->root_key, root_item);
> BUG_ON(ret);
> -
> + btrfs_put_root(reloc_root);
> out:
> return 0;
> }
> @@ -2297,19 +2310,28 @@ static int clean_dirty_subvols(struct reloc_control *rc)
> */
> smp_wmb();
> clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
> -
> if (reloc_root) {
> -
> + /*
> + * btrfs_drop_snapshot drops our ref we hold for
> + * ->reloc_root. If it fails however we must
> + * drop the ref ourselves.
> + */
> ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> - if (ret2 < 0 && !ret)
> - ret = ret2;
> + if (ret2 < 0) {
> + btrfs_put_root(reloc_root);
> + if (!ret)
> + ret = ret2;
> + }
> }
> btrfs_put_root(root);
> } else {
> /* Orphan reloc tree, just clean it up */
> ret2 = btrfs_drop_snapshot(root, NULL, 0, 1);
> - if (ret2 < 0 && !ret)
> - ret = ret2;
> + if (ret2 < 0) {
> + btrfs_put_root(root);
> + if (!ret)
> + ret = ret2;
> + }
> }
> }
> return ret;
> @@ -4687,7 +4709,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>
> err = __add_reloc_root(reloc_root);
> BUG_ON(err < 0); /* -ENOMEM or logic error */
> - fs_root->reloc_root = reloc_root;
> + fs_root->reloc_root = btrfs_grab_root(reloc_root);
> btrfs_put_root(fs_root);
> }
>
> @@ -4912,7 +4934,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
>
> ret = __add_reloc_root(reloc_root);
> BUG_ON(ret < 0);
> - new_root->reloc_root = reloc_root;
> + new_root->reloc_root = btrfs_grab_root(reloc_root);
>
> if (rc->create_reloc_tree)
> ret = clone_backref_node(trans, rc, root, reloc_root);
>
Attachment:
signature.asc
Description: OpenPGP digital signature
