Re: [PATCH v2] btrfs-progs: chunk tree search solution for btrfs249

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

On 2022/8/15 10:43, Flint.Wang wrote:
> Hi Qu,
>
> Thanks for your comment. I fix the issue you suggest.
> It is much clean now.
>
> Btrfs249 failed due to btrfs_ioctl_fs_info() return RW devices for fi_args->num_devices.
> This patch search chunk tree to find rw devices.
The commit message needs some improvement.

Firstly, the commit message should only explain what the problem is and
how this patch is going to solve it.

Currently there are tons of unnecessary things in the commit message,
including the first paragraph and the changelog.

Those things should go after the "---" line, so at apply time those
unnecessary lines will be ignored by git directly.

Secondly the subject can be more specific.

I would go something like the following:

Subject: [PATCH v2] btrfs-progs: search chunk tree to get correct device
info

[BUG]
Test case btrfs/249 failed with the following output:

   <The failure output>

[CAUSE]
Function btrfs_ioctl_fs_info() only returns the number of RW devices for
its num_devices, not including the seed device(s).

Thus above test case will fail as we have two more seed devices,
exceeding the num_device returned by btrfs_ioctl_fs_info().

[FIX]
Fix the bug by doing a tree-search ioctl to grab all devices from chunk
tree, which includes all RW and seed devices.

---
Changelog and other things should go here.
>
> v2 change:
> 1. code style fix.
> 2. noseed_dev => rw_devs, noseed_fsid => fsid.
> 3. remove redundant structure devid_uuid.
> 4. reuse the dev_info structure.
> 5. remove redundant uuid argument.
>
> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
> ---
>   cmds/filesystem-usage.c | 83 ++++++++++++++++++++++++++++++++---------
>   1 file changed, 66 insertions(+), 17 deletions(-)
>
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index 01729e18..71f0e14c 100644
> --- a/cmds/filesystem-usage.c
> +++ b/cmds/filesystem-usage.c
> @@ -25,6 +25,7 @@
>   #include <getopt.h>
>   #include <fcntl.h>
>   #include <linux/limits.h>
> +#include <uuid/uuid.h>
>
>   #include "common/utils.h"
>   #include "kerncompat.h"
> @@ -689,6 +690,62 @@ out:
>   	return ret;
>   }
>
> +static int load_devid(int fd, struct device_info *info,
> +			    int ndev, u8 *fsid)
> +{
> +	struct btrfs_ioctl_search_args_v2 *args2;
> +	struct btrfs_ioctl_search_key *sk;
> +	struct btrfs_ioctl_search_header *sh;
> +	struct btrfs_dev_item *dev_item;
> +	int args2_size = 1024;
> +	char args2_buf[args2_size];
> +	int ret = 0;
> +	int i = 0;
> +	int num = 0;
> +	int rw_devs = 0;
> +	int idx = 0;
> +
> +	args2 = (struct btrfs_ioctl_search_args_v2 *) args2_buf;
> +	sk = &(args2->key);
> +
> +	sk->tree_id = BTRFS_CHUNK_TREE_OBJECTID;
> +	sk->min_objectid = BTRFS_DEV_ITEMS_OBJECTID;
> +	sk->max_objectid = BTRFS_DEV_ITEMS_OBJECTID;
> +	sk->min_type = BTRFS_DEV_ITEM_KEY;
> +	sk->max_type = BTRFS_DEV_ITEM_KEY;
> +	sk->min_offset = 0;
> +	sk->max_offset = (u64)-1;
> +	sk->min_transid = 0;
> +	sk->max_transid = (u64)-1;
> +	sk->nr_items = -1;
> +	args2->buf_size = args2_size - sizeof(struct btrfs_ioctl_search_args_v2);
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH_V2, args2);
> +	if (ret != 0)
> +	       return -1;

It's better to output an error message and return -errno instead.

-1 is -EINVAL, which is not really helpful to debug what's going wrong.

> +
> +	sh = (struct btrfs_ioctl_search_header *) args2->buf;
> +	num = sk->nr_items;
> +
> +	dev_item = (struct btrfs_dev_item *) (sh + 1);
> +	for (i = 0; i < num; i++) {
> +		if (!uuid_compare(dev_item->fsid, fsid)) {
> +			rw_devs += 1;
> +			info[idx++].devid = dev_item->devid;
> +		}
> +		if (idx > ndev) {
> +			error("unexpected number of devices: %d >= %d", idx, ndev);
> +			return -1;
> +		}
> +		sh = (struct btrfs_ioctl_search_header *) dev_item + 1;
> +		dev_item = (struct btrfs_dev_item *) sh + 1;
> +	}
> +
> +	if (ndev != rw_devs)
> +		error("unexpected number of devices: %d != %d", ndev, rw_devs);
> +
> +	return 0;
> +}
> +
>   /*
>    *  This function loads the device_info structure and put them in an array
>    */
> @@ -718,19 +775,17 @@ static int load_device_info(int fd, struct device_info **device_info_ptr,
>   		return 1;
>   	}
>
> -	for (i = 0, ndevs = 0 ; i <= fi_args.max_id ; i++) {
> -		if (ndevs >= fi_args.num_devices) {
> -			error("unexpected number of devices: %d >= %llu", ndevs,
> -				(unsigned long long)fi_args.num_devices);
> -			error(
> -		"if seed device is used, try running this command as root");
> -			goto out;
> -		}
> +	ret = load_devid(fd, info, fi_args.num_devices, fi_args.fsid);

This will only load the device info for rw devices.

But no seed device will be populated, wouldn't this cause problem
showing missing seed devices?

Or is this always the case for the command from the very beginning?

Thanks,
Qu

> +	if (ret == -1)
> +		goto out;
> +
> +	for (i = 0, ndevs = 0 ; i < fi_args.num_devices ; i++) {
>   		memset(&dev_info, 0, sizeof(dev_info));
> -		ret = get_device_info(fd, i, &dev_info);
> +		ret = get_device_info(fd, info[i].devid, &dev_info);
>
> -		if (ret == -ENODEV)
> -			continue;
> +		if (ret == -ENODEV) {
> +			error("device not found\n");
> +		}
>   		if (ret) {
>   			error("cannot get info about device devid=%d", i);
>   			goto out;
> @@ -759,12 +814,6 @@ static int load_device_info(int fd, struct device_info **device_info_ptr,
>   		++ndevs;
>   	}
>
> -	if (ndevs != fi_args.num_devices) {
> -		error("unexpected number of devices: %d != %llu", ndevs,
> -				(unsigned long long)fi_args.num_devices);
> -		goto out;
> -	}
> -
>   	qsort(info, fi_args.num_devices,
>   		sizeof(struct device_info), cmp_device_info);
>