[PATCH 0/3] btrfs-progs: fix btrfs/249 failure

[PATCH 0/3] btrfs-progs: fix btrfs/249 failure

[Qu Wenruo]

Test case btrfs/249 fails from its introduction, as the fix is not
merged.

The root cause can be found in the first patch, which also introduced an
extra safe net to properly exclude the missing seed device.

However that safe net can only work with root privilege (as it goes
tree-search ioctl), that is good enough for the test case, but not the
best solution.

Instead a new kernel patch is introduced to expand the ability of dev
info ioctl (in a compatible way).

This patch would utilize that new member (and can fallback to the
tree-search if kernel doesn't support), so with newer kernel, fi-usage
can work without root privilege.

This patchset relies on the kernel patch titled:

  btrfs: ioctl: allow dev info ioctl to return fsid of a device

I really hope this won't cause extra delays, as the test case btrfs/249
is really causing too much hassle than it should.

Qu Wenruo (3):
  btrfs-progs: filesystem-usage: handle missing seed device properly
  btrfs-progs: sync ioctl from kernel
  btrfs-progs: filesystem-usage: use btrfs_ioctl_dev_info_args::fsid to
    determine if a device is seed

 cmds/filesystem-usage.c | 129 ++++++++++++++++++++++++++++++++++++----
 ioctl.h                 |  13 +++-
 libbtrfs/ioctl.h        |  13 +++-
 3 files changed, 141 insertions(+), 14 deletions(-)

-- 
2.39.1

[PATCH 1/3] btrfs-progs: filesystem-usage: handle missing seed device properly

[Qu Wenruo]

[BUG]
Test case btrfs/249 always fails since its introduction, the failure
comes from "btrfs filesystem usage" subcommand, and the error output
looks like this:

  QA output created by 249
  ERROR: unexpected number of devices: 1 >= 1
  ERROR: if seed device is used, try running this command as root
  FAILED: btrfs filesystem usage, ret 1. Check btrfs.ko and btrfs-progs version.
  (see /home/adam/xfstests/results//btrfs/249.full for details)

[CAUSE]
In function load_device_info(), we only allocate enough space for all
*RW* devices, expecting we can rule out all seed devices.

And in that function, we check if a device is a seed by checking its
super block fsid.

So if a seed device is missing (it can be an seed device without any
chunks on it, or a degraded RAID1 as seed), then we can not read the
super block.

In that case, we just assume it's not a seed device, and causing too
many devices than our expectation and cause the above failure.

[FIX]
Instead of unconditionally assume a missing device is not a seed, we add
a new safe net, is_seed_device_tree_search(), to search chunk tree and
determine if that device is a seed or not.

And if we found the device is still a seed, then just skip it as usual.

And since we're here, extract the seed device checking into a dedicated
helper, is_seed_device() for later expansion.

Now the test case btrfs/249 passes as expected:

  btrfs/249        2s
  Ran: btrfs/249
  Passed all 1 tests

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem-usage.c | 119 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 107 insertions(+), 12 deletions(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index 5810324f245e..abde04d715a7 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -700,6 +700,102 @@ out:
 	return ret;
 }
 
+/*
+ * Return 0 if this devid is not a seed device.
+ * Return 1 if this devid is a seed device.
+ * Return <0 if error (IO error or EPERM).
+ *
+ * Since this is done by tree search, it needs root privilege, and
+ * should not be triggered unless we hit a missing device and can not
+ * determine if it's a seed one.
+ */
+static int is_seed_device_tree_search(int fd, u64 devid, const u8 *fsid)
+{
+	struct btrfs_ioctl_search_args args = {0};
+	struct btrfs_ioctl_search_key *sk = &args.key;
+	struct btrfs_ioctl_search_header *sh;
+	struct btrfs_dev_item *dev;
+	unsigned long off = 0;
+	int ret;
+	int err;
+
+	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 = devid;
+	sk->max_offset = devid;
+	sk->max_transid = (u64)-1;
+	sk->nr_items = 1;
+
+	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
+	err = errno;
+	if (err == EPERM)
+		return -err;
+	if (ret < 0) {
+		error("cannot lookup chunk tree info: %m");
+		return ret;
+	}
+	/* No dev item found. */
+	if (sk->nr_items == 0)
+		return -ENOENT;
+
+	sh = (struct btrfs_ioctl_search_header *)(args.buf + off);
+	off += sizeof(*sh);
+
+	dev = (struct btrfs_dev_item *)(args.buf + off);
+	if (memcmp(dev->fsid, fsid, BTRFS_UUID_SIZE) == 0)
+		return 0;
+	return 1;
+}
+
+/*
+ * Return 0 if this devid is not a seed device.
+ * Return 1 if this devid is a seed device.
+ * Return <0 if error (IO error or EPERM).
+ */
+static int is_seed_device(int fd, u64 devid, const u8 *fsid,
+			  const struct btrfs_ioctl_dev_info_args *dev_arg)
+{
+	int ret;
+	u8 found_fsid[BTRFS_UUID_SIZE];
+
+	/*
+	 * A missing device, we can not determing if it's a seed
+	 * device by reading its super block.
+	 * Thus we have to go tree-search to make sure if it's a seed
+	 * device.
+	 */
+	if (!dev_arg->path[0]) {
+		ret = is_seed_device_tree_search(fd, devid, fsid);
+		if (ret < 0) {
+			errno = -ret;
+			warning(
+	"unable to determine if devid %llu is seed using tree search: %m",
+				devid);
+			return ret;
+		}
+		return ret;
+	}
+
+	/*
+	 * This device is not missing, try read its super block to determine its
+	 * fsid.
+	 */
+	ret = dev_to_fsid((const char *)dev_arg->path, found_fsid);
+	if (ret < 0) {
+		errno = -ret;
+		warning(
+	"unable to determine if devid %llu is seed using its super block: %m",
+			devid);
+		return ret;
+	}
+	if (!memcmp(found_fsid, fsid, BTRFS_UUID_SIZE))
+		return 0;
+	return 1;
+}
+
 /*
  *  This function loads the device_info structure and put them in an array
  */
@@ -708,9 +804,7 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
 {
 	int ret, i, ndevs;
 	struct btrfs_ioctl_fs_info_args fi_args;
-	struct btrfs_ioctl_dev_info_args dev_info;
 	struct device_info *info;
-	u8 fsid[BTRFS_UUID_SIZE];
 
 	*devcount_ret = 0;
 	*devinfo_ret = NULL;
@@ -730,6 +824,8 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
 	}
 
 	for (i = 0, ndevs = 0 ; i <= fi_args.max_id ; i++) {
+		struct btrfs_ioctl_dev_info_args dev_info = {0};
+
 		if (ndevs >= fi_args.num_devices) {
 			error("unexpected number of devices: %d >= %llu", ndevs,
 				fi_args.num_devices);
@@ -737,7 +833,6 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
 		"if seed device is used, try running this command as root");
 			goto out;
 		}
-		memset(&dev_info, 0, sizeof(dev_info));
 		ret = get_device_info(fd, i, &dev_info);
 
 		if (ret == -ENODEV)
@@ -747,16 +842,16 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
 			goto out;
 		}
 
-		/*
-		 * Skip seed device by checking device's fsid (requires root).
-		 * And we will skip only if dev_to_fsid is successful and dev
-		 * is a seed device.
-		 * Ignore any other error including -EACCES, which is seen when
-		 * a non-root process calls dev_to_fsid(path)->open(path).
-		 */
-		ret = dev_to_fsid((const char *)dev_info.path, fsid);
-		if (!ret && memcmp(fi_args.fsid, fsid, BTRFS_FSID_SIZE) != 0)
+		ret = is_seed_device(fd, i, fi_args.fsid, &dev_info);
+		/* Skip seed device. */
+		if (ret > 0)
 			continue;
+		if (ret < 0){
+			errno = -ret;
+			warning(
+		"unable to determine if devid %u is seed: %m, assuming not", i);
+			ret = 0;
+		}
 
 		info[ndevs].devid = dev_info.devid;
 		if (!dev_info.path[0]) {
-- 
2.39.1

Re: [PATCH 1/3] btrfs-progs: filesystem-usage: handle missing seed device properly

[David Sterba]

On Sat, Feb 11, 2023 at 07:50:30PM +0800, Qu Wenruo wrote:
> [BUG]
> Test case btrfs/249 always fails since its introduction, the failure
> comes from "btrfs filesystem usage" subcommand, and the error output
> looks like this:
> 
>   QA output created by 249
>   ERROR: unexpected number of devices: 1 >= 1
>   ERROR: if seed device is used, try running this command as root
>   FAILED: btrfs filesystem usage, ret 1. Check btrfs.ko and btrfs-progs version.
>   (see /home/adam/xfstests/results//btrfs/249.full for details)
> 
> [CAUSE]
> In function load_device_info(), we only allocate enough space for all
> *RW* devices, expecting we can rule out all seed devices.
> 
> And in that function, we check if a device is a seed by checking its
> super block fsid.
> 
> So if a seed device is missing (it can be an seed device without any
> chunks on it, or a degraded RAID1 as seed), then we can not read the
> super block.
> 
> In that case, we just assume it's not a seed device, and causing too
> many devices than our expectation and cause the above failure.
> 
> [FIX]
> Instead of unconditionally assume a missing device is not a seed, we add
> a new safe net, is_seed_device_tree_search(), to search chunk tree and
> determine if that device is a seed or not.
> 
> And if we found the device is still a seed, then just skip it as usual.
> 
> And since we're here, extract the seed device checking into a dedicated
> helper, is_seed_device() for later expansion.
> 
> Now the test case btrfs/249 passes as expected:
> 
>   btrfs/249        2s
>   Ran: btrfs/249
>   Passed all 1 tests
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

The patch now conflicts with 32c2e57c65b997 ("btrfs-progs: read fsid
from the sysfs in device_is_seed"). I tried to resolve it but it seems
that there's some overlap. Can you please refresh and resend? Also there
are some coding style issues.

> ---
>  cmds/filesystem-usage.c | 119 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 107 insertions(+), 12 deletions(-)
> 
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index 5810324f245e..abde04d715a7 100644
> --- a/cmds/filesystem-usage.c
> +++ b/cmds/filesystem-usage.c
> @@ -700,6 +700,102 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * Return 0 if this devid is not a seed device.
> + * Return 1 if this devid is a seed device.
> + * Return <0 if error (IO error or EPERM).
> + *
> + * Since this is done by tree search, it needs root privilege, and
> + * should not be triggered unless we hit a missing device and can not
> + * determine if it's a seed one.
> + */
> +static int is_seed_device_tree_search(int fd, u64 devid, const u8 *fsid)
> +{
> +	struct btrfs_ioctl_search_args args = {0};

	{ 0 }

> +	struct btrfs_ioctl_search_key *sk = &args.key;
> +	struct btrfs_ioctl_search_header *sh;
> +	struct btrfs_dev_item *dev;
> +	unsigned long off = 0;
> +	int ret;
> +	int err;

	err not needed
> +
> +	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 = devid;
> +	sk->max_offset = devid;
> +	sk->max_transid = (u64)-1;
> +	sk->nr_items = 1;
> +
> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
> +	err = errno;

You need to check ret < 0 first then the errno is valid and you can use
it directly without the temporary variable.

> +	if (err == EPERM)
> +		return -err;
> +	if (ret < 0) {
> +		error("cannot lookup chunk tree info: %m");
> +		return ret;
> +	}
> +	/* No dev item found. */
> +	if (sk->nr_items == 0)
> +		return -ENOENT;
> +
> +	sh = (struct btrfs_ioctl_search_header *)(args.buf + off);
> +	off += sizeof(*sh);
> +
> +	dev = (struct btrfs_dev_item *)(args.buf + off);
> +	if (memcmp(dev->fsid, fsid, BTRFS_UUID_SIZE) == 0)
> +		return 0;
> +	return 1;
> +}
> +
> +/*
> + * Return 0 if this devid is not a seed device.
> + * Return 1 if this devid is a seed device.
> + * Return <0 if error (IO error or EPERM).
> + */
> +static int is_seed_device(int fd, u64 devid, const u8 *fsid,
> +			  const struct btrfs_ioctl_dev_info_args *dev_arg)
> +{
> +	int ret;
> +	u8 found_fsid[BTRFS_UUID_SIZE];
> +
> +	/*
> +	 * A missing device, we can not determing if it's a seed
> +	 * device by reading its super block.
> +	 * Thus we have to go tree-search to make sure if it's a seed
> +	 * device.
> +	 */
> +	if (!dev_arg->path[0]) {
> +		ret = is_seed_device_tree_search(fd, devid, fsid);
> +		if (ret < 0) {
> +			errno = -ret;
> +			warning(
> +	"unable to determine if devid %llu is seed using tree search: %m",
> +				devid);
> +			return ret;
> +		}
> +		return ret;
> +	}
> +
> +	/*
> +	 * This device is not missing, try read its super block to determine its
> +	 * fsid.
> +	 */
> +	ret = dev_to_fsid((const char *)dev_arg->path, found_fsid);
> +	if (ret < 0) {
> +		errno = -ret;
> +		warning(
> +	"unable to determine if devid %llu is seed using its super block: %m",
> +			devid);
> +		return ret;
> +	}
> +	if (!memcmp(found_fsid, fsid, BTRFS_UUID_SIZE))

Please use explicit == 0 or != 0

> +		return 0;
> +	return 1;
> +}
> +
>  /*
>   *  This function loads the device_info structure and put them in an array
>   */
> @@ -708,9 +804,7 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>  {
>  	int ret, i, ndevs;
>  	struct btrfs_ioctl_fs_info_args fi_args;
> -	struct btrfs_ioctl_dev_info_args dev_info;
>  	struct device_info *info;
> -	u8 fsid[BTRFS_UUID_SIZE];
>  
>  	*devcount_ret = 0;
>  	*devinfo_ret = NULL;
> @@ -730,6 +824,8 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>  	}
>  
>  	for (i = 0, ndevs = 0 ; i <= fi_args.max_id ; i++) {
> +		struct btrfs_ioctl_dev_info_args dev_info = {0};

		{ 0 }
> +
>  		if (ndevs >= fi_args.num_devices) {
>  			error("unexpected number of devices: %d >= %llu", ndevs,
>  				fi_args.num_devices);
> @@ -737,7 +833,6 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>  		"if seed device is used, try running this command as root");
>  			goto out;
>  		}
> -		memset(&dev_info, 0, sizeof(dev_info));
>  		ret = get_device_info(fd, i, &dev_info);
>  
>  		if (ret == -ENODEV)
> @@ -747,16 +842,16 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>  			goto out;
>  		}
>  
> -		/*
> -		 * Skip seed device by checking device's fsid (requires root).
> -		 * And we will skip only if dev_to_fsid is successful and dev
> -		 * is a seed device.
> -		 * Ignore any other error including -EACCES, which is seen when
> -		 * a non-root process calls dev_to_fsid(path)->open(path).
> -		 */
> -		ret = dev_to_fsid((const char *)dev_info.path, fsid);
> -		if (!ret && memcmp(fi_args.fsid, fsid, BTRFS_FSID_SIZE) != 0)
> +		ret = is_seed_device(fd, i, fi_args.fsid, &dev_info);
> +		/* Skip seed device. */
> +		if (ret > 0)
>  			continue;
> +		if (ret < 0){
> +			errno = -ret;
> +			warning(
> +		"unable to determine if devid %u is seed: %m, assuming not", i);

Strange that 'i' is printed here as it's int but devids are u64, so the
types don't match.

> +			ret = 0;
> +		}
>  
>  		info[ndevs].devid = dev_info.devid;
>  		if (!dev_info.path[0]) {
> -- 
> 2.39.1

Re: [PATCH 1/3] btrfs-progs: filesystem-usage: handle missing seed device properly

[Qu Wenruo]

Hi David,

Mind to drop the series?

Although the series is to solve the btrfs/249 failure, Anand's patch is 
already merged.

Furthermore the situation of btrfs/249 is very niche, it involves RAID1 
as seed, which should be rare or even non-exist in real world.

Thus I don't think the fallback for such a niche usage is really worthy 
anymore.

Thanks,
Qu

On 2023/3/16 23:25, David Sterba wrote:
> On Sat, Feb 11, 2023 at 07:50:30PM +0800, Qu Wenruo wrote:
>> [BUG]
>> Test case btrfs/249 always fails since its introduction, the failure
>> comes from "btrfs filesystem usage" subcommand, and the error output
>> looks like this:
>>
>>    QA output created by 249
>>    ERROR: unexpected number of devices: 1 >= 1
>>    ERROR: if seed device is used, try running this command as root
>>    FAILED: btrfs filesystem usage, ret 1. Check btrfs.ko and btrfs-progs version.
>>    (see /home/adam/xfstests/results//btrfs/249.full for details)
>>
>> [CAUSE]
>> In function load_device_info(), we only allocate enough space for all
>> *RW* devices, expecting we can rule out all seed devices.
>>
>> And in that function, we check if a device is a seed by checking its
>> super block fsid.
>>
>> So if a seed device is missing (it can be an seed device without any
>> chunks on it, or a degraded RAID1 as seed), then we can not read the
>> super block.
>>
>> In that case, we just assume it's not a seed device, and causing too
>> many devices than our expectation and cause the above failure.
>>
>> [FIX]
>> Instead of unconditionally assume a missing device is not a seed, we add
>> a new safe net, is_seed_device_tree_search(), to search chunk tree and
>> determine if that device is a seed or not.
>>
>> And if we found the device is still a seed, then just skip it as usual.
>>
>> And since we're here, extract the seed device checking into a dedicated
>> helper, is_seed_device() for later expansion.
>>
>> Now the test case btrfs/249 passes as expected:
>>
>>    btrfs/249        2s
>>    Ran: btrfs/249
>>    Passed all 1 tests
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> The patch now conflicts with 32c2e57c65b997 ("btrfs-progs: read fsid
> from the sysfs in device_is_seed"). I tried to resolve it but it seems
> that there's some overlap. Can you please refresh and resend? Also there
> are some coding style issues.
> 
>> ---
>>   cmds/filesystem-usage.c | 119 ++++++++++++++++++++++++++++++++++++----
>>   1 file changed, 107 insertions(+), 12 deletions(-)
>>
>> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
>> index 5810324f245e..abde04d715a7 100644
>> --- a/cmds/filesystem-usage.c
>> +++ b/cmds/filesystem-usage.c
>> @@ -700,6 +700,102 @@ out:
>>   	return ret;
>>   }
>>   
>> +/*
>> + * Return 0 if this devid is not a seed device.
>> + * Return 1 if this devid is a seed device.
>> + * Return <0 if error (IO error or EPERM).
>> + *
>> + * Since this is done by tree search, it needs root privilege, and
>> + * should not be triggered unless we hit a missing device and can not
>> + * determine if it's a seed one.
>> + */
>> +static int is_seed_device_tree_search(int fd, u64 devid, const u8 *fsid)
>> +{
>> +	struct btrfs_ioctl_search_args args = {0};
> 
> 	{ 0 }
> 
>> +	struct btrfs_ioctl_search_key *sk = &args.key;
>> +	struct btrfs_ioctl_search_header *sh;
>> +	struct btrfs_dev_item *dev;
>> +	unsigned long off = 0;
>> +	int ret;
>> +	int err;
> 
> 	err not needed
>> +
>> +	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 = devid;
>> +	sk->max_offset = devid;
>> +	sk->max_transid = (u64)-1;
>> +	sk->nr_items = 1;
>> +
>> +	ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH, &args);
>> +	err = errno;
> 
> You need to check ret < 0 first then the errno is valid and you can use
> it directly without the temporary variable.
> 
>> +	if (err == EPERM)
>> +		return -err;
>> +	if (ret < 0) {
>> +		error("cannot lookup chunk tree info: %m");
>> +		return ret;
>> +	}
>> +	/* No dev item found. */
>> +	if (sk->nr_items == 0)
>> +		return -ENOENT;
>> +
>> +	sh = (struct btrfs_ioctl_search_header *)(args.buf + off);
>> +	off += sizeof(*sh);
>> +
>> +	dev = (struct btrfs_dev_item *)(args.buf + off);
>> +	if (memcmp(dev->fsid, fsid, BTRFS_UUID_SIZE) == 0)
>> +		return 0;
>> +	return 1;
>> +}
>> +
>> +/*
>> + * Return 0 if this devid is not a seed device.
>> + * Return 1 if this devid is a seed device.
>> + * Return <0 if error (IO error or EPERM).
>> + */
>> +static int is_seed_device(int fd, u64 devid, const u8 *fsid,
>> +			  const struct btrfs_ioctl_dev_info_args *dev_arg)
>> +{
>> +	int ret;
>> +	u8 found_fsid[BTRFS_UUID_SIZE];
>> +
>> +	/*
>> +	 * A missing device, we can not determing if it's a seed
>> +	 * device by reading its super block.
>> +	 * Thus we have to go tree-search to make sure if it's a seed
>> +	 * device.
>> +	 */
>> +	if (!dev_arg->path[0]) {
>> +		ret = is_seed_device_tree_search(fd, devid, fsid);
>> +		if (ret < 0) {
>> +			errno = -ret;
>> +			warning(
>> +	"unable to determine if devid %llu is seed using tree search: %m",
>> +				devid);
>> +			return ret;
>> +		}
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * This device is not missing, try read its super block to determine its
>> +	 * fsid.
>> +	 */
>> +	ret = dev_to_fsid((const char *)dev_arg->path, found_fsid);
>> +	if (ret < 0) {
>> +		errno = -ret;
>> +		warning(
>> +	"unable to determine if devid %llu is seed using its super block: %m",
>> +			devid);
>> +		return ret;
>> +	}
>> +	if (!memcmp(found_fsid, fsid, BTRFS_UUID_SIZE))
> 
> Please use explicit == 0 or != 0
> 
>> +		return 0;
>> +	return 1;
>> +}
>> +
>>   /*
>>    *  This function loads the device_info structure and put them in an array
>>    */
>> @@ -708,9 +804,7 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>>   {
>>   	int ret, i, ndevs;
>>   	struct btrfs_ioctl_fs_info_args fi_args;
>> -	struct btrfs_ioctl_dev_info_args dev_info;
>>   	struct device_info *info;
>> -	u8 fsid[BTRFS_UUID_SIZE];
>>   
>>   	*devcount_ret = 0;
>>   	*devinfo_ret = NULL;
>> @@ -730,6 +824,8 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>>   	}
>>   
>>   	for (i = 0, ndevs = 0 ; i <= fi_args.max_id ; i++) {
>> +		struct btrfs_ioctl_dev_info_args dev_info = {0};
> 
> 		{ 0 }
>> +
>>   		if (ndevs >= fi_args.num_devices) {
>>   			error("unexpected number of devices: %d >= %llu", ndevs,
>>   				fi_args.num_devices);
>> @@ -737,7 +833,6 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>>   		"if seed device is used, try running this command as root");
>>   			goto out;
>>   		}
>> -		memset(&dev_info, 0, sizeof(dev_info));
>>   		ret = get_device_info(fd, i, &dev_info);
>>   
>>   		if (ret == -ENODEV)
>> @@ -747,16 +842,16 @@ static int load_device_info(int fd, struct device_info **devinfo_ret,
>>   			goto out;
>>   		}
>>   
>> -		/*
>> -		 * Skip seed device by checking device's fsid (requires root).
>> -		 * And we will skip only if dev_to_fsid is successful and dev
>> -		 * is a seed device.
>> -		 * Ignore any other error including -EACCES, which is seen when
>> -		 * a non-root process calls dev_to_fsid(path)->open(path).
>> -		 */
>> -		ret = dev_to_fsid((const char *)dev_info.path, fsid);
>> -		if (!ret && memcmp(fi_args.fsid, fsid, BTRFS_FSID_SIZE) != 0)
>> +		ret = is_seed_device(fd, i, fi_args.fsid, &dev_info);
>> +		/* Skip seed device. */
>> +		if (ret > 0)
>>   			continue;
>> +		if (ret < 0){
>> +			errno = -ret;
>> +			warning(
>> +		"unable to determine if devid %u is seed: %m, assuming not", i);
> 
> Strange that 'i' is printed here as it's int but devids are u64, so the
> types don't match.
> 
>> +			ret = 0;
>> +		}
>>   
>>   		info[ndevs].devid = dev_info.devid;
>>   		if (!dev_info.path[0]) {
>> -- 
>> 2.39.1

Re: [PATCH 1/3] btrfs-progs: filesystem-usage: handle missing seed device properly

[David Sterba]

On Sat, Mar 18, 2023 at 08:00:18AM +0800, Qu Wenruo wrote:
> Hi David,
> 
> Mind to drop the series?
> 
> Although the series is to solve the btrfs/249 failure, Anand's patch is 
> already merged.
> 
> Furthermore the situation of btrfs/249 is very niche, it involves RAID1 
> as seed, which should be rare or even non-exist in real world.
> 
> Thus I don't think the fallback for such a niche usage is really worthy 
> anymore.

The multi-device seeding should work, though I agree it's a niche use
case. A fallback code should be there either in full or with a warning
that some combination is not supported or could be problematic.

[PATCH 2/3] btrfs-progs: sync ioctl from kernel

[Qu Wenruo]

This sync is mostly for the new member, btrfs_ioctl_dev_info_args::fsid.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 ioctl.h          | 13 ++++++++++++-
 libbtrfs/ioctl.h | 13 ++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/ioctl.h b/ioctl.h
index 1af16db13241..034f38dd702a 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -214,7 +214,18 @@ struct btrfs_ioctl_dev_info_args {
 	__u8 uuid[BTRFS_UUID_SIZE];		/* in/out */
 	__u64 bytes_used;			/* out */
 	__u64 total_bytes;			/* out */
-	__u64 unused[379];			/* pad to 4k */
+	/*
+	 * Optional, out.
+	 *
+	 * Showing the fsid of the device, allowing user space
+	 * to check if this device is a seed one.
+	 *
+	 * Introduced in v6.4, thus user space still needs to
+	 * check if kernel changed this value.
+	 * Older kernel will not touch the values here.
+	 */
+	__u8 fsid[BTRFS_UUID_SIZE];
+	__u64 unused[377];			/* pad to 4k */
 	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
 };
 BUILD_ASSERT(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
diff --git a/libbtrfs/ioctl.h b/libbtrfs/ioctl.h
index f19695e30a63..0b19efde4915 100644
--- a/libbtrfs/ioctl.h
+++ b/libbtrfs/ioctl.h
@@ -215,7 +215,18 @@ struct btrfs_ioctl_dev_info_args {
 	__u8 uuid[BTRFS_UUID_SIZE];		/* in/out */
 	__u64 bytes_used;			/* out */
 	__u64 total_bytes;			/* out */
-	__u64 unused[379];			/* pad to 4k */
+	/*
+	 * Optional, out.
+	 *
+	 * Showing the fsid of the device, allowing user space
+	 * to check if this device is a seed one.
+	 *
+	 * Introduced in v6.4, thus user space still needs to
+	 * check if kernel changed this value.
+	 * Older kernel will not touch the values here.
+	 */
+	__u8 fsid[BTRFS_UUID_SIZE];
+	__u64 unused[377];			/* pad to 4k */
 	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
 };
 BUILD_ASSERT(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
-- 
2.39.1

Re: [PATCH 2/3] btrfs-progs: sync ioctl from kernel

[David Sterba]

On Sat, Feb 11, 2023 at 07:50:31PM +0800, Qu Wenruo wrote:
> This sync is mostly for the new member, btrfs_ioctl_dev_info_args::fsid.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  ioctl.h          | 13 ++++++++++++-
>  libbtrfs/ioctl.h | 13 ++++++++++++-
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/ioctl.h b/ioctl.h
> index 1af16db13241..034f38dd702a 100644
> --- a/ioctl.h
> +++ b/ioctl.h
> @@ -214,7 +214,18 @@ struct btrfs_ioctl_dev_info_args {
>  	__u8 uuid[BTRFS_UUID_SIZE];		/* in/out */
>  	__u64 bytes_used;			/* out */
>  	__u64 total_bytes;			/* out */
> -	__u64 unused[379];			/* pad to 4k */
> +	/*
> +	 * Optional, out.
> +	 *
> +	 * Showing the fsid of the device, allowing user space
> +	 * to check if this device is a seed one.
> +	 *
> +	 * Introduced in v6.4, thus user space still needs to
> +	 * check if kernel changed this value.
> +	 * Older kernel will not touch the values here.
> +	 */
> +	__u8 fsid[BTRFS_UUID_SIZE];
> +	__u64 unused[377];			/* pad to 4k */
>  	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
>  };
>  BUILD_ASSERT(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
> diff --git a/libbtrfs/ioctl.h b/libbtrfs/ioctl.h
> index f19695e30a63..0b19efde4915 100644
> --- a/libbtrfs/ioctl.h
> +++ b/libbtrfs/ioctl.h
> @@ -215,7 +215,18 @@ struct btrfs_ioctl_dev_info_args {
>  	__u8 uuid[BTRFS_UUID_SIZE];		/* in/out */
>  	__u64 bytes_used;			/* out */
>  	__u64 total_bytes;			/* out */
> -	__u64 unused[379];			/* pad to 4k */
> +	/*
> +	 * Optional, out.
> +	 *
> +	 * Showing the fsid of the device, allowing user space
> +	 * to check if this device is a seed one.
> +	 *
> +	 * Introduced in v6.4, thus user space still needs to
> +	 * check if kernel changed this value.
> +	 * Older kernel will not touch the values here.
> +	 */
> +	__u8 fsid[BTRFS_UUID_SIZE];
> +	__u64 unused[377];			/* pad to 4k */

Please don't change libbtrfs interface, it's supposed to be frozen and
not changed anymore. The libbtrfsutil/btrfs.h should be changed instead.

>  	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
>  };
>  BUILD_ASSERT(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
> -- 
> 2.39.1

Re: [PATCH 2/3] btrfs-progs: sync ioctl from kernel

[Qu Wenruo]

On 2023/3/16 23:27, David Sterba wrote:
> On Sat, Feb 11, 2023 at 07:50:31PM +0800, Qu Wenruo wrote:
>> This sync is mostly for the new member, btrfs_ioctl_dev_info_args::fsid.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   ioctl.h          | 13 ++++++++++++-
>>   libbtrfs/ioctl.h | 13 ++++++++++++-
>>   2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/ioctl.h b/ioctl.h
>> index 1af16db13241..034f38dd702a 100644
>> --- a/ioctl.h
>> +++ b/ioctl.h
>> @@ -214,7 +214,18 @@ struct btrfs_ioctl_dev_info_args {
>>   	__u8 uuid[BTRFS_UUID_SIZE];		/* in/out */
>>   	__u64 bytes_used;			/* out */
>>   	__u64 total_bytes;			/* out */
>> -	__u64 unused[379];			/* pad to 4k */
>> +	/*
>> +	 * Optional, out.
>> +	 *
>> +	 * Showing the fsid of the device, allowing user space
>> +	 * to check if this device is a seed one.
>> +	 *
>> +	 * Introduced in v6.4, thus user space still needs to
>> +	 * check if kernel changed this value.
>> +	 * Older kernel will not touch the values here.
>> +	 */
>> +	__u8 fsid[BTRFS_UUID_SIZE];
>> +	__u64 unused[377];			/* pad to 4k */
>>   	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
>>   };
>>   BUILD_ASSERT(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
>> diff --git a/libbtrfs/ioctl.h b/libbtrfs/ioctl.h
>> index f19695e30a63..0b19efde4915 100644
>> --- a/libbtrfs/ioctl.h
>> +++ b/libbtrfs/ioctl.h
>> @@ -215,7 +215,18 @@ struct btrfs_ioctl_dev_info_args {
>>   	__u8 uuid[BTRFS_UUID_SIZE];		/* in/out */
>>   	__u64 bytes_used;			/* out */
>>   	__u64 total_bytes;			/* out */
>> -	__u64 unused[379];			/* pad to 4k */
>> +	/*
>> +	 * Optional, out.
>> +	 *
>> +	 * Showing the fsid of the device, allowing user space
>> +	 * to check if this device is a seed one.
>> +	 *
>> +	 * Introduced in v6.4, thus user space still needs to
>> +	 * check if kernel changed this value.
>> +	 * Older kernel will not touch the values here.
>> +	 */
>> +	__u8 fsid[BTRFS_UUID_SIZE];
>> +	__u64 unused[377];			/* pad to 4k */
> 
> Please don't change libbtrfs interface, it's supposed to be frozen and
> not changed anymore. The libbtrfsutil/btrfs.h should be changed instead.

Got it, I'll only update this patch to sync the headers for libbtrfsutil 
instead.

Thanks,
Qu
> 
>>   	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
>>   };
>>   BUILD_ASSERT(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
>> -- 
>> 2.39.1

[PATCH 3/3] btrfs-progs: filesystem-usage: use btrfs_ioctl_dev_info_args::fsid to determine if a device is seed

[Qu Wenruo]

With the new member, btrfs_ioctl_dev_info_args::fsid, we can easily
determine if a device is seed, no matter if it's missing or not.

And this method is better than all the existing methods by:

- No root privilege requirement compared to tree search

- Can handle missing device compared to super block read

The only problem is, we need to check if the kernel supports the new
member.
If not, we still have to fall back to the existing methods.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem-usage.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index abde04d715a7..f7fd4f90112f 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -759,7 +759,17 @@ static int is_seed_device(int fd, u64 devid, const u8 *fsid,
 			  const struct btrfs_ioctl_dev_info_args *dev_arg)
 {
 	int ret;
-	u8 found_fsid[BTRFS_UUID_SIZE];
+	u8 found_fsid[BTRFS_UUID_SIZE] = {0};
+
+	/*
+	 * Check if @dev_arg contains a valid fsid.
+	 * If so, we can determine if it's seed immediately.
+	 */
+	if (memcmp(dev_arg->fsid, found_fsid, BTRFS_FSID_SIZE)) {
+		if (memcmp(dev_arg->fsid, fsid, BTRFS_FSID_SIZE))
+			return 1;
+		return 0;
+	}
 
 	/*
 	 * A missing device, we can not determing if it's a seed
-- 
2.39.1