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@xxxxxxxxxxxxxx> Since there is a comment added, it looks pretty OK to me. Reviewed-by: Qu Wenruo <wqu@xxxxxxxx> 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:
signature.asc
Description: OpenPGP digital signature
