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

[Konstantinos Skarlatos <k.skarlatos@gmail.com>]

Hi Qu, thanks for your patch. I just tried it on a clean btrfs-progs 
tree with my filesystem:

❯ ./btrfstune --convert-to-block-group-tree /dev/sda
[1]    796483 segmentation fault (core dumped)  ./btrfstune 
--convert-to-block-group-tree /dev/sda

Sep 28 17:46:17 elsinki kernel: btrfstune[796483]: segfault at 
ffffffffff000f2f ip 0000564b6c2107aa sp 00007ffdc8ad25c8 error 5 in 
btrfstune[564b6c1d1000+5b000] likely on CPU 3 (core 2, socket 0)
Sep 28 17:46:17 elsinki kernel: Code: ff 48 8b 34 24 48 8d 3d 5a d8 01 
00 b8 00 00 00 00 e8 5a 37 00 00 48 8b 33 bf 0a 00 00 00 e8 1d 0c fc ff 
eb a8 e8 86 0a fc ff <48> 8b 4f 20 48 8b 56 08 48 89 c8 48 03 47 28 48 
89 c7 b8 01 00 00
Sep 28 17:46:17 elsinki systemd[1]: Created slice Slice 
/system/systemd-coredump.
Sep 28 17:46:17 elsinki systemd[1]: Started Process Core Dump (PID 
796493/UID 0).
Sep 28 17:46:21 elsinki systemd-coredump[796494]: [🡕] Process 796483 
(btrfstune) of user 0 dumped core.

                                                   Stack trace of thread 
796483:
                                                   #0 0x0000564b6c2107aa 
n/a (/root/btrfs-progs/btrfstune + 0x4d7aa)
                                                   ELF object binary 
architecture: AMD x86-64
Sep 28 17:46:21 elsinki systemd[1]: systemd-coredump@0-796493-0.service: 
Deactivated successfully.
Sep 28 17:46:21 elsinki systemd[1]: systemd-coredump@0-796493-0.service: 
Consumed 4.248s CPU time.


On 28/9/2023 1:50 π.μ., Qu Wenruo wrote:
> [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;