[PATCH v2 0/2] provide fsid in sysfs devinfo

[PATCH v2 0/2] provide fsid in sysfs devinfo

[Anand Jain]

v2:
Fix sysfs_emit conversion at the two non-sysfs show functions in patch1.

btrfs-progs tries to read the fsid from the super-block for a missing
device and, it fails. It needs to find out if the device is a seed
device. It does it by comparing the device's fsid with the fsid of the
mounted filesystem. To help this scenario introduce a new sysfs file to
read the fsid from the kernel.
     /sys/fs/btrfs/<fsid>/devinfo/<devid>/fsid

Patch 1 is a cleanup converts scnprtin()f and snprintf() to sysfs_emit()
Patch 2 introduces the new sysfs interface as above

The other implementation choice is to add another parameter to the
struct btrfs_ioctl_dev_info_args and use BTRFS_IOC_DEV_INFO ioctl. But
then backward kernel compatibility with the newer btrfs-progs is more
complicated. If needed, we can add that too.

Related btrfs-progs patches:
  btrfs-progs: prepare helper device_is_seed
  btrfs-progs: read fsid from the sysfs in device_is_seed

Anand Jain (2):
  btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit
  btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

 fs/btrfs/sysfs.c | 106 +++++++++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 49 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit

[Anand Jain]

commit 2efc459d06f1 (sysfs: Add sysfs_emit and sysfs_emit_at to format
sysfs out) merged in 5.10 introduced two new functions sysfs_emit() and
sysfs_emit_at() which are aware of the PAGE_SIZE max_limit of the buf.

Use the above two new functions instead of scnprintf() and snprintf()
in various sysfs show().

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reported-by: kernel test robot <oliver.sang@intel.com>
---
 fs/btrfs/sysfs.c | 95 +++++++++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 49 deletions(-)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 25a6f587852b..28ff7fce1ac5 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -177,7 +177,7 @@ static ssize_t btrfs_feature_attr_show(struct kobject *kobj,
 	} else
 		val = can_modify_feature(fa);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+	return sysfs_emit(buf, "%d\n", val);
 }
 
 static ssize_t btrfs_feature_attr_store(struct kobject *kobj,
@@ -330,7 +330,7 @@ static const struct attribute_group btrfs_feature_attr_group = {
 static ssize_t rmdir_subvol_show(struct kobject *kobj,
 				 struct kobj_attribute *ka, char *buf)
 {
-	return scnprintf(buf, PAGE_SIZE, "0\n");
+	return sysfs_emit(buf, "0\n");
 }
 BTRFS_ATTR(static_feature, rmdir_subvol, rmdir_subvol_show);
 
@@ -345,12 +345,12 @@ static ssize_t supported_checksums_show(struct kobject *kobj,
 		 * This "trick" only works as long as 'enum btrfs_csum_type' has
 		 * no holes in it
 		 */
-		ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
-				(i == 0 ? "" : " "), btrfs_super_csum_name(i));
+		ret += sysfs_emit_at(buf, ret, "%s%s", (i == 0 ? "" : " "),
+				     btrfs_super_csum_name(i));
 
 	}
 
-	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
+	ret += sysfs_emit_at(buf, ret, "\n");
 	return ret;
 }
 BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
@@ -358,7 +358,7 @@ BTRFS_ATTR(static_feature, supported_checksums, supported_checksums_show);
 static ssize_t send_stream_version_show(struct kobject *kobj,
 					struct kobj_attribute *ka, char *buf)
 {
-	return snprintf(buf, PAGE_SIZE, "%d\n", BTRFS_SEND_STREAM_VERSION);
+	return sysfs_emit(buf, "%d\n", BTRFS_SEND_STREAM_VERSION);
 }
 BTRFS_ATTR(static_feature, send_stream_version, send_stream_version_show);
 
