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

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

On 2022/8/12 15:26, Flint.Wang wrote:
> Hi Qu,
> Thanks for your proposal. I have implemented the first edition of chunk
> tree search solution.
> But it seems chunk tree search solution need root privilege.

Yes, and that's expected.

For a more elegant and less confusing solution, we either need to expand
the fs_info ioctl to return extra member indicating all devices, or go
sysfs to find out all devices.

Either solution needs certain kernel version, thus the tree search ioctl
based is needed as a fallback anyway.

>
> Btrfs249 failed due to btrfs_ioctl_fs_info() return RW devices for
> fi_args->num_devices. This patch search chunk tree to find no seed device.

I didn't understand the "to find no seed device" line.

Did you mean "to find any seed device" ?

>
> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
> ---
>   cmds/filesystem-usage.c | 89 +++++++++++++++++++++++++++++++++--------
>   cmds/filesystem-usage.h |  5 +++
>   common/utils.c          | 11 +++--
>   common/utils.h          |  2 +-
>   4 files changed, 85 insertions(+), 22 deletions(-)
>
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index 01729e18..ef022ae4 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,59 @@ out:
>   	return ret;
>   }
>
> +static int load_devid_uuid(int fd, struct devid_uuid *pair,
> +			    int ndev, u8 *noseed_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, i, num, noseed_dev = 0, pidx = 0;

This doesn't look like the common style we go.

Normally we go one variable per line.


Another thing is, noseed_* related naming, can we just rename
@noseed_dev to @rw_devs, and @noseed_fsid to @fsid?

> +
> +	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;
> +
> +	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, noseed_fsid)) {
> +			noseed_dev += 1;
> +			pair[pidx].devid = dev_item->devid;

I don't think we need to bother a new struct devid_uuid, can't we just
reuse device_info structure?

Personally speaking, we can completely fill the device_info array inside
the function, we have total size, used bytes, devid, the only thing we
need from the dev_info ioctl is the path.

Which can be later handled by that for () loop inside load_device_info().

Thus to me, we can do the device_info_ptr allocation (and later
enlarge), fill devid/device_size/size members.
Then re-use the for loop just to grab the path.

> +			memcpy(&pair[pidx++].uuid, dev_item->uuid, BTRFS_UUID_SIZE);
> +		}
> +		if (pidx > ndev) {
> +			error("unexpected number of devices: %d >= %d", pidx, ndev);
> +			return -1;
> +		}
> +		sh = (struct btrfs_ioctl_search_header *) dev_item + 1;
> +		dev_item = (struct btrfs_dev_item *) sh + 1;
> +	}
> +
> +	if (ndev != noseed_dev)
> +		error("unexpected number of devices: %d != %d", ndev, noseed_dev);
> +
> +	return 0;
> +}
> +
>   /*
>    *  This function loads the device_info structure and put them in an array
>    */
> @@ -699,6 +753,7 @@ static int load_device_info(int fd, struct device_info **device_info_ptr,
>   	struct btrfs_ioctl_fs_info_args fi_args;
>   	struct btrfs_ioctl_dev_info_args dev_info;
>   	struct device_info *info;
> +	struct devid_uuid *pair;
>   	u8 fsid[BTRFS_UUID_SIZE];
>
>   	*device_info_count = 0;
> @@ -718,19 +773,23 @@ 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;
> -		}
> +	pair = malloc(sizeof(struct devid_uuid) * fi_args.num_devices);
> +	if (!pair) {
> +		error("not enough memory");
> +		return 1;
> +	}
> +
> +	ret = load_devid_uuid(fd, pair, fi_args.num_devices, fi_args.fsid);
> +	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, pair[i].devid, pair[i].uuid, &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,21 +818,17 @@ 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);
>
>   	*device_info_count = fi_args.num_devices;
>   	*device_info_ptr = info;
> +	free(pair);
>
>   	return 0;
>
>   out:
> +	free(pair);
>   	free(info);
>   	return ret;
>   }
> diff --git a/cmds/filesystem-usage.h b/cmds/filesystem-usage.h
> index cab38462..f78b2f2e 100644
> --- a/cmds/filesystem-usage.h
> +++ b/cmds/filesystem-usage.h
> @@ -44,6 +44,11 @@ struct chunk_info {
>   	u64	num_stripes;
>   };
>
> +struct devid_uuid {
> +	__le64 devid;
> +	u8 uuid[BTRFS_UUID_SIZE];
> +};
> +
>   int load_chunk_and_device_info(int fd, struct chunk_info **chunkinfo,
>   		int *chunkcount, struct device_info **devinfo, int *devcount);
>   void print_device_chunks(struct device_info *devinfo,
> diff --git a/common/utils.c b/common/utils.c
> index 1ed5571f..d09177ef 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -284,13 +284,16 @@ void btrfs_format_csum(u16 csum_type, const u8 *data, char *output)
>   	}
>   }
>
> -int get_device_info(int fd, u64 devid,
> +int get_device_info(int fd, u64 devid, u8 *uuid,
>   		struct btrfs_ioctl_dev_info_args *di_args)
>   {
>   	int ret;
>
>   	di_args->devid = devid;
> -	memset(&di_args->uuid, '\0', sizeof(di_args->uuid));
> +	if (!uuid)
> +		memset(&di_args->uuid, '\0', sizeof(di_args->uuid));
> +	else
> +		memcpy(&di_args->uuid, uuid, sizeof(di_args->uuid));

Any reason we need this new @uuid argument?

I know it's to make btrfs_ioctl_dev_info() to have the correct uuid to
do the search, but shouldn't the devid is enough to locate the seed device?

IIRC btrfs shouldn't have conflicting devid, even for seed devices.

Thus I doubt if we need the new @uuid arugment at all.

Thanks,
Qu

>
>   	ret = ioctl(fd, BTRFS_IOC_DEV_INFO, di_args);
>   	return ret < 0 ? -errno : 0;
> @@ -498,7 +501,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>   		 * search_chunk_tree_for_fs_info() will lacks the devid 0
>   		 * so manual probe for it here.
>   		 */
> -		ret = get_device_info(fd, 0, &tmp);
> +		ret = get_device_info(fd, 0, NULL, &tmp);
>   		if (!ret) {
>   			fi_args->num_devices++;
>   			ndevs++;
> @@ -521,7 +524,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>   		memcpy(di_args, &tmp, sizeof(tmp));
>   	for (; last_devid <= fi_args->max_id && ndevs < fi_args->num_devices;
>   	     last_devid++) {
> -		ret = get_device_info(fd, last_devid, &di_args[ndevs]);
> +		ret = get_device_info(fd, last_devid, NULL, &di_args[ndevs]);
>   		if (ret == -ENODEV)
>   			continue;
>   		if (ret)
> diff --git a/common/utils.h b/common/utils.h
> index ea05fe5b..f1bbd807 100644
> --- a/common/utils.h
> +++ b/common/utils.h
> @@ -67,7 +67,7 @@ int ask_user(const char *question);
>   int lookup_path_rootid(int fd, u64 *rootid);
>   int find_mount_fsroot(const char *subvol, const char *subvolid, char **mount);
>   int find_mount_root(const char *path, char **mount_root);
> -int get_device_info(int fd, u64 devid,
> +int get_device_info(int fd, u64 devid, u8 *uuid,
>   		struct btrfs_ioctl_dev_info_args *di_args);
>   int get_df(int fd, struct btrfs_ioctl_space_args **sargs_ret);
>