Re: [PATCH 7/7] btrfs: remove a BUG_ON() from merge_reloc_roots()

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

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> This was pretty subtle, we default to reloc roots having 0 root refs, so
> if we crash in the middle of the relocation they can just be deleted.
> If we successfully complete the relocation operations we'll set our root
> refs to 1 in prepare_to_merge() and then go on to merge_reloc_roots().
> 
> At prepare_to_merge() time if any of the reloc roots have a 0 reference
> still, we will remove that reloc root from our reloc root rb tree, and
> then clean it up later.
> 
> However this only happens if we successfully start a transaction.  If
> we've aborted previously we will skip this step completely, and only
> have reloc roots with a reference count of 0, but were never properly
> removed from the reloc control's rb tree.

Great, this explains the reason why we're seeing one internal report of
the BUG_ON() get triggered.

> 
> This isn't a problem per-se, our references are held by the list the
> reloc roots are on, and by the original root the reloc root belongs to.
> If we end up in this situation all the reloc roots will be added to the
> dirty_reloc_list, and then properly dropped at that point.  The reloc
> control will be free'd and the rb tree is no longer used.
> 
> There were two options when fixing this, one was to remove the BUG_ON(),
> the other was to make prepare_to_merge() handle the case where we
> couldn't start a trans handle.
> 
> IMO this is the cleaner solution.  I started with handling the error in
> prepare_to_merge(), but it turned out super ugly.  And in the end this
> BUG_ON() simply doesn't matter, the cleanup was happening properly, we
> were just panicing because this BUG_ON() only matters in the success
> case.  So I've opted to just remove it and add a comment where it was.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Since there is a comment added, it looks pretty OK to me.

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

Thanks,
Qu
> ---
>  fs/btrfs/relocation.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index c8ff28930677..387b0e7f1372 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2642,7 +2642,19 @@ void merge_reloc_roots(struct reloc_control *rc)
>  			free_reloc_roots(&reloc_roots);
>  	}
>  
> -	BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
> +	/*
> +	 * We used to have
> +	 *
> +	 * BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
> +	 *
> +	 * here, but it's wrong.  If we fail to start the transaction in
> +	 * prepare_to_merge() we will have only 0 ref reloc roots, none of which
> +	 * have actually been removed from the reloc_root_tree rb tree.  This is
> +	 * fine because we're bailing here, and we hold a reference on the root
> +	 * for the list that holds it, so these roots will be cleaned up when we
> +	 * do the reloc_dirty_list afterwards.  Meanwhile the root->reloc_root
> +	 * will be cleaned up on unmount.
> +	 */
>  }
>  
>  static void free_block_list(struct rb_root *blocks)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]