[PATCH] nvmet: preserve controller serial number between reboots

[PATCH] nvmet: preserve controller serial number between reboots

[Johannes Thumshirn]

The NVMe target has no way to preserve controller serial
IDs across reboots which breaks udev scripts doing
SYMLINK+="dev/disk/by-id/nvme-$env{ID_SERIAL}-part%n.

Export the randomly generated serial number via configfs and allow
setting of a serial via configfs to mitigate this breakage.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/nvme/target/configfs.c | 22 ++++++++++++++++++++++
 drivers/nvme/target/core.c     |  8 ++++++--
 drivers/nvme/target/nvmet.h    |  1 +
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index a358ecd93e11..9f19f0bde8f2 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -686,9 +686,31 @@ static ssize_t nvmet_subsys_version_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, version);
 
+static ssize_t nvmet_subsys_attr_serial_show(struct config_item *item,
+					     char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%llx\n", subsys->serial);
+}
+
+static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
+					      const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	down_write(&nvmet_config_sem);
+	sscanf(page, "%llx\n", &subsys->serial);
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_version,
+	&nvmet_subsys_attr_attr_serial,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b5b4ac103748..6967580ffecf 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -767,8 +767,12 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	memcpy(ctrl->subsysnqn, subsysnqn, NVMF_NQN_SIZE);
 	memcpy(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE);
 
-	/* generate a random serial number as our controllers are ephemeral: */
-	get_random_bytes(&ctrl->serial, sizeof(ctrl->serial));
+	down_read(&nvmet_config_sem);
+	if (!subsys->serial)
+		get_random_bytes(&subsys->serial, sizeof(subsys->serial));
+	up_read(&nvmet_config_sem);
+
+	ctrl->serial = subsys->serial;
 
 	kref_init(&ctrl->ref);
 	ctrl->subsys = subsys;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 747bbdb4f9c6..68c97d73e387 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -152,6 +152,7 @@ struct nvmet_subsys {
 	u16			max_qid;
 
 	u64			ver;
+	u64			serial;
 	char			*subsysnqn;
 
 	struct config_group	group;
-- 
2.12.3

[PATCH] nvmet: preserve controller serial number between reboots

[Johannes Thumshirn]

The NVMe target has no way to preserve controller serial
IDs across reboots which breaks udev scripts doing
SYMLINK+="dev/disk/by-id/nvme-$env{ID_SERIAL}-part%n.

Export the randomly generated serial number via configfs and allow
setting of a serial via configfs to mitigate this breakage.

Signed-off-by: Johannes Thumshirn <jthumshirn at suse.de>
---
 drivers/nvme/target/configfs.c | 22 ++++++++++++++++++++++
 drivers/nvme/target/core.c     |  8 ++++++--
 drivers/nvme/target/nvmet.h    |  1 +
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index a358ecd93e11..9f19f0bde8f2 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -686,9 +686,31 @@ static ssize_t nvmet_subsys_version_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, version);
 
+static ssize_t nvmet_subsys_attr_serial_show(struct config_item *item,
+					     char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%llx\n", subsys->serial);
+}
+
+static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
+					      const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	down_write(&nvmet_config_sem);
+	sscanf(page, "%llx\n", &subsys->serial);
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_version,
+	&nvmet_subsys_attr_attr_serial,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index b5b4ac103748..6967580ffecf 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -767,8 +767,12 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	memcpy(ctrl->subsysnqn, subsysnqn, NVMF_NQN_SIZE);
 	memcpy(ctrl->hostnqn, hostnqn, NVMF_NQN_SIZE);
 
-	/* generate a random serial number as our controllers are ephemeral: */
-	get_random_bytes(&ctrl->serial, sizeof(ctrl->serial));
+	down_read(&nvmet_config_sem);
+	if (!subsys->serial)
+		get_random_bytes(&subsys->serial, sizeof(subsys->serial));
+	up_read(&nvmet_config_sem);
+
+	ctrl->serial = subsys->serial;
 
 	kref_init(&ctrl->ref);
 	ctrl->subsys = subsys;
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 747bbdb4f9c6..68c97d73e387 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -152,6 +152,7 @@ struct nvmet_subsys {
 	u16			max_qid;
 
 	u64			ver;
+	u64			serial;
 	char			*subsysnqn;
 
 	struct config_group	group;
-- 
2.12.3

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Sagi Grimberg]

> +static ssize_t nvmet_subsys_attr_serial_show(struct config_item *item,
> +					     char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%llx\n", subsys->serial);
> +}
> +
> +static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
> +					      const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	down_write(&nvmet_config_sem);
> +	sscanf(page, "%llx\n", &subsys->serial);
> +	up_write(&nvmet_config_sem);
> +
> +	return count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_serial);

