Small rework of nvme_init/uninit_ctrl

The fact that nvme_free_ctrl cleans up after nvme_init_ctrl istead of nvme_uninit_ctrl is causing us to rely on non-trivial ctrl refcounting inside the fabrics ->create_ctrl handlers.

Instead, have nvme_uninit_ctrl cleanup nvme_init_ctrl and fixup rdma and loop ->create_ctrl error sequence to be sane.

This will also help when we will consolidate more code to our common nvme-core.

nvme-pci does not seem to rely on the former behavior and thus does not need to be fixed afaict. It seems that nvme-fc always prefers to use the ctrl refcount scheme to cleanup the controller (also from ->create_ctrl) but it always calls nvme_uninit_ctrl before the final put so looks like it should work as well.

Tested rdma, loop and pci but didn't test fc.

Roy Shterman (5):
nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric
nvme-rdma: rework create_ctrl error flow
nvme-rdma: remove redundant check if the ctrl is in the ctrl when
freeing it
nvme-loop: rework create_ctrl error flow
nvme-loop: remove redundant check if the ctrl is in the ctrl when
freeing it

 drivers/nvme/host/core.c   | 11 +++++------
 drivers/nvme/host/rdma.c   | 28 +++++++++++-----------------
 drivers/nvme/target/loop.c | 30 ++++++++++++------------------
 3 files changed, 28 insertions(+), 41 deletions(-)

2.7.4

From: Roy Shterman <[email protected]>

Its strange to have some of the cleanup for nvme_init_ctrl in nvme_free_ctrl. This cause us to have some magic ctrl refcount handling in controller initialization error paths.

Make the init/uninit symmetric so we can have a sane and maintainable error sequences.

---
 drivers/nvme/host/core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

index c95155696741..b8abd016202e 100644
@@ -2749,11 +2749,14 @@ EXPORT_SYMBOL_GPL(nvme_start_ctrl);
 
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl)
 {
-	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
-
 	spin_lock(&dev_list_lock);
 	list_del(&ctrl->node);
 	spin_unlock(&dev_list_lock);
+
+	ida_destroy(&ctrl->ns_ida);
+	put_device(ctrl->device);
+	device_destroy(nvme_class, MKDEV(nvme_char_major, ctrl->instance));
+	nvme_release_instance(ctrl);
 }
 EXPORT_SYMBOL_GPL(nvme_uninit_ctrl);
 

@@ -2761,10 +2764,6 @@ static void nvme_free_ctrl(struct kref *kref)
 {
 	struct nvme_ctrl *ctrl = container_of(kref, struct nvme_ctrl, kref);
 
-	put_device(ctrl->device);
-	nvme_release_instance(ctrl);
-	ida_destroy(&ctrl->ns_ida);
-
 	ctrl->ops->free_ctrl(ctrl);
 }
 

2.7.4

It's not magic. It's just preventing name reuse when some management code retains an open handle after the device was removed. Otherwise you're going to get naming clashes when you bring up a new device with the same name from reusing the still in-use instance.

management code as in nvme-cli?

Can userspace keep an open-handle after device_destroy() was invoked?

Can you explain how would I see naming clashes? I'm not sure I understand what you are referring to.

It's been a while since I tested this. I used to observe -EEXIST errors setting up the kobj's from something as simple as this:

dd if=/dev/nvme0n1 of=/dev/null conv=noerror 2> /dev/null & echo 1 > /sys/class/nvme/nvme0/device/remove
echo 1 > /sys/bus/pci/rescan

The rescan would recreate nvme0n1 that still exists from the open handle. I'm not seeing an error today; not sure what changed, but I'm looking into that now.

Hm, I'm not hitting duplicate name issues anymore. I can't be bothered to bisect when that started working, but I think the dev_t lifetime fixes has something to do with it.

Anyway, simpler is certainly preferred. This is looking okay so far, and I'll try a few more things to see if I can break it.

Thanks Keith,

Would be good to get your review/tested tag if it survives your tests.

This totally conflicts with my refcount / device rework in the mpath patchset..

I know...

Do you want me to rebase on top of your next round of patches?

ping?

Sure. It might also make sense to queue up the device / refcount updates from th mpath series to nvme-4.15 ASAP so you only have to base on top of those.

From: Roy Shterman <[email protected]>

The magic ctrl refcount handling we have due to the asymmetric behavior in nvme_init/uninit_ctrl is making the controller creation error sequence a bit awkward and error prone as we rely on magic refcounting to invoke ->free_ctrl from create_ctrl error sequence.

Now that we have nvme_init/uninit_ctrl doing the right thing, we can fix this by making the error sequence sane and simple (just undo what we did until the error).

Also, allocate ctrl->queues before calling nvme_init_ctrl, just to separate the driver private resource allocation and the core resource allocations.

---
 drivers/nvme/host/rdma.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

index 605586b88186..866abee2374d 100644
@@ -1875,11 +1875,6 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		}
 	}
 
-	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_rdma_ctrl_ops,
-				0 /* no quirks, we're perfect! */);
-	if (ret)
-		goto out_free_ctrl;
-
 	INIT_DELAYED_WORK(&ctrl->reconnect_work,
 			nvme_rdma_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);

@@ -1894,11 +1889,16 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	ctrl->queues = kcalloc(ctrl->ctrl.queue_count, sizeof(*ctrl->queues),
 				GFP_KERNEL);
 	if (!ctrl->queues)
-		goto out_uninit_ctrl;
+		goto out_free_ctrl;
+
+	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_rdma_ctrl_ops,
+				0 /* no quirks, we're perfect! */);
+	if (ret)
+		goto out_free_queues;
 
 	ret = nvme_rdma_configure_admin_queue(ctrl, true);
 	if (ret)
