Improve readbility of NVME "wwid" attribute

With the current implementation, the default "fallback" WWID generation code (if no nguid, euid etc. are defined) for Linux NVME host and target results in the following WWID format:

nvme.0000-3163653363666438366239656630386200-4c696e75780000000000000000000000000000000000000000000000000000000000000000000000-00000002

This is not only hard to read, it poses real problems for multipath (dm WWIDs are limited to 128 characters), and it's not fully standards compliant.

With this patch series, the WWID on a Linux host connected to a Linux target looks like this:
nvme.0000-d319fc8b2883bfec-4c696e7578-00000001

Martin Wilck (3):
nvmet: identify controller: improve standard compliance
nvme: wwid_show: strip trailing 0-bytes
nvme: wwid_show: copy hex string verbatim

 drivers/nvme/host/core.c        | 30 ++++++++++++++++++++++++++----
 drivers/nvme/target/admin-cmd.c | 21 ++++++++++++++-------
 2 files changed, 40 insertions(+), 11 deletions(-)

2.13.2

The NVME standard mandates that the SN, MN, and FR fields of the Indentify Controller Data Structure be "ASCII strings". That means that they may not contain 0-bytes, not even string terminators.

---
 drivers/nvme/target/admin-cmd.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

index 35f930db3c02c..b580141d57401 100644
@@ -168,11 +168,21 @@ static void nvmet_execute_get_log_page(struct nvmet_req *req)
 	nvmet_req_complete(req, status);
 }
 
+static void copy_and_pad(char *dst, int dst_len, const char *src, int src_len)
+{
+	int len = min(src_len, dst_len);
+
+	memcpy(dst, src, len);
+	if (dst_len > len)
+		memset(dst + len, ' ', dst_len - len);
+}
+
 static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 {
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	struct nvme_id_ctrl *id;
 	u16 status = 0;
+	const char model[] = "Linux";
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
 	if (!id) {

@@ -184,14 +194,11 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 	id->vid = 0;
 	id->ssvid = 0;
 
-	memset(id->sn, ' ', sizeof(id->sn));
-	snprintf(id->sn, sizeof(id->sn), "%llx", ctrl->serial);
+	bin2hex(id->sn, &ctrl->serial, min(sizeof(ctrl->serial),
+					   sizeof(id->sn) / 2));
 
-	memset(id->mn, ' ', sizeof(id->mn));
-	strncpy((char *)id->mn, "Linux", sizeof(id->mn));
-
-	memset(id->fr, ' ', sizeof(id->fr));
-	strncpy((char *)id->fr, UTS_RELEASE, sizeof(id->fr));
+	copy_and_pad(id->mn, sizeof(id->mn), model, sizeof(model) - 1);
+	copy_and_pad(id->fr, sizeof(id->fr), UTS_RELEASE, strlen(UTS_RELEASE));
 
 	id->rab = 6;
 

2.13.2

Can you add your copy_and_pad helper to include/linux/kernel.h? I'll also need it for the nvme host multipath support.

Some broken targets (such as the current Linux target) pad model or serial fields with 0-bytes rather than spaces. The NVME spec disallows 0 bytes in "ASCII" fields. Thus strip trailing 0-bytes, too.

---
 drivers/nvme/host/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index d70df1d0072d4..deced6b91442d 100644
@@ -2001,9 +2001,11 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
 	if (memchr_inv(ns->eui, 0, sizeof(ns->eui)))
 		return sprintf(buf, "eui.%8phN\n", ns->eui);
 
-	while (ctrl->serial[serial_len - 1] == ' ')
+	while (ctrl->serial[serial_len - 1] == ' ' ||
+	       ctrl->serial[serial_len - 1] == '\0')
 		serial_len--;
-	while (ctrl->model[model_len - 1] == ' ')
+	while (ctrl->model[model_len - 1] == ' ' ||
+	       ctrl->model[model_len - 1] == '\0')
 		model_len--;
 
 	return sprintf(buf, "nvme.%04x-%*phN-%*phN-%08x\n", ctrl->vid,

2.13.2

Looks fine,

If the serial number is a series of hex digits (as usual for the Linux target), there's no need to encode it in hex once more. This will improve the readability of NVME wwids for users.

---
 drivers/nvme/host/core.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

index deced6b91442d..c80e90603c9c4 100644
@@ -27,6 +27,7 @@
 #include <linux/nvme_ioctl.h>
 #include <linux/t10-pi.h>
 #include <linux/pm_qos.h>
+#include <linux/ctype.h>
 #include <asm/unaligned.h>
 
 #include "nvme.h"

@@ -1987,6 +1988,14 @@ static ssize_t nvme_sysfs_rescan(struct device *dev,
 }
 static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, nvme_sysfs_rescan);
 
+static bool _is_hex_string(const char *buf, int len)
+{
+	const char *p;
+
+	for (p = buf; p < buf + len && isxdigit(*p); p++);
+	return p == buf + len;
+}
+
 static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
 								char *buf)
 {

@@ -1994,6 +2003,7 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	int serial_len = sizeof(ctrl->serial);
 	int model_len = sizeof(ctrl->model);
+	char *p;
 
 	if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid)))
 		return sprintf(buf, "eui.%16phN\n", ns->nguid);

@@ -2008,8 +2018,18 @@ static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
 	       ctrl->model[model_len - 1] == '\0')
 		model_len--;
 
