[PATCH 0/2] nvme multipath naming fixes

[PATCH 0/2] nvme multipath naming fixes

[Keith Busch]

This is the update to the mpath naming conclict as discussed in thread:

http://lists.infradead.org/pipermail/linux-nvme/2018-April/016820.html

Keith Busch (2):
  nvme/multipath: Disable runtime writable enabling parameter
  nvme/multipath: Fix multipath disabled naming collisions

 drivers/nvme/host/core.c      | 26 +-------------------------
 drivers/nvme/host/multipath.c | 24 +++++++++++++++++++++++-
 drivers/nvme/host/nvme.h      | 12 ++++++++++++
 3 files changed, 36 insertions(+), 26 deletions(-)

-- 
2.14.3

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Keith Busch]

We can't allow the user to change multipath settings at runtime, as this
will create naming conflicts due to the different naming schemes used
for each mode.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/multipath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 956e0b8e9c4d..3ded009e7631 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -15,7 +15,7 @@
 #include "nvme.h"
 
 static bool multipath = true;
-module_param(multipath, bool, 0644);
+module_param(multipath, bool, 0444);
 MODULE_PARM_DESC(multipath,
 	"turn on native support for multiple controllers per subsystem");
 
-- 
2.14.3

[PATCH 2/2] nvme/multipath: Fix multipath disabled naming collisions

[Keith Busch]

When CONFIG_NVME_MULTIPATH is set, but we're not using nvme to multipath,
namespaces with multiple paths were not creating unique names due to
reusing the same instance number from the namespace's head.

This patch fixes this by falling back to the non-multipath naming method
when the parameter disabled using multipath.

Reported-by: Mike Snitzer <snitzer at redhat.com>
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c      | 26 +-------------------------
 drivers/nvme/host/multipath.c | 22 ++++++++++++++++++++++
 drivers/nvme/host/nvme.h      | 12 ++++++++++++
 3 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 127a9cbf3314..a3771c5729f5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2998,31 +2998,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	if (nvme_init_ns_head(ns, nsid, id))
 		goto out_free_id;
 	nvme_setup_streams_ns(ctrl, ns);
-	
-#ifdef CONFIG_NVME_MULTIPATH
-	/*
-	 * If multipathing is enabled we need to always use the subsystem
-	 * instance number for numbering our devices to avoid conflicts
-	 * between subsystems that have multiple controllers and thus use
-	 * the multipath-aware subsystem node and those that have a single
-	 * controller and use the controller node directly.
-	 */
-	if (ns->head->disk) {
-		sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
-				ctrl->cntlid, ns->head->instance);
-		flags = GENHD_FL_HIDDEN;
-	} else {
-		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
-				ns->head->instance);
-	}
-#else
-	/*
-	 * But without the multipath code enabled, multiple controller per
-	 * subsystems are visible as devices and thus we cannot use the
-	 * subsystem instance.
-	 */
-	sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
-#endif
+	nvme_set_disk_name(disk_name, ns, ctrl, &flags);
 
 	if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) {
 		if (nvme_nvm_register(ns, disk_name, node)) {
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 3ded009e7631..d7b664ae5923 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -19,6 +19,28 @@ module_param(multipath, bool, 0444);
 MODULE_PARM_DESC(multipath,
 	"turn on native support for multiple controllers per subsystem");
 
+/*
+ * If multipathing is enabled we need to always use the subsystem instance
+ * number for numbering our devices to avoid conflicts between subsystems that
+ * have multiple controllers and thus use the multipath-aware subsystem node
+ * and those that have a single controller and use the controller node
+ * directly.
+ */
+void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
+			struct nvme_ctrl *ctrl, int *flags)
+{
+	if (!multipath) {
+		sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
+	} else if (ns->head->disk) {
+		sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance,
+				ctrl->cntlid, ns->head->instance);
+		*flags = GENHD_FL_HIDDEN;
+	} else {
+		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
+				ns->head->instance);
+	}
+}
+
 void nvme_failover_req(struct request *req)
 {
 	struct nvme_ns *ns = req->q->queuedata;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 061fecfd44f5..7ded7a51c430 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -436,6 +436,8 @@ extern const struct attribute_group nvme_ns_id_attr_group;
 extern const struct block_device_operations nvme_ns_head_ops;
 
 #ifdef CONFIG_NVME_MULTIPATH
+void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
+			struct nvme_ctrl *ctrl, int *flags);
 void nvme_failover_req(struct request *req);
 bool nvme_req_needs_failover(struct request *req, blk_status_t error);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