@@ -378,9 +378,9 @@ static ssize_t supported_rescue_options_show(struct kobject *kobj,
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(rescue_opts); i++)
-		ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
-				 (i ? " " : ""), rescue_opts[i]);
-	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
+		ret += sysfs_emit_at(buf, ret, "%s%s", (i ? " " : ""),
+				     rescue_opts[i]);
+	ret += sysfs_emit_at(buf, ret, "\n");
 	return ret;
 }
 BTRFS_ATTR(static_feature, supported_rescue_options,
@@ -394,10 +394,10 @@ static ssize_t supported_sectorsizes_show(struct kobject *kobj,
 
 	/* 4K sector size is also supported with 64K page size */
 	if (PAGE_SIZE == SZ_64K)
-		ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%u ", SZ_4K);
+		ret += sysfs_emit_at(buf, ret, "%u ", SZ_4K);
 
 	/* Only sectorsize == PAGE_SIZE is now supported */
-	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%lu\n", PAGE_SIZE);
+	ret += sysfs_emit_at(buf, ret, "%lu\n", PAGE_SIZE);
 
 	return ret;
 }
@@ -437,7 +437,7 @@ static ssize_t btrfs_discardable_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%lld\n",
+	return sysfs_emit(buf, "%lld\n",
 			atomic64_read(&fs_info->discard_ctl.discardable_bytes));
 }
 BTRFS_ATTR(discard, discardable_bytes, btrfs_discardable_bytes_show);
@@ -448,7 +448,7 @@ static ssize_t btrfs_discardable_extents_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n",
+	return sysfs_emit(buf, "%d\n",
 			atomic_read(&fs_info->discard_ctl.discardable_extents));
 }
 BTRFS_ATTR(discard, discardable_extents, btrfs_discardable_extents_show);
@@ -459,8 +459,8 @@ static ssize_t btrfs_discard_bitmap_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%llu\n",
-			fs_info->discard_ctl.discard_bitmap_bytes);
+	return sysfs_emit(buf, "%llu\n",
+			  fs_info->discard_ctl.discard_bitmap_bytes);
 }
 BTRFS_ATTR(discard, discard_bitmap_bytes, btrfs_discard_bitmap_bytes_show);
 
@@ -470,7 +470,7 @@ static ssize_t btrfs_discard_bytes_saved_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%lld\n",
+	return sysfs_emit(buf, "%lld\n",
 		atomic64_read(&fs_info->discard_ctl.discard_bytes_saved));
 }
 BTRFS_ATTR(discard, discard_bytes_saved, btrfs_discard_bytes_saved_show);
@@ -481,8 +481,8 @@ static ssize_t btrfs_discard_extent_bytes_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%llu\n",
-			fs_info->discard_ctl.discard_extent_bytes);
+	return sysfs_emit(buf, "%llu\n",
+			  fs_info->discard_ctl.discard_extent_bytes);
 }
 BTRFS_ATTR(discard, discard_extent_bytes, btrfs_discard_extent_bytes_show);
 
