[PATCH] dm: add support for get_unique_id

[PATCH] dm: add support for get_unique_id

[Benjamin Coddington]

This adds support to obtain a device's unique id through dm, similar to the
existing ioctl and persistent resevation handling.  We limit this to
single-target devices.

This enables knfsd to export pNFS SCSI luns that have been exported from
multipath devices.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 drivers/md/dm.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 87bb90303435..e707b3f57f29 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -3342,6 +3342,54 @@ void dm_free_md_mempools(struct dm_md_mempools *pools)
 	kfree(pools);
 }
 
+struct dm_unique_id {
+	u8 *id;
+	enum blk_unique_id type;
+};
+
+static int __dm_get_unique_id(struct dm_target *ti, struct dm_dev *dev,
+				sector_t start, sector_t len, void *data)
+{
+	struct dm_unique_id *dmuuid = data;
+	const struct block_device_operations *fops = dev->bdev->bd_disk->fops;
+
+	if (fops->get_unique_id)
+		return fops->get_unique_id(dev->bdev->bd_disk, dmuuid->id, dmuuid->type);
+
+	return 0;
+}
+
+static int dm_blk_get_unique_id(struct gendisk *disk, u8 *id,
+		enum blk_unique_id type)
+{
+	struct mapped_device *md = disk->private_data;
+	struct dm_table *table;
+	struct dm_target *ti;
+	int ret = 0, srcu_idx;
+
+	struct dm_unique_id dmuuid = {
+		.id = id,
+		.type = type,
+	};
+
+	table = dm_get_live_table(md, &srcu_idx);
+	if (!table || !dm_table_get_size(table))
+		goto out;
+
+	/* We only support devices that have a single target */
+	if (table->num_targets != 1)
+		goto out;
+	ti = dm_table_get_target(table, 0);
+
+	if (!ti->type->iterate_devices)
+		goto out;
+
+	ret = ti->type->iterate_devices(ti, __dm_get_unique_id, &dmuuid);
+out:
+	dm_put_live_table(md, srcu_idx);
+	return ret;
+}
+
 struct dm_pr {
 	u64	old_key;
 	u64	new_key;
@@ -3667,6 +3715,7 @@ static const struct block_device_operations dm_blk_dops = {
 	.ioctl = dm_blk_ioctl,
 	.getgeo = dm_blk_getgeo,
 	.report_zones = dm_blk_report_zones,
+	.get_unique_id = dm_blk_get_unique_id,
 	.pr_ops = &dm_pr_ops,
 	.owner = THIS_MODULE
 };
@@ -3676,6 +3725,7 @@ static const struct block_device_operations dm_rq_blk_dops = {
 	.release = dm_blk_close,
 	.ioctl = dm_blk_ioctl,
 	.getgeo = dm_blk_getgeo,
+	.get_unique_id = dm_blk_get_unique_id,
 	.pr_ops = &dm_pr_ops,
 	.owner = THIS_MODULE
 };

base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.47.0

Re: [PATCH] dm: add support for get_unique_id

[Christoph Hellwig]

On Wed, Oct 30, 2024 at 08:57:56AM -0400, Benjamin Coddington wrote:
> This adds support to obtain a device's unique id through dm, similar to the
> existing ioctl and persistent resevation handling.  We limit this to
> single-target devices.
> 
> This enables knfsd to export pNFS SCSI luns that have been exported from
> multipath devices.

Is there anything that ensures that the underlying IDs actually
match?

> +	struct dm_unique_id *dmuuid = data;
> +	const struct block_device_operations *fops = dev->bdev->bd_disk->fops;
> +
> +	if (fops->get_unique_id)
> +		return fops->get_unique_id(dev->bdev->bd_disk, dmuuid->id, dmuuid->type);
> +
> +	return 0;

Overly long line here.  Also maybe just personal, but I find code easier
to read when the exit early condition is in the branch, e.g.

	strut gendisk *disk = dev->bdev->bd_disk

	if (!disk->fops->get_unique_id)
		return 0;
	return disk->fops->get_unique_id(disk, dmuuid->id, dmuuid->type);

Re: [PATCH] dm: add support for get_unique_id

[Benjamin Coddington]

On 30 Oct 2024, at 9:52, Christoph Hellwig wrote:

> On Wed, Oct 30, 2024 at 08:57:56AM -0400, Benjamin Coddington wrote:
>> This adds support to obtain a device's unique id through dm, similar to the
>> existing ioctl and persistent resevation handling.  We limit this to
>> single-target devices.
>>
>> This enables knfsd to export pNFS SCSI luns that have been exported from
>> multipath devices.
>
> Is there anything that ensures that the underlying IDs actually
> match?

Match each other in a multipath device you mean?  No, this will just return
the first one where get_unique_id returns non-zero.  Can they actually be
different, and if so should we return an error?

>
>> +	struct dm_unique_id *dmuuid = data;
>> +	const struct block_device_operations *fops = dev->bdev->bd_disk->fops;
>> +
>> +	if (fops->get_unique_id)
>> +		return fops->get_unique_id(dev->bdev->bd_disk, dmuuid->id, dmuuid->type);
>> +
>> +	return 0;
>
> Overly long line here.  Also maybe just personal, but I find code easier
> to read when the exit early condition is in the branch, e.g.
>
> 	strut gendisk *disk = dev->bdev->bd_disk
>
> 	if (!disk->fops->get_unique_id)
> 		return 0;
> 	return disk->fops->get_unique_id(disk, dmuuid->id, dmuuid->type);

That is nicer, will do.

Ben

Re: [PATCH] dm: add support for get_unique_id

[Christoph Hellwig]

On Wed, Oct 30, 2024 at 10:05:03AM -0400, Benjamin Coddington wrote:
> Match each other in a multipath device you mean?  No, this will just return
> the first one where get_unique_id returns non-zero.  Can they actually be
> different, and if so should we return an error?

That's what I've been wondering.  IIRC you can in theory create a
kernel mpath table for any devices you want.  multipathd only creates
them when the ids match, but do we want to rely on that?  It might be
perfectly fine to say if you break you keep the pieces, but then
I'd expected a comment about it in the comment.

Re: [PATCH] dm: add support for get_unique_id

[Benjamin Coddington]

On 30 Oct 2024, at 10:08, Christoph Hellwig wrote:

> On Wed, Oct 30, 2024 at 10:05:03AM -0400, Benjamin Coddington wrote:
>> Match each other in a multipath device you mean?  No, this will just return
>> the first one where get_unique_id returns non-zero.  Can they actually be
>> different, and if so should we return an error?
>
> That's what I've been wondering.  IIRC you can in theory create a
> kernel mpath table for any devices you want.  multipathd only creates
> them when the ids match, but do we want to rely on that?  It might be
> perfectly fine to say if you break you keep the pieces, but then
> I'd expected a comment about it in the comment.

Comment is easier than comparing them all, I'm happy to add it.  Seems like
a mpath table with different devices would make little pieces of anything
you try to put on it, and you wouldn't even get to keep those pieces.

Maybe other dm experts can think of ways that might break things.. I'll wait
a day or so.

Ben

Re: dm: add support for get_unique_id

[Mike Snitzer]

On Wed, Oct 30, 2024 at 10:19:11AM -0400, Benjamin Coddington wrote:
> On 30 Oct 2024, at 10:08, Christoph Hellwig wrote:
> 
> > On Wed, Oct 30, 2024 at 10:05:03AM -0400, Benjamin Coddington wrote:
> >> Match each other in a multipath device you mean?  No, this will just return
> >> the first one where get_unique_id returns non-zero.  Can they actually be
> >> different, and if so should we return an error?
> >
> > That's what I've been wondering.  IIRC you can in theory create a
> > kernel mpath table for any devices you want.  multipathd only creates
> > them when the ids match, but do we want to rely on that?  It might be
> > perfectly fine to say if you break you keep the pieces, but then
> > I'd expected a comment about it in the comment.
> 
> Comment is easier than comparing them all, I'm happy to add it.  Seems like
> a mpath table with different devices would make little pieces of anything
> you try to put on it, and you wouldn't even get to keep those pieces.
> 
> Maybe other dm experts can think of ways that might break things.. I'll wait
> a day or so.

It would be a concern for multipathd, you don't need to worry about this.

Documenting that dm_blk_get_unique_id returns the uuid from the
first underlying device that supports ->get_unique_id should be
sufficient.

Mike