-		goto out_kfree_queues;
+		goto out_uninit_ctrl;
 
 	/* sanity check icdoff */
 	if (ctrl->ctrl.icdoff) {

@@ -1954,16 +1954,14 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 
 out_remove_admin_queue:
 	nvme_rdma_destroy_admin_queue(ctrl, true);
-out_kfree_queues:
-	kfree(ctrl->queues);
 out_uninit_ctrl:
 	nvme_uninit_ctrl(&ctrl->ctrl);
-	nvme_put_ctrl(&ctrl->ctrl);
-	if (ret > 0)
-		ret = -EIO;
-	return ERR_PTR(ret);
+out_free_queues:
+	kfree(ctrl->queues);
 out_free_ctrl:
 	kfree(ctrl);
+	if (ret > 0)
+		ret = -EIO;
 	return ERR_PTR(ret);
 }
 

2.7.4

Once we did the init we'll have to do a put and not just free it. That will become even more important when using a struct device for refcounting the controller.

:(

I want to get to a point where the transports allocate everything they need in their private struct, and then do nvme common things. This asymmetric behavior is going hard to keep correct once got right.

From: Roy Shterman <[email protected]>

Its no longer acceptable now that we do not invoke ->free_ctrl from inside ->create_ctrl.

---
 drivers/nvme/host/rdma.c | 4 ----
 1 file changed, 4 deletions(-)

index 866abee2374d..03a3f0372450 100644
@@ -875,16 +875,12 @@ static void nvme_rdma_free_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_rdma_ctrl *ctrl = to_rdma_ctrl(nctrl);
 
-	if (list_empty(&ctrl->list))
-		goto free_ctrl;
-
 	mutex_lock(&nvme_rdma_ctrl_mutex);
 	list_del(&ctrl->list);
 	mutex_unlock(&nvme_rdma_ctrl_mutex);
 
 	kfree(ctrl->queues);
 	nvmf_free_options(nctrl->opts);
-free_ctrl:
 	kfree(ctrl);
 }
 

2.7.4

From: Roy Shterman <[email protected]>

The magic ctrl refcount handling we have due to the asymmetric behavior in nvme_init/uninit_ctrl is making the controller creation error sequence a bit awkward and error prone as we rely on magic refcounting to invoke ->free_ctrl from create_ctrl error sequence.

Now that we have nvme_init/uninit_ctrl doing the right thing, we can fix this by making the error sequence sane and simple (just undo what we did until the error).

Also, allocate ctrl->queues before calling nvme_init_ctrl, just to separate the driver private resource allocation and the core resource allocations.

---
 drivers/nvme/target/loop.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

index 92628c432926..cea6bab1e70c 100644
@@ -602,26 +602,24 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 
 	INIT_WORK(&ctrl->delete_work, nvme_loop_del_ctrl_work);
 	INIT_WORK(&ctrl->ctrl.reset_work, nvme_loop_reset_ctrl_work);
-
-	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_loop_ctrl_ops,
-				0 /* no quirks, we're perfect! */);
-	if (ret)
-		goto out_put_ctrl;
-
-	ret = -ENOMEM;
-
 	ctrl->ctrl.sqsize = opts->queue_size - 1;
 	ctrl->ctrl.kato = opts->kato;
 
+	ret = -ENOMEM;
 	ctrl->queues = kcalloc(opts->nr_io_queues + 1, sizeof(*ctrl->queues),
 			GFP_KERNEL);
 	if (!ctrl->queues)
-		goto out_uninit_ctrl;
+		goto out_free_ctrl;
 
-	ret = nvme_loop_configure_admin_queue(ctrl);
+	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_loop_ctrl_ops,
+				0 /* no quirks, we're perfect! */);
 	if (ret)
 		goto out_free_queues;
 
+	ret = nvme_loop_configure_admin_queue(ctrl);
+	if (ret)
+		goto out_uninit_ctrl;
+
 	if (opts->queue_size > ctrl->ctrl.maxcmd) {
 		/* warn if maxcmd is lower than queue_size */
 		dev_warn(ctrl->ctrl.device,

@@ -656,12 +654,12 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 
 out_remove_admin_queue:
 	nvme_loop_destroy_admin_queue(ctrl);
-out_free_queues:
-	kfree(ctrl->queues);
 out_uninit_ctrl:
 	nvme_uninit_ctrl(&ctrl->ctrl);
-out_put_ctrl:
-	nvme_put_ctrl(&ctrl->ctrl);
+out_free_queues:
+	kfree(ctrl->queues);
+out_free_ctrl:
+	kfree(ctrl);
 	if (ret > 0)
 		ret = -EIO;
 	return ERR_PTR(ret);

2.7.4

From: Roy Shterman <[email protected]>

Its no longer acceptable now that we do not invoke ->free_ctrl from inside ->create_ctrl.

---
 drivers/nvme/target/loop.c | 4 ----
 1 file changed, 4 deletions(-)

index cea6bab1e70c..6f9748a0b7cd 100644
@@ -284,9 +284,6 @@ static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
 {
 	struct nvme_loop_ctrl *ctrl = to_loop_ctrl(nctrl);
 
-	if (list_empty(&ctrl->list))
-		goto free_ctrl;
-
 	mutex_lock(&nvme_loop_ctrl_mutex);
 	list_del(&ctrl->list);
 	mutex_unlock(&nvme_loop_ctrl_mutex);

@@ -297,7 +294,6 @@ static void nvme_loop_free_ctrl(struct nvme_ctrl *nctrl)
 	}
 	kfree(ctrl->queues);
 	nvmf_free_options(nctrl->opts);
-free_ctrl:
 	kfree(ctrl);
 }
 

2.7.4