[PATCH 0/5] Small rework of nvme_init/uninit_ctrl

[PATCH 0/5] Small rework of nvme_init/uninit_ctrl

[Sagi Grimberg]

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

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Sagi Grimberg]

From: Roy Shterman <roys@lightbitslabs.com>

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.

Signed-off-by: Roy Shterman <roys at lightbitslabs.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c95155696741..b8abd016202e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Keith Busch]

On Wed, Oct 18, 2017@02:07:40PM +0300, Sagi Grimberg wrote:
> From: Roy Shterman <roys at lightbitslabs.com>
> 
> 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.

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.

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Sagi Grimberg]

>> From: Roy Shterman <roys at lightbitslabs.com>
>>
>> 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.
> 
> It's not magic. It's just preventing name reuse when some management
> code retains an open handle after the device was removed.

management code as in nvme-cli?

> 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.

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.

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Keith Busch]

On Wed, Oct 18, 2017@06:29:40PM +0300, Sagi Grimberg wrote:
> 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.

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Keith Busch]

On Wed, Oct 18, 2017@06:29:40PM +0300, Sagi Grimberg wrote:
> 
> > > From: Roy Shterman <roys at lightbitslabs.com>
> > > 
> > > 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.
> > 
> > It's not magic. It's just preventing name reuse when some management
> > code retains an open handle after the device was removed.
> 
> management code as in nvme-cli?
> 
> > 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.
> 
> 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.

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.

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Sagi Grimberg]

> 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.

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Christoph Hellwig]

On Wed, Oct 18, 2017@02:07:40PM +0300, Sagi Grimberg wrote:
> From: Roy Shterman <roys at lightbitslabs.com>
> 
> 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.

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

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Sagi Grimberg]

>> From: Roy Shterman <roys at lightbitslabs.com>
>>
>> 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.
> 
> 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?

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Sagi Grimberg]

>>> From: Roy Shterman <roys at lightbitslabs.com>
>>>
>>> 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.
>>
>> 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?

[PATCH 1/5] nvme-core: Make nvme_init/uninit_ctrl setup/teardown symmetric

[Christoph Hellwig]

On Sun, Oct 22, 2017@02:11:47PM +0300, Sagi Grimberg wrote:
>>> 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.

[PATCH 2/5] nvme-rdma: rework create_ctrl error flow

[Sagi Grimberg]

From: Roy Shterman <roys@lightbitslabs.com>

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.

Signed-off-by: Roy Shterman <roys at lightbitslabs.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 605586b88186..866abee2374d 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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

[PATCH 2/5] nvme-rdma: rework create_ctrl error flow

[Christoph Hellwig]

On Wed, Oct 18, 2017@02:07:41PM +0300, Sagi Grimberg wrote:
> From: Roy Shterman <roys at lightbitslabs.com>
> 
> 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.

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.

[PATCH 2/5] nvme-rdma: rework create_ctrl error flow

[Sagi Grimberg]

>> 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.
> 
> 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.

[PATCH 3/5] nvme-rdma: remove redundant check if the ctrl is in the ctrl when freeing it

[Sagi Grimberg]

From: Roy Shterman <roys@lightbitslabs.com>

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

Signed-off-by: Roy Shterman <roys at lightbitslabs.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 866abee2374d..03a3f0372450 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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

[PATCH 4/5] nvme-loop: rework create_ctrl error flow

[Sagi Grimberg]

From: Roy Shterman <roys@lightbitslabs.com>

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.

Signed-off-by: Roy Shterman <roys at lightbitslabs.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/loop.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 92628c432926..cea6bab1e70c 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -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

[PATCH 5/5] nvme-loop: remove redundant check if the ctrl is in the ctrl when freeing it

[Sagi Grimberg]

From: Roy Shterman <roys@lightbitslabs.com>

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

Signed-off-by: Roy Shterman <roys at lightbitslabs.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/loop.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index cea6bab1e70c..6f9748a0b7cd 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -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