[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Martin Wilck]

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

[PATCH 1/3] nvmet: identify controller: improve standard compliance

[Martin Wilck]

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.

Signed-off-by: Martin Wilck <mwilck at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.de>
---
 drivers/nvme/target/admin-cmd.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 35f930db3c02c..b580141d57401 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -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

[PATCH 1/3] nvmet: identify controller: improve standard compliance

[Christoph Hellwig]

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

[PATCH 2/3] nvme: wwid_show: strip trailing 0-bytes

[Martin Wilck]

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.

Signed-off-by: Martin Wilck <mwilck at suse.com>
Reviewed-by: Hannes Reinecke <hare at suse.de>
---
 drivers/nvme/host/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d70df1d0072d4..deced6b91442d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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

[PATCH 2/3] nvme: wwid_show: strip trailing 0-bytes

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Martin Wilck]

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.

Signed-off-by: Martin Wilck <mwilck at suse.com>
---
 drivers/nvme/host/core.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index deced6b91442d..c80e90603c9c4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Christoph Hellwig]

On Fri, Jul 14, 2017@12:25:33AM +0200, Martin Wilck wrote:
> 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.

NAK, this will break existing device identifiers.

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Martin Wilck]

On Fri, 2017-07-14@09:54 +0200, Christoph Hellwig wrote:
> On Fri, Jul 14, 2017@12:25:33AM +0200, Martin Wilck wrote:
> > 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.
> 
> 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)

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Christoph Hellwig]

On Fri, Jul 14, 2017@11:58:16AM +0200, Martin Wilck wrote:
> 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

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).

> 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).

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.

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Martin Wilck]

On Fri, 2017-07-14@14:30 +0200, Christoph Hellwig wrote:
> On Fri, Jul 14, 2017@11:58:16AM +0200, Martin Wilck wrote:
> > 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
> 
> 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).

Is that a NAK, or not?

> > 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).
> 
> 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.

"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)

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Christoph Hellwig]

On Fri, Jul 14, 2017@09:40:57PM +0200, Martin Wilck wrote:
> > 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).
> 
> Is that a NAK, or not?

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

> "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.

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.

[PATCH 3/3] nvme: wwid_show: copy hex string verbatim

[Hannes Reinecke]

On 07/15/2017 10:46 AM, Christoph Hellwig wrote:
> On Fri, Jul 14, 2017@09:40:57PM +0200, Martin Wilck wrote:
>>> 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).
>>
>> Is that a NAK, or not?
> 
> That's an ACK for the null vs whitespace changes.  The NAK
> for this patch still stands.
> 
>> "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.
> 
> 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)

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Keith Busch]

On Fri, Jul 14, 2017@12:25:30AM +0200, Martin Wilck wrote:
> 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

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

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Martin Wilck]

On Thu, 2017-07-13@18:57 -0400, Keith Busch wrote:
> On Fri, Jul 14, 2017@12:25:30AM +0200, Martin Wilck wrote:
> > 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-
> > 4c696e7578000000000000000000000000000000000000000000000000000000000
> > 0000000000000-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
> 
> 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.

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...).

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

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)

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Christoph Hellwig]

On Fri, Jul 14, 2017@09:54:12AM +0200, Martin Wilck wrote:
> 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...).

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

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Christoph Hellwig]

On Thu, Jul 13, 2017@06:57:39PM -0400, Keith Busch wrote:
> 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

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.

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Martin Wilck]

On Fri, 2017-07-14@09:56 +0200, Christoph Hellwig wrote:
> On Thu, Jul 13, 2017@06:57:39PM -0400, Keith Busch wrote:
> > 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
> 
> 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)

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Christoph Hellwig]

On Fri, Jul 14, 2017@11:41:37AM +0200, Martin Wilck wrote:
> 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.

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.

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Christoph Hellwig]

On Fri, Jul 14, 2017@12:25:30AM +0200, Martin Wilck wrote:
> 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.

What standard?  The wwid field is a Linux invention. 

[PATCH 0/3] Improve readbility of NVME "wwid" attribute

[Martin Wilck]

On Fri, 2017-07-14@09:53 +0200, Christoph Hellwig wrote:
> On Fri, Jul 14, 2017@12:25:30AM +0200, Martin Wilck wrote:
> > 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-
> > 4c696e7578000000000000000000000000000000000000000000000000000000000
> > 0000000000000-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.
> 
> 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)