[PATCH] NVMe: Expose namespace unique identifier to sysfs

[PATCH] NVMe: Expose namespace unique identifier to sysfs

[Keith Busch]

A controller namespace supporting 1.1 or 1.2 capabilities uniquely
identifies itself with either the 64-bit EUI or 128-bit NGUID.

This patch determines which the device supports and reports the unique
identifier in new sysfs binary attribute "uuid". The attribute group is
added to the gendisk's kobject directory.

No fallback is provided for controllers not reporting a unique id.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  2 ++
 2 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cd2a64f..472a96c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -574,6 +574,21 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 		ns->type = NVME_NS_LIGHTNVM;
 	}
 
+	if (ns->ctrl->vs >= NVME_VS(1, 1)) {
+		u8 *identifier = id->eui64;
+		int len = sizeof(id->eui64);
+
+		if (bitmap_empty((void *)identifier, len * 8) &&
+					ns->ctrl->vs >= NVME_VS(1, 2)) {
+			identifier = id->nguid;
+			len = sizeof(id->nguid);
+		}
+		if (!bitmap_empty((void *)identifier, len * 8)) {
+			memcpy(ns->uuid, identifier, len);
+			ns->uuid_len = len;
+		}
+	}
+
 	old_ms = ns->ms;
 	lbaf = id->flbas & NVME_NS_FLBAS_LBA_MASK;
 	ns->lba_shift = id->lbaf[lbaf].ds;
@@ -994,6 +1009,34 @@ static const struct attribute_group *nvme_dev_attr_groups[] = {
 	NULL
 };
 
+#define nvme_bin_attr(field)						\
+static ssize_t field##_show(struct file *filp, struct kobject *kobj,	\
+		 struct bin_attribute *bin_attr,			\
+		 char *buf, loff_t off, size_t count)			\
+{									\
+	struct device *dev = container_of(kobj, struct device, kobj);	\
+	struct nvme_ns *ns = dev_to_disk(dev)->private_data;		\
+	return memory_read_from_buffer(buf, count, &off,		\
+				       ns->field,			\
+				       ns->field##_len);		\
+}									\
+static struct bin_attribute dev_attr_nvme_##field = {			\
+	.attr =	{.name = #field, .mode = S_IRUGO },			\
+	.size = 0,							\
+	.read = field##_show,						\
+};
+
+nvme_bin_attr(uuid);
+
+static struct bin_attribute *nvme_ns_bin_attrs[] = {
+	&dev_attr_nvme_uuid,
+	NULL,
+};
+
+static struct attribute_group nvme_ns_attrs_group = {
+	.bin_attrs = nvme_ns_bin_attrs,
+};
+
 static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
 {
 	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
@@ -1068,9 +1111,16 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 	list_add_tail(&ns->list, &ctrl->namespaces);
 	kref_get(&ctrl->kref);
-	if (ns->type != NVME_NS_LIGHTNVM)
-		add_disk(ns->disk);
+	if (ns->type == NVME_NS_LIGHTNVM)
+		return;
 
+	add_disk(ns->disk);
+	if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
+			&nvme_ns_attrs_group)) {
+		del_gendisk(ns->disk);
+		nvme_put_ctrl(ctrl);
+		goto out_free_queue;
+	}
 	return;
  out_free_disk:
 	kfree(disk);