@@ -461,6 +463,16 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
 }
 
 #else
+/*
+ * Without the multipath code enabled, multiple controller per subsystems are
+ * visible as devices and thus we cannot use the subsystem instance.
+ */
+static inline void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns,
+				      struct nvme_ctrl *ctrl, int *flags)
+{
+	sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance);
+}
+
 static inline void nvme_failover_req(struct request *req)
 {
 }
-- 
2.14.3

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Christoph Hellwig]

Looks good,

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

[PATCH 2/2] nvme/multipath: Fix multipath disabled naming collisions

[Christoph Hellwig]

On Thu, Apr 26, 2018@03:49:56PM -0600, Keith Busch wrote:
> When CONFIG_NVME_MULTIPATH is set, but we're not using nvme to multipath,
> namespaces with multiple paths were not creating unique names due to
> reusing the same instance number from the namespace's head.
> 
> This patch fixes this by falling back to the non-multipath naming method
> when the parameter disabled using multipath.
> 
> Reported-by: Mike Snitzer <snitzer at redhat.com>
> Signed-off-by: Keith Busch <keith.busch at intel.com>

Looks good,

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

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Keith Busch]

Applied both to nvme-4.17-rc3.

Just FYI, I plan to send a pull request by end of today. I know there's
other pathches that may be 4.17 material still under review, and that
may need to wait for the next round.

[PATCH 2/2] nvme/multipath: Fix multipath disabled naming collisions

[Stephen Bates]

>    When CONFIG_NVME_MULTIPATH is set, but we're not using nvme to multipath,
>    namespaces with multiple paths were not creating unique names due to
>    reusing the same instance number from the namespace's head.
>    
>    This patch fixes this by falling back to the non-multipath naming method
>    when the parameter disabled using multipath.
    
Hi All

Andrew and I have been doing some testing on systems with quite a few NVMe controllers in them. Some of these have one namespace and some have multiple. We (by mistake) had CONFIG_NVME_MULTIPATH enabled in the .config and noticed some unusual naming in the sysfs tree. Basically the namespace names no longer align with the controller names. This leads to hilarity as we try to co-ordinate IO between the namespaces and try to work out what namespace is where in the system. For example on a QEMU system with six NVMe SSD models in it we get something like:

root at donard-qemu:~# ls -la /sys/bus/pci/devices/0000\:00\:04.0/nvme/nvme0/
total 0
drwxr-xr-x 4 root root    0 Oct 29 15:32 .
drwxr-xr-x 3 root root    0 Oct 29 15:32 ..
-r--r--r-- 1 root root 4096 Oct 29 15:32 address
-r--r--r-- 1 root root 4096 Oct 29 15:32 cntlid
-r--r--r-- 1 root root 4096 Oct 29 15:32 dev
lrwxrwxrwx 1 root root    0 Oct 29 15:32 device -> ../../../0000:00:04.0
-r--r--r-- 1 root root 4096 Oct 29 15:32 firmware_rev
-r--r--r-- 1 root root 4096 Oct 29 15:32 model
drwxr-xr-x 9 root root    0 Oct 29 15:32 nvme1n1
drwxr-xr-x 2 root root    0 Oct 29 15:32 power
--w------- 1 root root 4096 Oct 29 15:32 rescan_controller
--w------- 1 root root 4096 Oct 29 15:32 reset_controller
-r--r--r-- 1 root root 4096 Oct 29 15:32 serial
-r--r--r-- 1 root root 4096 Oct 29 15:32 state
-r--r--r-- 1 root root 4096 Oct 29 15:32 subsysnqn
lrwxrwxrwx 1 root root    0 Oct 29 15:32 subsystem -> ../../../../../class/nvme
-r--r--r-- 1 root root 4096 Oct 29 15:32 transport
-rw-r--r-- 1 root root 4096 Oct 29 15:32 uevent

