Re: [PATCH 04/10] btrfs: cleanup select_reloc_root

[Boris Burkov <boris@bur.io>]

On Thu, Oct 03, 2024 at 11:43:06AM -0400, Josef Bacik wrote:
> We have this setup as a loop, but in reality we will never walk back up
> the backref tree, if we do then it's a bug.  Get rid of the loop and
> handle the case where we have node->new_bytenr set at all.  Previous the
> check was only if node->new_bytenr != root->node->start, but if it did
> then we would hit the WARN_ON() and walk back up the tree.
> 
> Instead we want to just freak out if ->new_bytenr is set, and then do
> the normal updating of the node for the reloc root and carry on.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 144 ++++++++++++++++++------------------------
>  1 file changed, 61 insertions(+), 83 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index ac3658dce6a3..6b2308671d83 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c

I think this (and most other functions in backref caching) could really
benefit from some description of their invariants. I think that will pay
its weight in gold if we ever have to read this code again in 10
years...

> @@ -2058,97 +2058,75 @@ struct btrfs_root *select_reloc_root(struct btrfs_trans_handle *trans,
>  	int index = 0;
>  	int ret;
>  
> -	next = node;
> -	while (1) {
> -		cond_resched();
> -		next = walk_up_backref(next, edges, &index);
> -		root = next->root;
> +	next = walk_up_backref(node, edges, &index);
> +	root = next->root;
>  
> -		/*
> -		 * If there is no root, then our references for this block are
> -		 * incomplete, as we should be able to walk all the way up to a
> -		 * block that is owned by a root.
> -		 *
> -		 * This path is only for SHAREABLE roots, so if we come upon a
> -		 * non-SHAREABLE root then we have backrefs that resolve
> -		 * improperly.
> -		 *
> -		 * Both of these cases indicate file system corruption, or a bug
> -		 * in the backref walking code.
> -		 */
> -		if (!root) {
> -			ASSERT(0);
> -			btrfs_err(trans->fs_info,
> -		"bytenr %llu doesn't have a backref path ending in a root",
> -				  node->bytenr);
> -			return ERR_PTR(-EUCLEAN);
> -		}
> -		if (!test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) {
> -			ASSERT(0);
> -			btrfs_err(trans->fs_info,
> -	"bytenr %llu has multiple refs with one ending in a non-shareable root",
> -				  node->bytenr);
> -			return ERR_PTR(-EUCLEAN);
> -		}
> +	/*
> +	 * If there is no root, then our references for this block are
> +	 * incomplete, as we should be able to walk all the way up to a block
> +	 * that is owned by a root.
> +	 *
> +	 * This path is only for SHAREABLE roots, so if we come upon a
> +	 * non-SHAREABLE root then we have backrefs that resolve improperly.
> +	 *
> +	 * Both of these cases indicate file system corruption, or a bug in the
> +	 * backref walking code.
> +	 */
> +	if (!root) {
> +		ASSERT(0);
> +		btrfs_err(trans->fs_info,
> +			  "bytenr %llu doesn't have a backref path ending in a root",
> +			  node->bytenr);
> +		return ERR_PTR(-EUCLEAN);
> +	}
> +	if (!test_bit(BTRFS_ROOT_SHAREABLE, &root->state)) {
> +		ASSERT(0);
> +		btrfs_err(trans->fs_info,
> +			  "bytenr %llu has multiple refs with one ending in a non-shareable root",
> +			  node->bytenr);
> +		return ERR_PTR(-EUCLEAN);
> +	}
>  
> -		if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID) {
> -			ret = record_reloc_root_in_trans(trans, root);
> -			if (ret)
> -				return ERR_PTR(ret);
> -			break;
> -		}
> -
> -		ret = btrfs_record_root_in_trans(trans, root);
> +	if (btrfs_root_id(root) == BTRFS_TREE_RELOC_OBJECTID) {
> +		ret = record_reloc_root_in_trans(trans, root);
>  		if (ret)
>  			return ERR_PTR(ret);
> -		root = root->reloc_root;
> -
> -		/*
> -		 * We could have raced with another thread which failed, so
> -		 * root->reloc_root may not be set, return ENOENT in this case.
> -		 */
> -		if (!root)
> -			return ERR_PTR(-ENOENT);
> -
> -		if (next->new_bytenr != root->node->start) {
> -			/*
> -			 * We just created the reloc root, so we shouldn't have
> -			 * ->new_bytenr set yet. If it is then we have multiple
> -			 *  roots pointing at the same bytenr which indicates
> -			 *  corruption, or we've made a mistake in the backref
> -			 *  walking code.
> -			 */
> -			ASSERT(next->new_bytenr == 0);
> -			if (next->new_bytenr) {
> -				btrfs_err(trans->fs_info,
> -	"bytenr %llu possibly has multiple roots pointing at the same bytenr %llu",
> -					  node->bytenr, next->bytenr);
> -				return ERR_PTR(-EUCLEAN);
> -			}
> -
> -			next->new_bytenr = root->node->start;
> -			btrfs_put_root(next->root);
> -			next->root = btrfs_grab_root(root);
> -			ASSERT(next->root);
> -			mark_block_processed(rc, next);
> -			break;
> -		}
> -
> -		WARN_ON(1);
> -		root = NULL;
> -		next = walk_down_backref(edges, &index);
> -		if (!next || next->level <= node->level)
> -			break;
> +		goto found;
>  	}
> -	if (!root) {
> -		/*
> -		 * This can happen if there's fs corruption or if there's a bug
> -		 * in the backref lookup code.
> -		 */
> -		ASSERT(0);
> +
> +	ret = btrfs_record_root_in_trans(trans, root);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	root = root->reloc_root;
> +
> +	/*
> +	 * We could have raced with another thread which failed, so
> +	 * root->reloc_root may not be set, return ENOENT in this case.
> +	 */
> +	if (!root)
>  		return ERR_PTR(-ENOENT);
> +
> +	if (next->new_bytenr) {
> +		/*
> +		 * We just created the reloc root, so we shouldn't have
> +		 * ->new_bytenr set yet. If it is then we have multiple
> +		 * roots pointing at the same bytenr which indicates
> +		 * corruption, or we've made a mistake in the backref
> +		 * walking code.
> +		 */
> +		ASSERT(next->new_bytenr == 0);
> +		btrfs_err(trans->fs_info,
> +			  "bytenr %llu possibly has multiple roots pointing at the same bytenr %llu",
> +			  node->bytenr, next->bytenr);
> +		return ERR_PTR(-EUCLEAN);
>  	}
>  
> +	next->new_bytenr = root->node->start;
> +	btrfs_put_root(next->root);
> +	next->root = btrfs_grab_root(root);
> +	ASSERT(next->root);
> +	mark_block_processed(rc, next);
> +found:
>  	next = node;
>  	/* setup backref node path for btrfs_reloc_cow_block */
>  	while (1) {
> -- 
> 2.43.0
>