NVMe wwid revisited

NVMe wwid revisited

[Hannes Reinecke]

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)

NVMe wwid revisited

[Christoph Hellwig]

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.

NVMe wwid revisited

[Keith Busch]

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;
---

NVMe wwid revisited

[Hannes Reinecke]

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)