It seems weird that a subsystem has a serial.

I'm not sure that a dynamic controller should maintain
a serial. Dynamic controllers by definition are allocated
on demand with no state of prior associations. But not sure
if a serial is a state (it probably isn't). The area is a little
fuzzy for me.

[PATCH] nvmet: preserve controller serial number between reboots

[Sagi Grimberg]

> +static ssize_t nvmet_subsys_attr_serial_show(struct config_item *item,
> +					     char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%llx\n", subsys->serial);
> +}
> +
> +static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
> +					      const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	down_write(&nvmet_config_sem);
> +	sscanf(page, "%llx\n", &subsys->serial);
> +	up_write(&nvmet_config_sem);
> +
> +	return count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_serial);

It seems weird that a subsystem has a serial.

I'm not sure that a dynamic controller should maintain
a serial. Dynamic controllers by definition are allocated
on demand with no state of prior associations. But not sure
if a serial is a state (it probably isn't). The area is a little
fuzzy for me.

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Johannes Thumshirn]

On Thu, Jul 13, 2017 at 03:30:39PM +0300, Sagi Grimberg wrote:
> It seems weird that a subsystem has a serial.

The subsystem is more of a hack I admit. But we don't maintain
configurations for controllers in configfs, do we?

> I'm not sure that a dynamic controller should maintain
> a serial. Dynamic controllers by definition are allocated
> on demand with no state of prior associations. But not sure
> if a serial is a state (it probably isn't). The area is a little
> fuzzy for me.

I'm not certain as well, the only thing I know for sure currently is,
it changes but we use in the standard 60-persistent-storage.rules [1]
as a part of /dev/disk/by-id/nvme-$model-$serial-part%n [2] and I have a bit
of a headace when users use it to identify their partitions in say /etc/fstab
and the link changes as the target generated serial changes.

Maybe we should consider this more as an RFD than a patch.

I'm happy to withdraw if we find a better solution.

[1] https://github.com/systemd/systemd/blob/master/rules/60-persistent-storage.rules
[2] https://github.com/systemd/systemd/blob/master/rules/60-persistent-storage.rules#L27

Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH] nvmet: preserve controller serial number between reboots

[Johannes Thumshirn]

On Thu, Jul 13, 2017@03:30:39PM +0300, Sagi Grimberg wrote:
> It seems weird that a subsystem has a serial.

The subsystem is more of a hack I admit. But we don't maintain
configurations for controllers in configfs, do we?

> I'm not sure that a dynamic controller should maintain
> a serial. Dynamic controllers by definition are allocated
> on demand with no state of prior associations. But not sure
> if a serial is a state (it probably isn't). The area is a little
> fuzzy for me.

I'm not certain as well, the only thing I know for sure currently is,
it changes but we use in the standard 60-persistent-storage.rules [1]
as a part of /dev/disk/by-id/nvme-$model-$serial-part%n [2] and I have a bit
of a headace when users use it to identify their partitions in say /etc/fstab
and the link changes as the target generated serial changes.

Maybe we should consider this more as an RFD than a patch.

I'm happy to withdraw if we find a better solution.

[1] https://github.com/systemd/systemd/blob/master/rules/60-persistent-storage.rules
[2] https://github.com/systemd/systemd/blob/master/rules/60-persistent-storage.rules#L27

Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 03:30:39PM +0300, Sagi Grimberg wrote:
> It seems weird that a subsystem has a serial.

But that's actually how NVMe defines them.  Which mean we first
need to fix our code to generate a serial number per subsystem,
and on top of that the patch from Johannes seems perfectly reasonable.

[PATCH] nvmet: preserve controller serial number between reboots

[Christoph Hellwig]

On Thu, Jul 13, 2017@03:30:39PM +0300, Sagi Grimberg wrote:
> It seems weird that a subsystem has a serial.

But that's actually how NVMe defines them.  Which mean we first
need to fix our code to generate a serial number per subsystem,
and on top of that the patch from Johannes seems perfectly reasonable.

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Johannes Thumshirn]

On Thu, Jul 13, 2017 at 03:06:52PM +0200, Christoph Hellwig wrote:
> On Thu, Jul 13, 2017 at 03:30:39PM +0300, Sagi Grimberg wrote:
> > It seems weird that a subsystem has a serial.
> 
> But that's actually how NVMe defines them.  Which mean we first
> need to fix our code to generate a serial number per subsystem,
> and on top of that the patch from Johannes seems perfectly reasonable.