@@ -1090,6 +1140,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 	if (ns->disk->flags & GENHD_FL_UP) {
 		if (blk_get_integrity(ns->disk))
 			blk_integrity_unregister(ns->disk);
+		sysfs_remove_group(&disk_to_dev(ns->disk)->kobj, &nvme_ns_attrs_group);
 		del_gendisk(ns->disk);
 	}
 	if (kill || !blk_queue_dying(ns->queue)) {
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index b041762..4731205 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -102,6 +102,8 @@ struct nvme_ns {
 	struct gendisk *disk;
 	struct kref kref;
 
+	u8 uuid_len;
+	u8 uuid[16]; /* Either EUI64 or NGUID, if provided */
 	unsigned ns_id;
 	int lba_shift;
 	u16 ms;
-- 
2.6.2.307.g37023ba

[PATCH] NVMe: Expose namespace unique identifier to sysfs

[Matthew Wilcox]

On Tue, Dec 08, 2015@10:26:45AM -0700, Keith Busch wrote:
> A controller namespace supporting 1.1 or 1.2 capabilities uniquely
> identifies itself with either the 64-bit EUI or 128-bit NGUID.
> 
> This patch determines which the device supports and reports the unique
> identifier in new sysfs binary attribute "uuid". The attribute group is
> added to the gendisk's kobject directory.

I don't understand why we want to produce a binary attribute here instead
of a text attribute?  We already have nicely-formatted UUIDs in sysfs
(see the %pU specifier to printk)

I don't think we should have one attribute that might be an eui64 or might
be a uuid.  Maybe we should produce either an eui64 or a uuid attribute,
depending on which one the device reports?

> +	if (ns->ctrl->vs >= NVME_VS(1, 1)) {
> +		u8 *identifier = id->eui64;
> +		int len = sizeof(id->eui64);
> +
> +		if (bitmap_empty((void *)identifier, len * 8) &&
> +					ns->ctrl->vs >= NVME_VS(1, 2)) {
> +			identifier = id->nguid;
> +			len = sizeof(id->nguid);
> +		}

I'm a bit reluctant to use bitmap_empty here, because it's not actually
a bitmap.  We almost want the inverse of memchr ("find me the first
byte that is non-zero").  Maybe somebody else knows a better functoin
to call here?

> +	add_disk(ns->disk);
> +	if (sysfs_create_group(&disk_to_dev(ns->disk)->kobj,
> +			&nvme_ns_attrs_group)) {
> +		del_gendisk(ns->disk);
> +		nvme_put_ctrl(ctrl);
> +		goto out_free_queue;
> +	}
>  	return;
>   out_free_disk:
>  	kfree(disk);

An interesting philosophical point ... if creating the group fails,
should we really refuse to use the namespace?  It seems to me that we
can use it just fine.

[PATCH] NVMe: Expose namespace unique identifier to sysfs

[Keith Busch]

On Tue, Dec 08, 2015@01:53:25PM -0500, Matthew Wilcox wrote:
> On Tue, Dec 08, 2015@10:26:45AM -0700, Keith Busch wrote:
> > This patch determines which the device supports and reports the unique
> > identifier in new sysfs binary attribute "uuid". The attribute group is
> > added to the gendisk's kobject directory.
> 
> I don't understand why we want to produce a binary attribute here instead
> of a text attribute?  We already have nicely-formatted UUIDs in sysfs
> (see the %pU specifier to printk)

I don't have a strong opinion here. Just copying scsi vpd83 attribute,
and thought it's easier for a program to consume as binary, and easy to
read from the shell with hexdump.

> I don't think we should have one attribute that might be an eui64 or might
> be a uuid.  Maybe we should produce either an eui64 or a uuid attribute,
> depending on which one the device reports?

Sure, sounds reasonable.

These are supposed to be consumed by userspace to do something useful,
but I'm not getting much direction (I don't know why they insist on
these being available in sysfs instead of using existing ioctl).

I've no problem dynamically providing these based on device capabilities.
Hopefully the user space is okay with looking for the presense of two
different files.
 
> > +		if (bitmap_empty((void *)identifier, len * 8) &&
> 
> I'm a bit reluctant to use bitmap_empty here, because it's not actually
> a bitmap.  We almost want the inverse of memchr ("find me the first
> byte that is non-zero").  Maybe somebody else knows a better functoin
> to call here?

Better methods are learned all the time! It was nearly a year ago today
you suggested bitmap_empty for checking the very same fields. :)

  http://lists.infradead.org/pipermail/linux-nvme/2014-December/001349.html

[PATCH] NVMe: Expose namespace unique identifier to sysfs

[Jon Derrick]

> > I'm a bit reluctant to use bitmap_empty here, because it's not actually
> > a bitmap.  We almost want the inverse of memchr ("find me the first
> > byte that is non-zero").  Maybe somebody else knows a better functoin
> > to call here?
> 
> Better methods are learned all the time! It was nearly a year ago today
> you suggested bitmap_empty for checking the very same fields. :)
> 

How about memchr_inv? I think Matthew was alluding to that one

[PATCH] NVMe: Expose namespace unique identifier to sysfs

[Christoph Hellwig]

On Tue, Dec 08, 2015@07:15:55PM +0000, Keith Busch wrote:
> > I don't understand why we want to produce a binary attribute here instead
> > of a text attribute?  We already have nicely-formatted UUIDs in sysfs
> > (see the %pU specifier to printk)
> 
> I don't have a strong opinion here. Just copying scsi vpd83 attribute,
> and thought it's easier for a program to consume as binary, and easy to
> read from the shell with hexdump.

SCSI is a bad example.  James insisted we should not parse the EVPD
pages in kernel space for reasons no one agreed to (and now we'll need
to add that for in-kernel consumers anyway).  So unless some userspace
absolutely needs to do it the same way it might be better to offer a
ASCII representation.

> > I don't think we should have one attribute that might be an eui64 or might
> > be a uuid.  Maybe we should produce either an eui64 or a uuid attribute,
> > depending on which one the device reports?
> 
> Sure, sounds reasonable.
> 
> These are supposed to be consumed by userspace to do something useful,
> but I'm not getting much direction (I don't know why they insist on
> these being available in sysfs instead of using existing ioctl).

Using pass through ioctls from udev has been really painful from SCSI,
mostly in multipath environments or with flakey USB devices where we'd
need to extend a lot of magic done in the SCSI layer to those userspace
callers to get everything right.  NVMe is probably doing better in
that respect at the moment, but I'm not going to take any bets that
it's going to remain that way.

> I've no problem dynamically providing these based on device capabilities.
> Hopefully the user space is okay with looking for the presense of two
> different files.

An alternative might be to show the type of ID if we're using a text
format, but that sounds fairly ugly as well.

[PATCH] NVMe: Expose namespace unique identifier to sysfs

[Dan Williams]

On Tue, Dec 8, 2015@11:15 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Tue, Dec 08, 2015@01:53:25PM -0500, Matthew Wilcox wrote:
>> On Tue, Dec 08, 2015@10:26:45AM -0700, Keith Busch wrote:
>> > This patch determines which the device supports and reports the unique
>> > identifier in new sysfs binary attribute "uuid". The attribute group is
>> > added to the gendisk's kobject directory.
>>
>> I don't understand why we want to produce a binary attribute here instead
>> of a text attribute?  We already have nicely-formatted UUIDs in sysfs
>> (see the %pU specifier to printk)
>
> I don't have a strong opinion here. Just copying scsi vpd83 attribute,
> and thought it's easier for a program to consume as binary, and easy to
> read from the shell with hexdump.

Fwiw, the nvdimm subsystem dumps namespace uuids as text in sysfs, and
libuuid makes it trivial to convert back and forth.