@@ -492,8 +492,8 @@ static ssize_t btrfs_discard_iops_limit_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n",
-			READ_ONCE(fs_info->discard_ctl.iops_limit));
+	return sysfs_emit(buf, "%u\n",
+			  READ_ONCE(fs_info->discard_ctl.iops_limit));
 }
 
 static ssize_t btrfs_discard_iops_limit_store(struct kobject *kobj,
@@ -523,8 +523,8 @@ static ssize_t btrfs_discard_kbps_limit_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n",
-			READ_ONCE(fs_info->discard_ctl.kbps_limit));
+	return sysfs_emit(buf, "%u\n",
+			  READ_ONCE(fs_info->discard_ctl.kbps_limit));
 }
 
 static ssize_t btrfs_discard_kbps_limit_store(struct kobject *kobj,
@@ -553,8 +553,8 @@ static ssize_t btrfs_discard_max_discard_size_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = discard_to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%llu\n",
-			READ_ONCE(fs_info->discard_ctl.max_discard_size));
+	return sysfs_emit(buf, "%llu\n",
+			  READ_ONCE(fs_info->discard_ctl.max_discard_size));
 }
 
 static ssize_t btrfs_discard_max_discard_size_store(struct kobject *kobj,
@@ -627,7 +627,7 @@ static ssize_t btrfs_show_u64(u64 *value_ptr, spinlock_t *lock, char *buf)
 	val = *value_ptr;
 	if (lock)
 		spin_unlock(lock);
-	return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
+	return sysfs_emit(buf, "%llu\n", val);
 }
 
 static ssize_t global_rsv_size_show(struct kobject *kobj,
@@ -673,7 +673,7 @@ static ssize_t raid_bytes_show(struct kobject *kobj,
 			val += block_group->used;
 	}
 	up_read(&sinfo->groups_sem);
-	return scnprintf(buf, PAGE_SIZE, "%llu\n", val);
+	return sysfs_emit(buf, "%llu\n", val);
 }
 
 /*
@@ -771,7 +771,7 @@ static ssize_t btrfs_label_show(struct kobject *kobj,
 	ssize_t ret;
 
 	spin_lock(&fs_info->super_lock);
-	ret = scnprintf(buf, PAGE_SIZE, label[0] ? "%s\n" : "%s", label);
+	ret = sysfs_emit(buf, label[0] ? "%s\n" : "%s", label);
 	spin_unlock(&fs_info->super_lock);
 
 	return ret;
@@ -819,7 +819,7 @@ static ssize_t btrfs_nodesize_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->nodesize);
+	return sysfs_emit(buf, "%u\n", fs_info->super_copy->nodesize);
 }
 
 BTRFS_ATTR(, nodesize, btrfs_nodesize_show);
@@ -829,8 +829,7 @@ static ssize_t btrfs_sectorsize_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n",
-			 fs_info->super_copy->sectorsize);
+	return sysfs_emit(buf, "%u\n", fs_info->super_copy->sectorsize);
 }
 
 BTRFS_ATTR(, sectorsize, btrfs_sectorsize_show);
@@ -840,7 +839,7 @@ static ssize_t btrfs_clone_alignment_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%u\n", fs_info->super_copy->sectorsize);
+	return sysfs_emit(buf, "%u\n", fs_info->super_copy->sectorsize);
 }
 
 BTRFS_ATTR(, clone_alignment, btrfs_clone_alignment_show);
@@ -852,7 +851,7 @@ static ssize_t quota_override_show(struct kobject *kobj,
 	int quota_override;
 
 	quota_override = test_bit(BTRFS_FS_QUOTA_OVERRIDE, &fs_info->flags);
-	return scnprintf(buf, PAGE_SIZE, "%d\n", quota_override);
+	return sysfs_emit(buf, "%d\n", quota_override);
 }
 
 static ssize_t quota_override_store(struct kobject *kobj,
@@ -890,8 +889,7 @@ static ssize_t btrfs_metadata_uuid_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%pU\n",
-			fs_info->fs_devices->metadata_uuid);
+	return sysfs_emit(buf, "%pU\n", fs_info->fs_devices->metadata_uuid);
 }
 
 BTRFS_ATTR(, metadata_uuid, btrfs_metadata_uuid_show);
@@ -902,9 +900,9 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	u16 csum_type = btrfs_super_csum_type(fs_info->super_copy);
 
-	return scnprintf(buf, PAGE_SIZE, "%s (%s)\n",
-			btrfs_super_csum_name(csum_type),
-			crypto_shash_driver_name(fs_info->csum_shash));
+	return sysfs_emit(buf, "%s (%s)\n",
+			  btrfs_super_csum_name(csum_type),
+			  crypto_shash_driver_name(fs_info->csum_shash));
 }
 
 BTRFS_ATTR(, checksum, btrfs_checksum_show);
@@ -941,7 +939,7 @@ static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
 			str = "UNKNOWN\n";
 			break;
 	}
-	return scnprintf(buf, PAGE_SIZE, "%s", str);
+	return sysfs_emit(buf, "%s", str);
 }
 BTRFS_ATTR(, exclusive_operation, btrfs_exclusive_operation_show);
 
@@ -950,7 +948,7 @@ static ssize_t btrfs_generation_show(struct kobject *kobj,
 {
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%llu\n", fs_info->generation);
+	return sysfs_emit(buf, "%llu\n", fs_info->generation);
 }
 BTRFS_ATTR(, generation, btrfs_generation_show);
 
@@ -1028,8 +1026,8 @@ static ssize_t btrfs_bg_reclaim_threshold_show(struct kobject *kobj,
 	struct btrfs_fs_info *fs_info = to_fs_info(kobj);
 	ssize_t ret;
 
-	ret = scnprintf(buf, PAGE_SIZE, "%d\n",
-			READ_ONCE(fs_info->bg_reclaim_threshold));
+	ret = sysfs_emit(buf, "%d\n",
+			 READ_ONCE(fs_info->bg_reclaim_threshold));
 
 	return ret;
 }
@@ -1471,7 +1469,7 @@ static ssize_t btrfs_devinfo_in_fs_metadata_show(struct kobject *kobj,
 
 	val = !!test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+	return sysfs_emit(buf, "%d\n", val);
 }
 BTRFS_ATTR(devid, in_fs_metadata, btrfs_devinfo_in_fs_metadata_show);
 
@@ -1484,7 +1482,7 @@ static ssize_t btrfs_devinfo_missing_show(struct kobject *kobj,
 
 	val = !!test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+	return sysfs_emit(buf, "%d\n", val);
 }
 BTRFS_ATTR(devid, missing, btrfs_devinfo_missing_show);
 
@@ -1498,7 +1496,7 @@ static ssize_t btrfs_devinfo_replace_target_show(struct kobject *kobj,
 
 	val = !!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+	return sysfs_emit(buf, "%d\n", val);
 }
 BTRFS_ATTR(devid, replace_target, btrfs_devinfo_replace_target_show);
 
@@ -1509,8 +1507,7 @@ static ssize_t btrfs_devinfo_scrub_speed_max_show(struct kobject *kobj,
 	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
 						   devid_kobj);
 
-	return scnprintf(buf, PAGE_SIZE, "%llu\n",
-			 READ_ONCE(device->scrub_speed_max));
+	return sysfs_emit(buf, "%llu\n", READ_ONCE(device->scrub_speed_max));
 }
 
 static ssize_t btrfs_devinfo_scrub_speed_max_store(struct kobject *kobj,
@@ -1538,7 +1535,7 @@ static ssize_t btrfs_devinfo_writeable_show(struct kobject *kobj,
 
 	val = !!test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state);
 
-	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+	return sysfs_emit(buf, "%d\n", val);
 }
 BTRFS_ATTR(devid, writeable, btrfs_devinfo_writeable_show);
 
@@ -1549,14 +1546,14 @@ static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
 						   devid_kobj);
 
 	if (!device->dev_stats_valid)
-		return scnprintf(buf, PAGE_SIZE, "invalid\n");
+		return sysfs_emit(buf, "invalid\n");
 
 	/*
 	 * Print all at once so we get a snapshot of all values from the same
 	 * time. Keep them in sync and in order of definition of
 	 * btrfs_dev_stat_values.
 	 */
