'modprobe nvme_core multipath=N' crashes in face of multipath fabric

This isn't new since the 4.17 merge or anything, I first noticed this issue existed while using a 4.16-rc4 kernel.

modprobe nvme_core multipath=N

Using mptest's nvme_4port_create.sh

git clone git://github.com/snitm/mptest.git
cd mptest/lib/unittests
perl -pi -e 's|/dev/pmem0|/dev/some_test_device|' nvme_4port_create.sh ./nvme_4port_create.sh

[ 2309.845915] nvmet: adding nsid 1 to subsystem mptestnqn
[ 2310.019820] nvmet: creating controller 1 for subsystem mptestnqn for NQN nqn.2014-08.org.nvmexpress:uuid:71a69a69-edb9-4d51-8cbf-1c29778534ab.
[ 2310.034201] nvme nvme1: NVME-FC{0}: new ctrl: NQN "mptestnqn"
[ 2310.041705] nvmet: creating controller 2 for subsystem mptestnqn for NQN nqn.2014-08.org.nvmexpress:uuid:71a69a69-edb9-4d51-8cbf-1c29778534ab.
[ 2310.056005] nvme nvme2: NVME-FC{1}: new ctrl: NQN "mptestnqn"
[ 2310.056314] sysfs: cannot create duplicate filename '/class/block/nvme1n1'
[ 2310.063386] nvmet: creating controller 3 for subsystem mptestnqn for NQN nqn.2014-08.org.nvmexpress:uuid:71a69a69-edb9-4d51-8cbf-1c29778534ab.
[ 2310.068637] CPU: 3 PID: 1791 Comm: kworker/u497:1 Not tainted 4.16.0.snitm+ #13
[ 2310.088705] Hardware name: Supermicro SYS-1029P-WTR/X11DDW-L, BIOS 2.0a 12/06/2017
[ 2310.096274] Workqueue: nvme-wq nvme_scan_work [nvme_core]
[ 2310.101669] Call Trace:
[ 2310.104129]  dump_stack+0x5a/0x73
[ 2310.107444]  sysfs_warn_dup+0x58/0x70
[ 2310.111109]  sysfs_do_create_link_sd.isra.2+0xa3/0xb0
[ 2310.116162]  device_add+0x2ac/0x5f0
[ 2310.119658]  __device_add_disk+0x19c/0x4a0
[ 2310.123754]  nvme_validate_ns+0x4d2/0x860 [nvme_core]
[ 2310.128807]  ? wake_up_q+0x70/0x70
[ 2310.132214]  nvme_scan_work+0x211/0x2d0 [nvme_core]
[ 2310.137095]  process_one_work+0x158/0x360
[ 2310.141103]  worker_thread+0x47/0x3e0
[ 2310.144772]  kthread+0xf8/0x130
[ 2310.147917]  ? max_active_store+0x80/0x80
[ 2310.151929]  ? kthread_bind+0x10/0x10
[ 2310.155595]  ? do_syscall_64+0x74/0x1a0
[ 2310.159435]  ? SyS_exit_group+0x10/0x10
[ 2310.163273]  ret_from_fork+0x35/0x40
[ 2310.166872] nvme nvme3: NVME-FC{2}: new ctrl: NQN "mptestnqn"
[ 2310.166875] ------------[ cut here ]------------
[ 2310.167189] sysfs: cannot create duplicate filename '/class/block/nvme1n1'
[ 2310.167190] CPU: 3 PID: 8111 Comm: kworker/u497:3 Not tainted 4.16.0.snitm+ #13
[ 2310.167191] Hardware name: Supermicro SYS-1029P-WTR/X11DDW-L, BIOS 2.0a 12/06/2017
[ 2310.167194] Workqueue: nvme-wq nvme_scan_work [nvme_core]
[ 2310.167194] Call Trace:
[ 2310.167196]  dump_stack+0x5a/0x73
[ 2310.167198]  sysfs_warn_dup+0x58/0x70
[ 2310.167199]  sysfs_do_create_link_sd.isra.2+0xa3/0xb0
[ 2310.167200]  device_add+0x2ac/0x5f0
[ 2310.167202]  __device_add_disk+0x19c/0x4a0
[ 2310.167205]  nvme_validate_ns+0x4d2/0x860 [nvme_core]
[ 2310.167206]  ? wake_up_q+0x70/0x70
[ 2310.167208]  nvme_scan_work+0x211/0x2d0 [nvme_core]
[ 2310.167210]  process_one_work+0x158/0x360
[ 2310.167211]  worker_thread+0x47/0x3e0
[ 2310.167212]  kthread+0xf8/0x130
[ 2310.167213]  ? max_active_store+0x80/0x80
[ 2310.167214]  ? kthread_bind+0x10/0x10
[ 2310.167215]  ret_from_fork+0x35/0x40
[ 2310.167225] ------------[ cut here ]------------
[ 2310.167226] kernel BUG at fs/sysfs/group.c:111!

