[PATCH] btrfs-progs: fix failed resume due to bad search

[Qu Wenruo <wqu@suse.com>]

[BUG]
There is a bug report that when converting to bg tree crashed, the
resulted fs is unable to be resumed.

This problems comes with the following error messages:

  ./btrfstune --convert-to-block-group-tree /dev/sda
  ERROR: Corrupted fs, no valid METADATA block group found
  ERROR: failed to delete block group item from the old root: -117
  ERROR: failed to convert the filesystem to block group tree feature
  ERROR: btrfstune failed
  extent buffer leak: start 17825576632320 len 16384

[CAUSE]
When resuming a interrupted conversion, we go through
read_converting_block_groups() to handle block group items in both
extent and block group trees.

However for the block group items in the extent tree, there are several
problems involved:

- Uninitialized @key inside read_old_block_groups_from_root()
  Here we only set @key.type, not setting @key.objectid for the initial
  search.

  Thus if we're unlukcy, we can got (u64)-1 as key.objectid, and exit
  the search immediately.

- Wrong search direction
  The conversion is converting block groups in descending order, but the
  block groups read is in ascending order.
  Meaning if we start from the last converted block group, we would at
  most read one block group.

[FIX]
To fix the problems, this patch would just remove
read_old_block_groups_from_root() function completely.

As for the conversion, we ensured the block group item is either in the
old extent or the new block group tree.
Thus there is no special handling needed reading block groups.

We only need to read all block groups from both trees, using the same
read_old_block_groups_from_root() function.

Reported-by: Konstantinos Skarlatos <k.skarlatos@gmail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
To Konstantinos:

The bug I fixed here can explain all the failures you hit (the initial
one and the one after the quick diff).

Please verify if this helps and report back (without the quick diff in
the original thread).

We may have other corner cases to go, but I believe the patch itself is
necessary no matter what, as the deleted code is really
over-engineered and buggy.
---
 kernel-shared/extent-tree.c | 79 +------------------------------------
 1 file changed, 1 insertion(+), 78 deletions(-)

diff --git a/kernel-shared/extent-tree.c b/kernel-shared/extent-tree.c
index 7022643a9843..4d6bf2b228e9 100644
--- a/kernel-shared/extent-tree.c
+++ b/kernel-shared/extent-tree.c
@@ -2852,83 +2852,6 @@ out:
 	return ret;
 }
 
-/*
- * Helper to read old block groups items from specified root.
- *
- * The difference between this and read_block_groups_from_root() is,
- * we will exit if we have already read the last bg in the old root.
- *
- * This is to avoid wasting time finding bg items which should be in the
- * new root.
- */
-static int read_old_block_groups_from_root(struct btrfs_fs_info *fs_info,
-					   struct btrfs_root *root)
-{
-	struct btrfs_path path = {0};
-	struct btrfs_key key;
-	struct cache_extent *ce;
-	/* The last block group bytenr in the old root. */
-	u64 last_bg_in_old_root;
-	int ret;
-
-	if (fs_info->last_converted_bg_bytenr != (u64)-1) {
-		/*
-		 * We know the last converted bg in the other tree, load the chunk
-		 * before that last converted as our last bg in the tree.
-		 */
-		ce = search_cache_extent(&fs_info->mapping_tree.cache_tree,
-			         fs_info->last_converted_bg_bytenr);
-		if (!ce || ce->start != fs_info->last_converted_bg_bytenr) {
-			error("no chunk found for bytenr %llu",
-			      fs_info->last_converted_bg_bytenr);
-			return -ENOENT;
-		}
-		ce = prev_cache_extent(ce);
-		/*
-		 * We should have previous unconverted chunk, or we have
-		 * already finished the convert.
-		 */
-		ASSERT(ce);
-
-		last_bg_in_old_root = ce->start;
-	} else {
-		last_bg_in_old_root = (u64)-1;
-	}
-
-	key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
-
-	while (true) {
-		ret = find_first_block_group(root, &path, &key);
-		if (ret > 0) {
-			ret = 0;
-			goto out;
-		}
-		if (ret != 0) {
-			goto out;
-		}
-		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
-
-		ret = read_one_block_group(fs_info, &path);
-		if (ret < 0 && ret != -ENOENT)
-			goto out;
-
-		/* We have reached last bg in the old root, no need to continue */
-		if (key.objectid >= last_bg_in_old_root)
-			break;
-
-		if (key.offset == 0)
-			key.objectid++;
-		else
-			key.objectid = key.objectid + key.offset;
-		key.offset = 0;
-		btrfs_release_path(&path);
-	}
-	ret = 0;
-out:
-	btrfs_release_path(&path);
-	return ret;
-}
-
 /* Helper to read all block groups items from specified root. */
 static int read_block_groups_from_root(struct btrfs_fs_info *fs_info,
 					   struct btrfs_root *root)
@@ -2989,7 +2912,7 @@ static int read_converting_block_groups(struct btrfs_fs_info *fs_info)
 		return ret;
 	}
 
-	ret = read_old_block_groups_from_root(fs_info, old_root);
+	ret = read_block_groups_from_root(fs_info, old_root);
 	if (ret < 0) {
 		error("failed to load block groups from the old root: %d", ret);
 		return ret;
-- 
2.42.0