Note how the /nvme/nvme0/ sysfs directory contains the nvme1n1 namespace. Is this expected? I am not that familiar with the multipath code so I'm not sure if this is a bug or not but it sure is confusing. Disabling CONFIG_NVME_MULTIPATH reverts to the legacy naming convention. This happens in all the 4.19, 4.18 and 4.17 kernels we tested. I've not tried the for-next tree.

Stephen

    

[PATCH 2/2] nvme/multipath: Fix multipath disabled naming collisions

[Keith Busch]

On Mon, Oct 29, 2018@03:37:46PM +0000, Stephen  Bates wrote:
> Note how the /nvme/nvme0/ sysfs directory contains the nvme1n1
> namespace. Is this expected? I am not that familiar with the multipath
> code so I'm not sure if this is a bug or not but it sure is confusing.
> Disabling CONFIG_NVME_MULTIPATH reverts to the legacy naming
> convention. This happens in all the 4.19, 4.18 and 4.17 kernels we
> tested. I've not tried the for-next tree.

That is expected. We still need character devices to each controller path
for administration, but we don't need block devices to each namespace
path, so the numbers in the controller and block handles are independent
of each other.

[PATCH 2/2] nvme/multipath: Fix multipath disabled naming collisions

[Stephen Bates]

> > Note how the /nvme/nvme0/ sysfs directory contains the nvme1n1
> > namespace. Is this expected? I am not that familiar with the multipath
> > code so I'm not sure if this is a bug or not but it sure is confusing.
> > Disabling CONFIG_NVME_MULTIPATH reverts to the legacy naming
> > convention. This happens in all the 4.19, 4.18 and 4.17 kernels we
> > tested. I've not tried the for-next tree.
>    
>    That is expected. We still need character devices to each controller path
>    for administration, but we don't need block devices to each namespace
>    path, so the numbers in the controller and block handles are independent
>    of each other.

Keith

Ooooookay. Thanks for letting us know!

Stephen
    

[PATCH 2/2] nvme/multipath: Fix multipath disabled naming collisions

[Keith Busch]

On Mon, Oct 29, 2018@03:48:52PM +0000, Stephen  Bates wrote:
> > > Note how the /nvme/nvme0/ sysfs directory contains the nvme1n1
> > > namespace. Is this expected? I am not that familiar with the multipath
> > > code so I'm not sure if this is a bug or not but it sure is confusing.
> > > Disabling CONFIG_NVME_MULTIPATH reverts to the legacy naming
> > > convention. This happens in all the 4.19, 4.18 and 4.17 kernels we
> > > tested. I've not tried the for-next tree.
> >    
> >    That is expected. We still need character devices to each controller path
> >    for administration, but we don't need block devices to each namespace
> >    path, so the numbers in the controller and block handles are independent
> >    of each other.
> 
> Keith
> 
> Ooooookay. Thanks for letting us know!

Note the naming convention that's causing confusion applies to the
visible namespace paths. The hidden paths have naming convention like
"nvme<X>c<Y>n<Z>", and that <Y> value matches up the the controller
handle at /dev/nvme<Y>.

That doesn't do you any good if you're looking in /dev/ since those
paths are hidden, but the topology can be observed in /sys/.

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Sagi Grimberg]

> Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

Ignore that :)

My mailer glitched...

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Keith Busch]

On Tue, Oct 30, 2018@11:18:03AM -0700, Sagi Grimberg wrote:
> 
> > Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
> 
> Ignore that :)
> 
> My mailer glitched...

Everything on the nvme mailing list is out of whack for me. It looks
like lists.infradead.org is toast. I can still ssh to git.infradead.org,
but can't access its gitweb.

[PATCH 1/2] nvme/multipath: Disable runtime writable enabling parameter

[Christoph Hellwig]

On Tue, Oct 30, 2018@12:24:56PM -0600, Keith Busch wrote:
> Everything on the nvme mailing list is out of whack for me. It looks
> like lists.infradead.org is toast. I can still ssh to git.infradead.org,
> but can't access its gitweb.

Even the mail part didn't work yesterday Israeli time, so it might just
take some more time to be back up.