Re: [PATCH] btrfs-progs: change load_device_info to chunk tree search solution

Re: [PATCH] btrfs-progs: change load_device_info to chunk tree search solution

[hmsjwzb]

Hi Qu,

Thanks for your coaching. I have updated the commit message.


Thanks,

Flint

在 2022/12/12 7:20, hmsjwzb 写道:
> From: "Flint.Wang" <hmsjwzb@zoho.com>
>
> btrfs/249 fails due to unexpected number of devices
>
> [How to reproduce]
> mkfs.btrfs -f -d raid1 -m raid1 /dev/sdb /dev/sdc
> btrfstune -S 1 /dev/sdb
> wipefs -a /dev/sdb
> mount -o degrade /dev/sdc /mnt/scratch
> btrfs device add -f /dev/sdd /mnt/scratch
> btrfs filesystem usage /mnt/scratch
>
> [Root cause]
> ioctl call BTRFS_IOC_FS_INFO returns both rw device and seed device.
> It will cause device number mismatch issues.
>
> [How to Fix]
> This patch introduce function load_devid which only return the information
> of rw devices.
>
> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
> ---
>   cmds/filesystem-usage.c | 81 +++++++++++++++++++++++++++++++++++------
>   1 file changed, 70 insertions(+), 11 deletions(-)
>
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index 5810324f..887fe034 100644
> --- a/cmds/filesystem-usage.c
> +++ b/cmds/filesystem-usage.c
> @@ -27,6 +27,7 @@
>   #include <fcntl.h>
>   #include <dirent.h>
>   #include <limits.h>
> +#include <uuid/uuid.h>
>   #include "kernel-lib/sizes.h"
>   #include "kernel-shared/ctree.h"
>   #include "kernel-shared/disk-io.h"
> @@ -700,6 +701,65 @@ out:
>   	return ret;
>   }
>   
> +/*
> + * This function searches chunk tree to find rw_devs
> + */
> +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;
> +
> +	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
>    */
> @@ -729,19 +789,18 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>   		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,
> -				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);
> +	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) {
> +			error("device not found\n");
> +		}
>   
> -		if (ret == -ENODEV)
> -			continue;
>   		if (ret) {
>   			error("cannot get info about device devid=%d", i);
>   			goto out;

[PATCH] btrfs-progs: change load_device_info to chunk tree search solution

[hmsjwzb]

From: "Flint.Wang" <hmsjwzb@zoho.com>

btrfs/249 fails due to unexpected number of devices

[How to reproduce]
mkfs.btrfs -f -d raid1 -m raid1 /dev/sdb /dev/sdc
btrfstune -S 1 /dev/sdb
wipefs -a /dev/sdb
mount -o degrade /dev/sdc /mnt/scratch
btrfs device add -f /dev/sdd /mnt/scratch
btrfs filesystem usage /mnt/scratch

[Root cause]
ioctl call BTRFS_IOC_FS_INFO returns both rw device and seed device.
It will cause device number mismatch issues.

[How to Fix]
This patch introduce function load_devid which only return the information
of rw devices.

Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
---
 cmds/filesystem-usage.c | 81 +++++++++++++++++++++++++++++++++++------
 1 file changed, 70 insertions(+), 11 deletions(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index 5810324f..887fe034 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -27,6 +27,7 @@
 #include <fcntl.h>
 #include <dirent.h>
 #include <limits.h>
+#include <uuid/uuid.h>
 #include "kernel-lib/sizes.h"
 #include "kernel-shared/ctree.h"
 #include "kernel-shared/disk-io.h"
@@ -700,6 +701,65 @@ out:
 	return ret;
 }
 
+/*
+ * This function searches chunk tree to find rw_devs
+ */
+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;
+
+	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
  */
@@ -729,19 +789,18 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
 		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,
-				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);
+	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) {
+			error("device not found\n");
+		}
 
-		if (ret == -ENODEV)
-			continue;
 		if (ret) {
 			error("cannot get info about device devid=%d", i);
 			goto out;
-- 
2.37.2

Re: [PATCH] btrfs-progs: change load_device_info to chunk tree search solution

[Qu Wenruo]

On 2022/12/9 17:08, Flint.Wang wrote:
> btrfs/249 failed due to btrfs_ioctl_fs_info() return RW devices for fi_args->num_devices.
> This patch change load_device_info to chunk tree search solution.

The commit message is still far from ideal.

At least you need to explain why the test case failed (in details).

And how your fix is going to change the situation.

> 
> 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;
> +
> +	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);
> +	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);
>   

[PATCH] btrfs-progs: change load_device_info to chunk tree search solution

[Flint.Wang]

btrfs/249 failed due to btrfs_ioctl_fs_info() return RW devices for fi_args->num_devices.
This patch change load_device_info to chunk tree search solution.

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;
+
+	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);
+	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);
 
-- 
2.37.0