-	return scnprintf(buf, PAGE_SIZE,
+	return sysfs_emit(buf,
 		"write_errs %d\n"
 		"read_errs %d\n"
 		"flush_errs %d\n"
-- 
2.31.1

[PATCH v2 2/2] btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

[Anand Jain]

In the case of the seed device, the fsid can be different from the mounted
sprout fsid.  The userland has to read the device superblock to know the
fsid but, that idea fails if the device is missing. So add a sysfs
interface devinfo/<devid>/fsid to show the fsid of the device.

For example:
 $ cd /sys/fs/btrfs/b10b02a5-f9de-4276-b9e8-2bfd09a578a8

 $ cat devinfo/1/fsid
 c44d771f-639d-4df3-99ec-5bc7ad2af93b
 $ cat  devinfo/3/fsid
 b10b02a5-f9de-4276-b9e8-2bfd09a578a8

Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/sysfs.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 28ff7fce1ac5..591d49deb552 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1539,6 +1539,16 @@ static ssize_t btrfs_devinfo_writeable_show(struct kobject *kobj,
 }
 BTRFS_ATTR(devid, writeable, btrfs_devinfo_writeable_show);
 
+static ssize_t btrfs_devinfo_fsid_show(struct kobject *kobj,
+				       struct kobj_attribute *a, char *buf)
+{
+	struct btrfs_device *device = container_of(kobj, struct btrfs_device,
+						   devid_kobj);
+
+	return sysfs_emit(buf, "%pU\n", device->fs_devices->fsid);
+}
+BTRFS_ATTR(devid, fsid, btrfs_devinfo_fsid_show);
+
 static ssize_t btrfs_devinfo_error_stats_show(struct kobject *kobj,
 		struct kobj_attribute *a, char *buf)
 {
@@ -1579,6 +1589,7 @@ static struct attribute *devid_attrs[] = {
 	BTRFS_ATTR_PTR(devid, replace_target),
 	BTRFS_ATTR_PTR(devid, scrub_speed_max),
 	BTRFS_ATTR_PTR(devid, writeable),
+	BTRFS_ATTR_PTR(devid, fsid),
 	NULL
 };
 ATTRIBUTE_GROUPS(devid);
-- 
2.31.1

Re: [PATCH v2 1/2] btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit

[David Sterba]

On Thu, Oct 21, 2021 at 11:31:16PM +0800, Anand Jain wrote:
> commit 2efc459d06f1 (sysfs: Add sysfs_emit and sysfs_emit_at to format
> sysfs out)

Please use the common formatting which is 2efc459d06f1 ("sysfs: Add
sysfs_emit and sysfs_emit_at to format sysfs output") in this case.

> merged in 5.10 introduced two new functions sysfs_emit() and
> sysfs_emit_at() which are aware of the PAGE_SIZE max_limit of the buf.
> 
> Use the above two new functions instead of scnprintf() and snprintf()
> in various sysfs show().
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reported-by: kernel test robot <oliver.sang@intel.com>

Reported-by should be used only for when the patch is fixing something
based on that report.

I've applied the patch over the local version 1 but I don't see any
fix related to the the crash report, is that correct?

Re: [PATCH v2 1/2] btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit

[David Sterba]

On Thu, Oct 21, 2021 at 06:19:39PM +0200, David Sterba wrote:
> On Thu, Oct 21, 2021 at 11:31:16PM +0800, Anand Jain wrote:
> I've applied the patch over the local version 1 but I don't see any
> fix related to the the crash report, is that correct?

I saw the reply, the warning was caused by the removed hunks, so it's
sorted now. Patch added to misc-next, thanks.

Re: [PATCH v2 1/2] btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit

[Anand Jain]

On 22/10/2021 00:19, David Sterba wrote:
> On Thu, Oct 21, 2021 at 11:31:16PM +0800, Anand Jain wrote:
>> commit 2efc459d06f1 (sysfs: Add sysfs_emit and sysfs_emit_at to format
>> sysfs out)
> 
> Please use the common formatting which is 2efc459d06f1 ("sysfs: Add
> sysfs_emit and sysfs_emit_at to format sysfs output") in this case.
> 

  Err. My bad. You told me that before. But, for some reason the related
  config isn't working. So I am doing it manually. I am going to try
  the config again.


>> merged in 5.10 introduced two new functions sysfs_emit() and
>> sysfs_emit_at() which are aware of the PAGE_SIZE max_limit of the buf.
>>
>> Use the above two new functions instead of scnprintf() and snprintf()
>> in various sysfs show().
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> Reported-by: kernel test robot <oliver.sang@intel.com>
> 
> Reported-by should be used only for when the patch is fixing something
> based on that report.

  Right. Ok.

> 
> I've applied the patch over the local version 1 but I don't see any
> fix related to the the crash report, is that correct?

  Right. There is no crash report for the v1.

Thanks, Anand


> 

Re: [PATCH v2 1/2] btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit

[Anand Jain]

On 22/10/2021 00:25, David Sterba wrote:
> On Thu, Oct 21, 2021 at 06:19:39PM +0200, David Sterba wrote:
>> On Thu, Oct 21, 2021 at 11:31:16PM +0800, Anand Jain wrote:
>> I've applied the patch over the local version 1 but I don't see any
>> fix related to the the crash report, is that correct?
> 
> I saw the reply, the warning was caused by the removed hunks, so it's
> sorted now. Patch added to misc-next, thanks.
> 

Oh. Ok. Thanks.

Re: [PATCH v2 0/2] provide fsid in sysfs devinfo

[Anand Jain]

David,

  And this one could also merge?

Thanks, Anand


On 21/10/21 11:31 pm, Anand Jain wrote:
> v2:
> Fix sysfs_emit conversion at the two non-sysfs show functions in patch1.
> 
> btrfs-progs tries to read the fsid from the super-block for a missing
> device and, it fails. It needs to find out if the device is a seed
> device. It does it by comparing the device's fsid with the fsid of the
> mounted filesystem. To help this scenario introduce a new sysfs file to
> read the fsid from the kernel.
>       /sys/fs/btrfs/<fsid>/devinfo/<devid>/fsid
> 
> Patch 1 is a cleanup converts scnprtin()f and snprintf() to sysfs_emit()
> Patch 2 introduces the new sysfs interface as above
> 
> The other implementation choice is to add another parameter to the
> struct btrfs_ioctl_dev_info_args and use BTRFS_IOC_DEV_INFO ioctl. But
> then backward kernel compatibility with the newer btrfs-progs is more
> complicated. If needed, we can add that too.
> 
> Related btrfs-progs patches:
>    btrfs-progs: prepare helper device_is_seed
>    btrfs-progs: read fsid from the sysfs in device_is_seed
> 
> Anand Jain (2):
>    btrfs: sysfs convert scnprintf and snprintf to use sysfs_emit
>    btrfs: sysfs add devinfo/fsid to retrieve fsid from the device
> 
>   fs/btrfs/sysfs.c | 106 +++++++++++++++++++++++++----------------------
>   1 file changed, 57 insertions(+), 49 deletions(-)
> 

Re: [PATCH v2 2/2] btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

[David Sterba]

On Thu, Oct 21, 2021 at 11:31:17PM +0800, Anand Jain wrote:
> In the case of the seed device, the fsid can be different from the mounted
> sprout fsid.  The userland has to read the device superblock to know the
> fsid but, that idea fails if the device is missing. So add a sysfs
> interface devinfo/<devid>/fsid to show the fsid of the device.
> 
> For example:
>  $ cd /sys/fs/btrfs/b10b02a5-f9de-4276-b9e8-2bfd09a578a8
> 
>  $ cat devinfo/1/fsid
>  c44d771f-639d-4df3-99ec-5bc7ad2af93b
>  $ cat  devinfo/3/fsid
>  b10b02a5-f9de-4276-b9e8-2bfd09a578a8

From user perspective, it's another fsid, one is in the path, so I'm
wondering if it should be named like read_fsid or sprout_fsid or if the
seed/sprout information should be put into another directory completely.

Re: [PATCH v2 2/2] btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

[Anand Jain]

On 17/11/2021 01:16, David Sterba wrote:
> On Thu, Oct 21, 2021 at 11:31:17PM +0800, Anand Jain wrote:
>> In the case of the seed device, the fsid can be different from the mounted
>> sprout fsid.  The userland has to read the device superblock to know the
>> fsid but, that idea fails if the device is missing. So add a sysfs
>> interface devinfo/<devid>/fsid to show the fsid of the device.
>>
>> For example:
>>   $ cd /sys/fs/btrfs/b10b02a5-f9de-4276-b9e8-2bfd09a578a8
>>
>>   $ cat devinfo/1/fsid
>>   c44d771f-639d-4df3-99ec-5bc7ad2af93b
>>   $ cat  devinfo/3/fsid
>>   b10b02a5-f9de-4276-b9e8-2bfd09a578a8
> 
>  From user perspective, it's another fsid, one is in the path, so I'm
> wondering if it should be named like read_fsid or sprout_fsid or if the
> seed/sprout information should be put into another directory completely.

I am viewing it as fsid as per the device's sb. This fsid matches with
the blkid(8) output. A path to the device's fsid will help to script.
So I am not voting for sprout_fsid because it does not exist in the
most common non-sprouted fsid.

Now to show whether a device is seed or sprout, the user friendly
approach will be like for example:
      /sys/fs/btrfs/<fsid>/devinfo/<devid>/device_state
to show all that apply, for example:
      SEED|SPROUT|READ_ONLY|WRITABLE|ZONED|METADATA_ONLY|\
      DATA_ONLY|READ_PREFERRED|REPLACE_TGT|REPLACE_SRC|\
      SCRUB_RUNNING

However, per kernel general rule of thumb one value per sys-fs file,
so it should be for example:
      /sys/fs/btrfs/<fsid>/devinfo/<devid>/seed
0
      /sys/fs/btrfs/<fsid>/devinfo/<devid>/sprout
1

So based on this my patch "btrfs: introduce new device-state
read_preferred" in the mailing list already proposed
      /sys/fs/btrfs/<fsid>/devinfo/<devid>/read_preferred

So to summarize,  I can rename fsid to read_fsid. And, progressively
in a separate patch, we can add <>/seed <>/sprout sysfs files as above.
What do you think?

Re: [PATCH v2 2/2] btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

[Anand Jain]

David,

  Gentle ping on this patch?

Thanks, Anand


On 17/11/2021 11:29, Anand Jain wrote:
> 
> 
> On 17/11/2021 01:16, David Sterba wrote:
>> On Thu, Oct 21, 2021 at 11:31:17PM +0800, Anand Jain wrote:
>>> In the case of the seed device, the fsid can be different from the 
>>> mounted
>>> sprout fsid.  The userland has to read the device superblock to know the
>>> fsid but, that idea fails if the device is missing. So add a sysfs
>>> interface devinfo/<devid>/fsid to show the fsid of the device.
>>>
>>> For example:
>>>   $ cd /sys/fs/btrfs/b10b02a5-f9de-4276-b9e8-2bfd09a578a8
>>>
>>>   $ cat devinfo/1/fsid
>>>   c44d771f-639d-4df3-99ec-5bc7ad2af93b
>>>   $ cat  devinfo/3/fsid
>>>   b10b02a5-f9de-4276-b9e8-2bfd09a578a8
>>
>>  From user perspective, it's another fsid, one is in the path, so I'm
>> wondering if it should be named like read_fsid or sprout_fsid or if the
>> seed/sprout information should be put into another directory completely.
> 
> I am viewing it as fsid as per the device's sb. This fsid matches with
> the blkid(8) output. A path to the device's fsid will help to script.
> So I am not voting for sprout_fsid because it does not exist in the
> most common non-sprouted fsid.
> 
> Now to show whether a device is seed or sprout, the user friendly
> approach will be like for example:
>       /sys/fs/btrfs/<fsid>/devinfo/<devid>/device_state
> to show all that apply, for example:
>       SEED|SPROUT|READ_ONLY|WRITABLE|ZONED|METADATA_ONLY|\
>       DATA_ONLY|READ_PREFERRED|REPLACE_TGT|REPLACE_SRC|\
>       SCRUB_RUNNING
> 
> However, per kernel general rule of thumb one value per sys-fs file,
> so it should be for example:
>       /sys/fs/btrfs/<fsid>/devinfo/<devid>/seed
> 0
>       /sys/fs/btrfs/<fsid>/devinfo/<devid>/sprout
> 1
> 
> So based on this my patch "btrfs: introduce new device-state
> read_preferred" in the mailing list already proposed
>       /sys/fs/btrfs/<fsid>/devinfo/<devid>/read_preferred
> 
> So to summarize,  I can rename fsid to read_fsid. And, progressively
> in a separate patch, we can add <>/seed <>/sprout sysfs files as above.
> What do you think?

Re: [PATCH v2 2/2] btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

[David Sterba]

On Wed, Nov 17, 2021 at 11:29:26AM +0800, Anand Jain wrote:
> On 17/11/2021 01:16, David Sterba wrote:
> > On Thu, Oct 21, 2021 at 11:31:17PM +0800, Anand Jain wrote:
> >> In the case of the seed device, the fsid can be different from the mounted
> >> sprout fsid.  The userland has to read the device superblock to know the
> >> fsid but, that idea fails if the device is missing. So add a sysfs
> >> interface devinfo/<devid>/fsid to show the fsid of the device.
> >>
> >> For example:
> >>   $ cd /sys/fs/btrfs/b10b02a5-f9de-4276-b9e8-2bfd09a578a8
> >>
> >>   $ cat devinfo/1/fsid
> >>   c44d771f-639d-4df3-99ec-5bc7ad2af93b
> >>   $ cat  devinfo/3/fsid
> >>   b10b02a5-f9de-4276-b9e8-2bfd09a578a8
> > 
> >  From user perspective, it's another fsid, one is in the path, so I'm
> > wondering if it should be named like read_fsid or sprout_fsid or if the
> > seed/sprout information should be put into another directory completely.
> 
> I am viewing it as fsid as per the device's sb.

That's a reasonable point.

> This fsid matches with
> the blkid(8) output. A path to the device's fsid will help to script.
> So I am not voting for sprout_fsid because it does not exist in the
> most common non-sprouted fsid.

Yeah agreed, I've put this to the changelog as it explains the naming.

Re: [PATCH v2 2/2] btrfs: sysfs add devinfo/fsid to retrieve fsid from the device

[David Sterba]

On Thu, Oct 21, 2021 at 11:31:17PM +0800, Anand Jain wrote:
> In the case of the seed device, the fsid can be different from the mounted
> sprout fsid.  The userland has to read the device superblock to know the
> fsid but, that idea fails if the device is missing. So add a sysfs
> interface devinfo/<devid>/fsid to show the fsid of the device.
> 
> For example:
>  $ cd /sys/fs/btrfs/b10b02a5-f9de-4276-b9e8-2bfd09a578a8
> 
>  $ cat devinfo/1/fsid
>  c44d771f-639d-4df3-99ec-5bc7ad2af93b
>  $ cat  devinfo/3/fsid
>  b10b02a5-f9de-4276-b9e8-2bfd09a578a8
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Added to misc-next, thanks.