Thanks for the notice.

There is definitely a bug here when CONFIG_NVME_MULTIPATH=y but nvme_core multipath is disabled in the presence of shared namespaces. I think we'd need each namespace to get a different "head" out of the subsystem in this case, but it may take a moment for me to detangle this.

No problem, it certainly isn't something I could tackle any quicker ;)

Thanks for looking to resolve this though.

It doesn't look like we'll be able to allocate new namespace 'heads' here without complicating this even more.

The below should fix the naming collision by getting new instances for each namespace that attaches to a head. I'm not sure this is much better, but maybe Christoph will have a better suggestion.

---

index 38b450ec00eb..683ccaa70cdc 100644
@@ -350,6 +350,7 @@ static void nvme_free_ns_head(struct kref *ref)
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	list_del_init(&head->entry);
 	cleanup_srcu_struct(&head->srcu);
+	ida_destroy(&head->ns_ida);
 	kfree(head);
 }
 

@@ -366,6 +367,7 @@ static void nvme_free_ns(struct kref *kref)
 		nvme_nvm_unregister(ns);
 
 	put_disk(ns->disk);
+	ida_simple_remove(&ns->head->ns_ida, ns->instance);
 	nvme_put_ns_head(ns->head);
 	nvme_put_ctrl(ns->ctrl);
 	kfree(ns);

@@ -2836,6 +2838,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	head->subsys = ctrl->subsys;
 	head->ns_id = nsid;
 	kref_init(&head->ref);
+	ida_init(&head->ns_ida);
 
 	nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
 

@@ -2856,6 +2859,7 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	cleanup_srcu_struct(&head->srcu);
 out_ida_remove:
 	ida_simple_remove(&ctrl->subsys->ns_ida, head->instance);
+	ida_destroy(&head->ns_ida);
 out_free_head:
 	kfree(head);
 out:

@@ -2895,6 +2899,12 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 	list_add_tail(&ns->siblings, &head->list);
 	ns->head = head;
 
+	ret = ida_simple_get(&head->ns_ida, 1, 0, GFP_KERNEL);
+	if (ret >= 0) {
+		ns->instance = ret;
+		ret = 0;
+	}
+
 out_unlock:
 	mutex_unlock(&ctrl->subsys->lock);
 	return ret;

@@ -3003,7 +3013,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 		flags = GENHD_FL_HIDDEN;
 	} else {
 		sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance,
-				ns->head->instance);
+				ns->instance);
 	}
 #else
 	/*

@@ -3059,6 +3069,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	mutex_lock(&ctrl->subsys->lock);
 	list_del_rcu(&ns->siblings);
 	mutex_unlock(&ctrl->subsys->lock);
+	ida_simple_remove(&ns->head->ns_ida, ns->instance);
  out_free_id:
 	kfree(id);
  out_free_queue:

index ced5b3d3269d..3f95b2f7ecf3 100644
@@ -252,6 +252,7 @@ struct nvme_ns_head {
 	spinlock_t		requeue_lock;
 	struct work_struct	requeue_work;
 #endif
+	struct ida		ns_ida;
 	struct list_head	list;
 	struct srcu_struct      srcu;
 	struct nvme_subsystem	*subsys;

@@ -282,6 +283,7 @@ struct nvme_ns {
 	struct kref kref;
 	struct nvme_ns_head *head;
 
+	int instance;
 	int lba_shift;
 	u16 ms;
 	u16 sgs;

--

This patch fixed the issue for me. Verified that modprobe nvme_core multipath=N and multipath=Y works with the mptest case I shared in my original report.

If you do go with this patch, please feel free to add:

Thanks,
Mike

This looks good to me too, you can add my

I'm going to have to make one minor change here since this will still break if you've a multi-path subsystem with multiple namespaces. I'll just need to move the IDA up to the subsystem level instead of on the subsystem heads, but otherwise much the same.