| Message ID | f3657efae72f9abb93d3169308052e77bf0dbad6.1730292757.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mikulas Patocka |
| Headers | show |
| Series | dm: add support for get_unique_id | expand |
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);
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
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.
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
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
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 };
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(+) base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652