* NVMe wwid revisited @ 2016-02-18 13:04 Hannes Reinecke 2016-02-18 13:28 ` Christoph Hellwig 0 siblings, 1 reply; 4+ messages in thread From: Hannes Reinecke @ 2016-02-18 13:04 UTC (permalink / raw) Hi Keith, I've had a look at the current NVMe implementation for SCSI VPD page 0x83, in order to implement a generic 'wwid' field. And I've even looked at the NVM-Express-SCSI-Translation Reference version 1.1. What a mess :-( First of all, there's an error in the spec: Section 6.1.4.3 T10 Vendor ID Based Descriptor has: DESIGNATOR TYPE Shall be set to 8h indicating SCSI name string format and assignment authority. T10 VENDOR IDENTIFICATION Refer to 3.12 VENDOR SPECIFIC IDENTIFIER Shall be set the concatenation of the translation of PRODUCT IDENTIFICATION field from standard INQUIRY data as specified in 3.9, and the IEEE Extended Unique Identifier (EUI64) field of the Identify Namespace Data Structure This does _not_ match with T-10 usage; T-10 Vendor ID based identification is _NOT_ a scsi name string. And the scsi name string has to start with either 'eui.', 'naa.' or 'iqn.'. Plus the SCSI name string has to be in UTF-8, not ASCII. So I guess they actually meant for the designator type to be of T-10 Vendor Identification (ie designator type 1). Can you address this with the NVMe committee? Or do you have someone which I can poke? Which, of course, raises the question: What _is_ the correct identification? Implement it according to the translation spec, leaving out the 'T10 Vendor Identification' field? Or implement it with the correct designator type? (Which is interesting in itself; one simply _cannot_ implement a valid translation according to the spec. How curious ...) Cheers, Hannes -- Dr. Hannes Reinecke Teamlead Storage & Networking hare at suse.de +49 911 74053 688 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG N?rnberg) ^ permalink raw reply [flat|nested] 4+ messages in thread
* NVMe wwid revisited 2016-02-18 13:04 NVMe wwid revisited Hannes Reinecke @ 2016-02-18 13:28 ` Christoph Hellwig 2016-02-18 15:10 ` Keith Busch 0 siblings, 1 reply; 4+ messages in thread From: Christoph Hellwig @ 2016-02-18 13:28 UTC (permalink / raw) On Thu, Feb 18, 2016@02:04:28PM +0100, Hannes Reinecke wrote: > Hi Keith, > > I've had a look at the current NVMe implementation for SCSI VPD page > 0x83, in order to implement a generic 'wwid' field. > > And I've even looked at the NVM-Express-SCSI-Translation Reference > version 1.1. What a mess :-( There is a version 1.5 by now.. > So I guess they actually meant for the designator type to be of T-10 > Vendor Identification (ie designator type 1). The 1.5 spec has actually fixed this up: 6.1.4.3 T10 Vendor ID Based Descriptor ... DESIGNATOR TYPE | Shall be set to 1h indicating T10 vendor ID based format. > Which, of course, raises the question: What _is_ the correct > identification? > Implement it according to the translation spec, leaving out the 'T10 > Vendor Identification' field? > Or implement it with the correct designator type? Leave it as-is, given a certain Linux distributor started using it for their udev rules and we'd otherwise break their setup. ^ permalink raw reply [flat|nested] 4+ messages in thread
* NVMe wwid revisited 2016-02-18 13:28 ` Christoph Hellwig @ 2016-02-18 15:10 ` Keith Busch 2016-02-18 15:39 ` Hannes Reinecke 0 siblings, 1 reply; 4+ messages in thread From: Keith Busch @ 2016-02-18 15:10 UTC (permalink / raw) On Thu, Feb 18, 2016@02:28:56PM +0100, Christoph Hellwig wrote: > On Thu, Feb 18, 2016@02:04:28PM +0100, Hannes Reinecke wrote: > > Which, of course, raises the question: What _is_ the correct > > identification? > > Implement it according to the translation spec, leaving out the 'T10 > > Vendor Identification' field? > > Or implement it with the correct designator type? > > Leave it as-is, given a certain Linux distributor started using it for > their udev rules and we'd otherwise break their setup. We mentioned exposing a single sysfs handle that provides the unique so the user doesn't have to check/concat multiple files or use the deprecated NVMe SG_IO interface. Does the below patch look alright? It uses either 64 or 128 bit EUI if provided, or concats other parts to make a unique (and long!) identifier if the namespace doesn't support its own unique id. Here's a couple examples with a 1.0 controller and a 1.2 controller (I don't have a 1.1 controller; no EUI-64 example): 1.0: # cat /sys/block/nvme0n1/wwid nvme.8086-43564654343032333030324b38303043474e-494e54454c205353445045444d443830304734-00000001 1.2: # cat /sys/block/nvme1n2/wwid eui.5cd2e400000000000000000000000100000002 --- diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index face31f..93ec2e0 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -872,6 +876,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) return -EIO; } + ctrl->vid = le16_to_cpu(id->vid); ctrl->oncs = le16_to_cpup(&id->oncs); atomic_set(&ctrl->abort_limit, id->acl + 1); ctrl->vwc = id->vwc; @@ -1008,6 +1013,30 @@ static ssize_t nvme_sysfs_reset(struct device *dev, } static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, nvme_sysfs_reset); +static ssize_t wwid_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct nvme_ns *ns = dev_to_disk(dev)->private_data; + struct nvme_ctrl *ctrl = ns->ctrl; + int serial_len = sizeof(ctrl->serial); + int model_len = sizeof(ctrl->model); + + if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) + return sprintf(buf, "eui.%16phN\n", ns->uuid); + + if (memchr_inv(ns->eui, 0, sizeof(ns->eui))) + return sprintf(buf, "eui.%8phN\n", ns->eui); + + while (ctrl->serial[serial_len - 1] == ' ') + serial_len--; + while (ctrl->model[model_len - 1] == ' ') + model_len--; + + return sprintf(buf, "nvme.%04x-%*phN-%*phN-%08x\n", ctrl->vid, + serial_len, ctrl->serial, model_len, ctrl->model, ns->ns_id); +} +static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL); + static ssize_t uuid_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -1033,6 +1062,7 @@ static ssize_t nsid_show(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL); static struct attribute *nvme_ns_attrs[] = { + &dev_attr_wwid.attr, &dev_attr_uuid.attr, &dev_attr_eui.attr, &dev_attr_nsid.attr, diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index dcdc2d7..b50e591 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -84,6 +84,7 @@ struct nvme_ctrl { u32 max_hw_sectors; u32 stripe_size; u16 oncs; + u16 vid; atomic_t abort_limit; u8 event_limit; u8 vwc; --- ^ permalink raw reply related [flat|nested] 4+ messages in thread
* NVMe wwid revisited 2016-02-18 15:10 ` Keith Busch @ 2016-02-18 15:39 ` Hannes Reinecke 0 siblings, 0 replies; 4+ messages in thread From: Hannes Reinecke @ 2016-02-18 15:39 UTC (permalink / raw) On 02/18/2016 04:10 PM, Keith Busch wrote: > On Thu, Feb 18, 2016@02:28:56PM +0100, Christoph Hellwig wrote: >> On Thu, Feb 18, 2016@02:04:28PM +0100, Hannes Reinecke wrote: >>> Which, of course, raises the question: What _is_ the correct >>> identification? >>> Implement it according to the translation spec, leaving out the 'T10 >>> Vendor Identification' field? >>> Or implement it with the correct designator type? >> >> Leave it as-is, given a certain Linux distributor started using it for >> their udev rules and we'd otherwise break their setup. > > We mentioned exposing a single sysfs handle that provides the unique so > the user doesn't have to check/concat multiple files or use the deprecated > NVMe SG_IO interface. > > Does the below patch look alright? It uses either 64 or 128 bit EUI if > provided, or concats other parts to make a unique (and long!) identifier > if the namespace doesn't support its own unique id. > > Here's a couple examples with a 1.0 controller and a 1.2 controller > (I don't have a 1.1 controller; no EUI-64 example): > > 1.0: > # cat /sys/block/nvme0n1/wwid > nvme.8086-43564654343032333030324b38303043474e-494e54454c205353445045444d443830304734-00000001 > > 1.2: > # cat /sys/block/nvme1n2/wwid > eui.5cd2e400000000000000000000000100000002 > > --- > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index face31f..93ec2e0 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -872,6 +876,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) > return -EIO; > } > > + ctrl->vid = le16_to_cpu(id->vid); > ctrl->oncs = le16_to_cpup(&id->oncs); > atomic_set(&ctrl->abort_limit, id->acl + 1); > ctrl->vwc = id->vwc; > @@ -1008,6 +1013,30 @@ static ssize_t nvme_sysfs_reset(struct device *dev, > } > static DEVICE_ATTR(reset_controller, S_IWUSR, NULL, nvme_sysfs_reset); > > +static ssize_t wwid_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct nvme_ns *ns = dev_to_disk(dev)->private_data; > + struct nvme_ctrl *ctrl = ns->ctrl; > + int serial_len = sizeof(ctrl->serial); > + int model_len = sizeof(ctrl->model); > + > + if (memchr_inv(ns->uuid, 0, sizeof(ns->uuid))) > + return sprintf(buf, "eui.%16phN\n", ns->uuid); > + > + if (memchr_inv(ns->eui, 0, sizeof(ns->eui))) > + return sprintf(buf, "eui.%8phN\n", ns->eui); > + > + while (ctrl->serial[serial_len - 1] == ' ') > + serial_len--; > + while (ctrl->model[model_len - 1] == ' ') > + model_len--; > + > + return sprintf(buf, "nvme.%04x-%*phN-%*phN-%08x\n", ctrl->vid, > + serial_len, ctrl->serial, model_len, ctrl->model, ns->ns_id); > +} > +static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL); > + > static ssize_t uuid_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > @@ -1033,6 +1062,7 @@ static ssize_t nsid_show(struct device *dev, struct device_attribute *attr, > static DEVICE_ATTR(nsid, S_IRUGO, nsid_show, NULL); > > static struct attribute *nvme_ns_attrs[] = { > + &dev_attr_wwid.attr, > &dev_attr_uuid.attr, > &dev_attr_eui.attr, > &dev_attr_nsid.attr, > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index dcdc2d7..b50e591 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -84,6 +84,7 @@ struct nvme_ctrl { > u32 max_hw_sectors; > u32 stripe_size; > u16 oncs; > + u16 vid; > atomic_t abort_limit; > u8 event_limit; > u8 vwc; > --- > Yeah, that looks good. Reviewed-by: Hannes Reinecke <hare at suse.com> Cheers, Hannes -- Dr. Hannes Reinecke zSeries & Storage hare at suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: J. Hawn, J. Guild, F. Imend?rffer, HRB 16746 (AG N?rnberg) ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-18 15:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-18 13:04 NVMe wwid revisited Hannes Reinecke 2016-02-18 13:28 ` Christoph Hellwig 2016-02-18 15:10 ` Keith Busch 2016-02-18 15:39 ` Hannes Reinecke
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.