-	return sprintf(buf, "nvme.%04x-%*phN-%*phN-%08x\n", ctrl->vid,
-		serial_len, ctrl->serial, model_len, ctrl->model, ns->ns_id);
+	p = buf + sprintf(buf, "nvme.%04x-", ctrl->vid);
+
+	if (_is_hex_string(ctrl->serial, serial_len)) {
+		memcpy(p, ctrl->serial, serial_len);
+		p += serial_len;
+		*p++ = '-';
+	} else
+		p += sprintf(p, "%*phN-", serial_len, ctrl->serial);
+
+	p += sprintf(p, "%*phN-%08x\n", model_len, ctrl->model, ns->ns_id);
+
+	return p - buf;
 }
 static DEVICE_ATTR(wwid, S_IRUGO, wwid_show, NULL);
 

2.13.2

NAK, this will break existing device identifiers.

If this is what you think, you must also NAK my patch 2/3, because stripping the 0-bytes also changes how the WWID is presented to user space. For example

nvme.0000-3163653363666438366239656630386200- 4c696e75780000000000000000000000000000000000000000000000000000000000000 000000000-00000001

becomes after 2/3:

nvme.0000-31636533636664383662396566303862-4c696e7578-00000001

3/3 would change this again to

nvme.0000-1ce3cfd86b9ef08b-4c696e7578-00000001

and Keith's proposal would change it to

nvme.0000-1ce3cfd86b9ef08b-Linux-00000001

User space that relies on the string in the first format would break already with 2/3. However, multipath, which is probably the main consumer of the WWID for practical purposes, can't handle the overlong format anyway (I'm preparing a patch series to fix that).

Therefore, please think twice. It may be worthwhile to accept an exception to the API stability rule here. Some people will be bitten, but we will have a saner WWID format in the future. Of course, if this is to be changed, we'd better do it asap.

Note also that this identifier is volatile for Linux targets anyway, as the serial number changes between target reboots (I'm aware of Johannes' recent attempt to change this, but again this is going to be a configuration that the storage admin must actively apply).

As I wrote in my other mail, if API stability prevails, let's introduce a new attribute with cleaner format.

Martin

Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

True. Although that only really is a fix for buggy controllers, and should not affect the PCIe controllers (for which a compliance test for this exists, unfortunately that doesn't work for fabrics).

We'll handle multipath in NVMe and the kernel itself, so we really should not be worried about dm-multipath here, which is the wrong thing to do for NVMe.

Is that a NAK, or not?

"We will"? People are (trying to) use NVMe with multipath today, and they use dm-multipath and multipathd for that. Maybe that'll change some day, but not too soon, I believe.

Regards
Martin

Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

That's an ACK for the null vs whitespace changes. The NAK for this patch still stands.

Who are these people and why aren't they on the nvme list? The dm-mpath approach is fundamentally wrong for NVMe, and especially NVMeOF - e.g. for NVMeOF we have the mandatory keep alive, so doing any wonky path checking from userspace is just going to create problems.

Hi Christoph; we are on the list :-)

One could argue that _all_ current path checkers in dm-mpath are 'wrong', so it feels a bit of a silly argument.
And it was never meant to be the 'real' solution; but it seems to work and has the immediate benefit that we're having a solution _now_. (And as our next release is shipping withing weeks that makes a _really_ compelling argument)
And I'm happy to enter any path checker discussion;
seeing that we've had issues with KATO even now (cf the thread 'I/O errors due to keepalive timeouts with NVMf RDMA) I somewhat fail to see the inherent benefits compared to the 'standard' path checkers ...

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)

Just curious for non-hex strings, is there a problem with any utilities if we use the ASCII for both serial and model? It'd be half as long.

For example, my device's wwid attribute looks like this today:

nvme.8086-46554d42353235363030304a32383041-494e54454c2053534450454431443134304741-00000001

But would this cause a problem for anything?

nvme.8086-FUMB5256000J280A-INTEL_SSDPED1D140GA-00000001

That was my first approach to the issue. But then I realized that the term "WWID" is ususally associated with a hex string.
(https://en.wikipedia.org/wiki/World_Wide_Name), so allowing arbitrary ASCII might violate some people's assumptions of how a WWID should look like. (Not to mention that we don't have a vendor OUI...).

I don't know. That's why I took the more conservative approach. I think yours is fine (actually much better for human beings), we just shouldn't call it "wwid".

Martin

Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

iSCSI device identifiers also use a string, and even worse it's utf8 and not just 7-bit ASCII.

It would have been nicer, but it will break existing setups.

Distros should instead switch to use the nqn for the char dev uniqueue identifier (we now fake it up even for controllers that don't provide it), and the EUI64/NGUID/UUID if provided for the block device names.

The "if provided" is the crucial point here. In real life currently many people struggle with ugly overlong identifiers because these IDs are _not_ provided by default. I daresay that this attribute is pretty much broken in current setups, in particular linux host with linux target.

If it's late to change this attribute for API stability reasons, we should consider keeping it as legacy and add (yet) another one with a saner format.

Martin

Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

NVMe has some pretty strong language in that regard since 1.2, which says for the EUI64/NGUID fields:

"The controller shall specify a globally unique namespace identifier in this
field or the EUI64 field when the namespace is created."

And with 1.3 the new mandatory Identify 03h. So there really aren't any excuses anymore.

What standard? The wwid field is a Linux invention.

I was referring to patch 1/3 in the series, the Linux target padding the "model" string with 0-bytes, which is not allowed.

Regards
Martin

Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)