Re: [PATCH 6/7] btrfs: hold a ref on the root->reloc_root

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4904 bytes --]



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@toxicpanda.com>

A small question inlined below, despite that,

Reviewed-by: Qu Wenruo <wqu@suse.com>

> ---
>  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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]