Well no problem, I can easily add that.

-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH] nvmet: preserve controller serial number between reboots

[Johannes Thumshirn]

On Thu, Jul 13, 2017@03:06:52PM +0200, Christoph Hellwig wrote:
> On Thu, Jul 13, 2017@03:30:39PM +0300, Sagi Grimberg wrote:
> > It seems weird that a subsystem has a serial.
> 
> But that's actually how NVMe defines them.  Which mean we first
> need to fix our code to generate a serial number per subsystem,
> and on top of that the patch from Johannes seems perfectly reasonable.

Well no problem, I can easily add that.

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Martin Wilck]

On Thu, 2017-07-13 at 12:48 +0200, Johannes Thumshirn wrote:
> The NVMe target has no way to preserve controller serial
> IDs across reboots which breaks udev scripts doing
> SYMLINK+="dev/disk/by-id/nvme-$env{ID_SERIAL}-part%n.
> 
> Export the randomly generated serial number via configfs and allow
> setting of a serial via configfs to mitigate this breakage.

I'm wondering if this should be a write-once attribute. Also,
Once the serial number has been passed on to some host (or maybe only:
while the device is in use by some host), the attribute should probably
be read-only.

Martin

[PATCH] nvmet: preserve controller serial number between reboots

[Martin Wilck]

On Thu, 2017-07-13@12:48 +0200, Johannes Thumshirn wrote:
> The NVMe target has no way to preserve controller serial
> IDs across reboots which breaks udev scripts doing
> SYMLINK+="dev/disk/by-id/nvme-$env{ID_SERIAL}-part%n.
> 
> Export the randomly generated serial number via configfs and allow
> setting of a serial via configfs to mitigate this breakage.

I'm wondering if this should be a write-once attribute. Also,
Once the serial number has been passed on to some host (or maybe only:
while the device is in use by some host), the attribute should probably
be read-only.

Martin

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 05:08:25PM +0200, Martin Wilck wrote:
> I'm wondering if this should be a write-once attribute. Also,
> Once the serial number has been passed on to some host (or maybe only:
> while the device is in use by some host), the attribute should probably
> be read-only.

In practive it should.  But I don't think it's worth the extra code
just to prevent people from shooting themselves into the foot.

[PATCH] nvmet: preserve controller serial number between reboots

[Christoph Hellwig]

On Thu, Jul 13, 2017@05:08:25PM +0200, Martin Wilck wrote:
> I'm wondering if this should be a write-once attribute. Also,
> Once the serial number has been passed on to some host (or maybe only:
> while the device is in use by some host), the attribute should probably
> be read-only.

In practive it should.  But I don't think it's worth the extra code
just to prevent people from shooting themselves into the foot.

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Sagi Grimberg]

>> It seems weird that a subsystem has a serial.
> 
> But that's actually how NVMe defines them.

Where is that wording?

> Which mean we first
> need to fix our code to generate a serial number per subsystem,
> and on top of that the patch from Johannes seems perfectly reasonable.

If that is indeed the case then sure.

[PATCH] nvmet: preserve controller serial number between reboots

[Sagi Grimberg]

>> It seems weird that a subsystem has a serial.
> 
> But that's actually how NVMe defines them.

Where is that wording?

> Which mean we first
> need to fix our code to generate a serial number per subsystem,
> and on top of that the patch from Johannes seems perfectly reasonable.

If that is indeed the case then sure.

Re: [PATCH] nvmet: preserve controller serial number between reboots

[Christoph Hellwig]

On Thu, Jul 13, 2017 at 07:38:27PM +0300, Sagi Grimberg wrote:
>
>>> It seems weird that a subsystem has a serial.
>>
>> But that's actually how NVMe defines them.
>
> Where is that wording?

Right there in the SN field description:

"Serial Number (SN): Contains the serial number for the NVM subsystem
 that is assigned by the vendor as an ASCII string. Refer to section
 7.10 for unique identifier requirements. Refer to section 1.5 for
 ASCII string requirements"

[PATCH] nvmet: preserve controller serial number between reboots

[Christoph Hellwig]

On Thu, Jul 13, 2017@07:38:27PM +0300, Sagi Grimberg wrote:
>
>>> It seems weird that a subsystem has a serial.
>>
>> But that's actually how NVMe defines them.
>
> Where is that wording?

Right there in the SN field description:

"Serial Number (SN): Contains the serial number for the NVM subsystem
 that is assigned by the vendor as an ASCII string. Refer to section
 7.10 for unique identifier requirements. Refer to section 1.5 for
 ASCII string requirements"