[PATCH 0/5] NVMe fixes and updates for 4.5

[PATCH 0/5] NVMe fixes and updates for 4.5

[Keith Busch]

This is based on linux-block/for-next.

The first two fix a couple minor issues not caught in initial review or
testing for some larger recent patch sets.

The biggest is 5/5. It simplifies IO queue deletion and removes some
of the most unnecessarily complicated code in this driver. If you're
wondering why the new code is so far away from the code it replaces,
this was the closest I could get it and produce a readable diff. :)

Keith Busch (5):
  NVMe: Fix admin queue ring wrap
  NVMe: Use a retryable error code on reset
  NVMe: Remove queue freezing on resets
  NVMe: Shutdown controller only for power-off
  NVMe: IO queue deletion re-write

 drivers/nvme/host/pci.c | 315 +++++++++++++++++-------------------------------
 1 file changed, 113 insertions(+), 202 deletions(-)

-- 
2.6.2.307.g37023ba

[PATCH 1/5] NVMe: Fix admin queue ring wrap

[Keith Busch]

The tag set queue depth needs to be one less than the h/w queue depth
so we don't wrap the circular buffer. This conforms to the specification
defined "Full Queue" condition.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b82bbea..2e6e665 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1271,7 +1271,7 @@ static int nvme_alloc_admin_tags(struct nvme_dev *dev)
 	if (!dev->ctrl.admin_q) {
 		dev->admin_tagset.ops = &nvme_mq_admin_ops;
 		dev->admin_tagset.nr_hw_queues = 1;
-		dev->admin_tagset.queue_depth = NVME_AQ_BLKMQ_DEPTH;
+		dev->admin_tagset.queue_depth = NVME_AQ_BLKMQ_DEPTH - 1;
 		dev->admin_tagset.timeout = ADMIN_TIMEOUT;
 		dev->admin_tagset.numa_node = dev_to_node(dev->dev);
 		dev->admin_tagset.cmd_size = nvme_cmd_size(dev);
-- 
2.6.2.307.g37023ba

[PATCH 1/5] NVMe: Fix admin queue ring wrap

[Christoph Hellwig]

On Wed, Dec 30, 2015@10:27:47AM -0700, Keith Busch wrote:
> The tag set queue depth needs to be one less than the h/w queue depth
> so we don't wrap the circular buffer. This conforms to the specification
> defined "Full Queue" condition.

You'll need to change NVME_AQ_DEPTH to make sure the magic for the
AEN commands still work.

But can you explain the actual issue a little more while we're at it?
Preferably in a comment in the code!

[PATCH 1/5] NVMe: Fix admin queue ring wrap

[Keith Busch]

On Wed, Dec 30, 2015@09:49:54AM -0800, Christoph Hellwig wrote:
> On Wed, Dec 30, 2015@10:27:47AM -0700, Keith Busch wrote:
> > The tag set queue depth needs to be one less than the h/w queue depth
> > so we don't wrap the circular buffer. This conforms to the specification
> > defined "Full Queue" condition.
> 
> You'll need to change NVME_AQ_DEPTH to make sure the magic for the
> AEN commands still work.

It'll still work. The command id is just a cookie to the controller,
and the AEN command id is still higher than the highest request "tag",
so the logic all works.
 
> But can you explain the actual issue a little more while we're at it?
> Preferably in a comment in the code!

Our admin queue depth is fixed to 256. We reserve one for AEN, so the
depth we told blk-mq is 255. If we use all 255 tags, plus the special one
for AEN, we wrapped the queue. In practice, it is unlikely you're going
to use up 255 admin tags, so I don't think it's a condition frequently
tested, but we need to tell blk-mq to use only 254 tags instead.

[PATCH 1/5] NVMe: Fix admin queue ring wrap

[Jens Axboe]

On 12/30/2015 10:27 AM, Keith Busch wrote:
> The tag set queue depth needs to be one less than the h/w queue depth
> so we don't wrap the circular buffer. This conforms to the specification
> defined "Full Queue" condition.
>
> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>   drivers/nvme/host/pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index b82bbea..2e6e665 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -1271,7 +1271,7 @@ static int nvme_alloc_admin_tags(struct nvme_dev *dev)
>   	if (!dev->ctrl.admin_q) {
>   		dev->admin_tagset.ops = &nvme_mq_admin_ops;
>   		dev->admin_tagset.nr_hw_queues = 1;
> -		dev->admin_tagset.queue_depth = NVME_AQ_BLKMQ_DEPTH;
> +		dev->admin_tagset.queue_depth = NVME_AQ_BLKMQ_DEPTH - 1;
>   		dev->admin_tagset.timeout = ADMIN_TIMEOUT;
>   		dev->admin_tagset.numa_node = dev_to_node(dev->dev);
>   		dev->admin_tagset.cmd_size = nvme_cmd_size(dev);

I missed this during review, which is a little sad since I ran into the 
same thing in the initial conversion of the nvme driver to blk-mq.

-- 
Jens Axboe

[PATCH 1/5] NVMe: Fix admin queue ring wrap

[Christoph Hellwig]

On Wed, Dec 30, 2015@10:53:39AM -0700, Jens Axboe wrote:
> I missed this during review, which is a little sad since I ran into the same
> thing in the initial conversion of the nvme driver to blk-mq.

Another reason to finally add a comment and explain the magic one off..

[PATCH 2/5] NVMe: Use a retryable error code on reset

[Keith Busch]

Use a fake status that can potentially be retried on reset.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 2e6e665..3abcb3a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1017,7 +1017,7 @@ static void nvme_cancel_queue_ios(struct request *req, void *data, bool reserved
 	dev_warn(nvmeq->q_dmadev,
 		 "Cancelling I/O %d QID %d\n", req->tag, nvmeq->qid);
 
-	status = NVME_SC_CANCELLED;
+	status = NVME_SC_ABORT_REQ;
 	if (blk_queue_dying(req->q))
 		status |= NVME_SC_DNR;
 	blk_mq_complete_request(req, status);
-- 
2.6.2.307.g37023ba

[PATCH 2/5] NVMe: Use a retryable error code on reset

[Christoph Hellwig]

On Wed, Dec 30, 2015@10:27:48AM -0700, Keith Busch wrote:
> Use a fake status that can potentially be retried on reset.

I thought I only added this to make you happy, so I'm fine with
changing it.

But I don't understand why NVME_SC_CANCELLED isn't retryable,
nvme_req_needs_retry doesn't treat negative error codes special.
If it did we'd need to fix it as we want to be able to rety
the other cases where it is returned as well.

Which btw brings me to a bug I noticed a while ago but didn't
have time to fully understand: nvme_req_needs_retry looks
at req->start_time vs req->timeout, but that means we'll
fail any command that times out, even if we resubmit it
after a controller reset.  Should we really have that clause
in there?

[PATCH 2/5] NVMe: Use a retryable error code on reset

[Keith Busch]

On Wed, Dec 30, 2015@09:52:32AM -0800, Christoph Hellwig wrote:
> On Wed, Dec 30, 2015@10:27:48AM -0700, Keith Busch wrote:
> > Use a fake status that can potentially be retried on reset.
> 
> I thought I only added this to make you happy, so I'm fine with
> changing it.
> 
> But I don't understand why NVME_SC_CANCELLED isn't retryable,
> nvme_req_needs_retry doesn't treat negative error codes special.
> If it did we'd need to fix it as we want to be able to rety
> the other cases where it is returned as well.

nvme_req_needs_retry checks NVME status bit "DNR", which happens to be
set with a negative return code.
 
> Which btw brings me to a bug I noticed a while ago but didn't
> have time to fully understand: nvme_req_needs_retry looks
> at req->start_time vs req->timeout, but that means we'll
> fail any command that times out, even if we resubmit it
> after a controller reset.  Should we really have that clause
> in there?

Yeah, a timed out command isn't retryable here. The check is to allow
requeuing when a controller reset occured for reasons other than
a timeout.

A long time ago, we had total command lifetime timeout value just to
allow timed out commands to requeue after a reset, but that was removed
with the blk-mq conversion. I didn't think it was a big deal, though. The
amount of time it takes to trigger a reset is 60 seconds by default, so
that's a heck of a long time to wait just to requeue the IO. Some users
I spoke with prefered to just fail the command sooner, so I didn't make
any attempt to change this behavior.

[PATCH 2/5] NVMe: Use a retryable error code on reset

[Christoph Hellwig]

On Wed, Dec 30, 2015@06:09:08PM +0000, Keith Busch wrote:
> > I thought I only added this to make you happy, so I'm fine with
> > changing it.
> > 
> > But I don't understand why NVME_SC_CANCELLED isn't retryable,
> > nvme_req_needs_retry doesn't treat negative error codes special.
> > If it did we'd need to fix it as we want to be able to rety
> > the other cases where it is returned as well.
> 
> nvme_req_needs_retry checks NVME status bit "DNR", which happens to be
> set with a negative return code.

Uh-oh.  I had specificly written a little test case to check if
DNR works with a signed value, and it did.  But I forgot to do the
negative check.

This means I need to revisit this scheme entirely as NVME_SC_CANCELLED
should not imply a DNR in general.

> Yeah, a timed out command isn't retryable here. The check is to allow
> requeuing when a controller reset occured for reasons other than
> a timeout.

Generally we want to be able to retry after a timeout, so both
this and the above issue need to be fixed.

> A long time ago, we had total command lifetime timeout value just to
> allow timed out commands to requeue after a reset, but that was removed
> with the blk-mq conversion. I didn't think it was a big deal, though. The
> amount of time it takes to trigger a reset is 60 seconds by default, so
> that's a heck of a long time to wait just to requeue the IO. Some users
> I spoke with prefered to just fail the command sooner, so I didn't make
> any attempt to change this behavior.

For SCSI we have a separate error handlind deadline fo that. 

[PATCH 3/5] NVMe: Remove queue freezing on resets

[Keith Busch]

NVMe submits all commands through the block layer now. This means we
can let requests queue at the blk-mq hardware context since there is no
path that bypasses this anymore. This means we don't need to freeze the
queues anymore and simply stop the h/w queues from running during a reset.

This also fixes a WARN in percpu_ref_reinit when the queue was unfrozen
with requeued requests.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 3abcb3a..6fb65d4 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1064,7 +1064,7 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	spin_unlock_irq(&nvmeq->q_lock);
 
 	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
-		blk_mq_freeze_queue_start(nvmeq->dev->ctrl.admin_q);
+		blk_mq_stop_hw_queues(nvmeq->dev->ctrl.admin_q);
 
 	irq_set_affinity_hint(vector, NULL);
 	free_irq(vector, nvmeq);
@@ -1291,7 +1291,7 @@ static int nvme_alloc_admin_tags(struct nvme_dev *dev)
 			return -ENODEV;
 		}
 	} else
-		blk_mq_unfreeze_queue(dev->ctrl.admin_q);
+		blk_mq_start_stopped_hw_queues(dev->ctrl.admin_q, true);
 
 	return 0;
 }
@@ -1903,13 +1903,11 @@ static void nvme_dev_list_remove(struct nvme_dev *dev)
 		kthread_stop(tmp);
 }
 
-static void nvme_freeze_queues(struct nvme_dev *dev)
+static void nvme_stop_queues(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns;
 
 	list_for_each_entry(ns, &dev->ctrl.namespaces, list) {
-		blk_mq_freeze_queue_start(ns->queue);
-
 		spin_lock_irq(ns->queue->queue_lock);
 		queue_flag_set(QUEUE_FLAG_STOPPED, ns->queue);
 		spin_unlock_irq(ns->queue->queue_lock);
@@ -1919,13 +1917,12 @@ static void nvme_freeze_queues(struct nvme_dev *dev)
 	}
 }
 
-static void nvme_unfreeze_queues(struct nvme_dev *dev)
+static void nvme_start_queues(struct nvme_dev *dev)
 {
 	struct nvme_ns *ns;
 
 	list_for_each_entry(ns, &dev->ctrl.namespaces, list) {
 		queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, ns->queue);
-		blk_mq_unfreeze_queue(ns->queue);
 		blk_mq_start_stopped_hw_queues(ns->queue, true);
 		blk_mq_kick_requeue_list(ns->queue);
 	}
@@ -1940,7 +1937,7 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
 
 	mutex_lock(&dev->shutdown_lock);
 	if (dev->bar) {
-		nvme_freeze_queues(dev);
+		nvme_stop_queues(dev);
 		csts = readl(dev->bar + NVME_REG_CSTS);
 	}
 	if (csts & NVME_CSTS_CFS || !(csts & NVME_CSTS_RDY)) {
@@ -2049,7 +2046,7 @@ static void nvme_reset_work(struct work_struct *work)
 		dev_warn(dev->dev, "IO queues not created\n");
 		nvme_remove_namespaces(&dev->ctrl);
 	} else {
-		nvme_unfreeze_queues(dev);
+		nvme_start_queues(dev);
 		nvme_dev_add(dev);
 	}
 
-- 
2.6.2.307.g37023ba

[PATCH 3/5] NVMe: Remove queue freezing on resets

[Christoph Hellwig]

Looks fine to me, but this conflicts with Sagis patch to move
nvme_{un,}freeze_queues to common code and my patches that sit
on top of that.  Guess I'll need to rebase that series again.

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

[PATCH 3/5] NVMe: Remove queue freezing on resets

[Sagi Grimberg]

> Looks fine to me, but this conflicts with Sagis patch to move
> nvme_{un,}freeze_queues to common code and my patches that sit
> on top of that.  Guess I'll need to rebase that series again.

Is there any reason not to take that patch? I have no problem
resending it if it'd save you guys some time...

[PATCH 3/5] NVMe: Remove queue freezing on resets

[Sagi Grimberg]

> NVMe submits all commands through the block layer now. This means we
> can let requests queue at the blk-mq hardware context since there is no
> path that bypasses this anymore. This means we don't need to freeze the
> queues anymore and simply stop the h/w queues from running during a reset.

Does this have any dependencies? I tried this at some point but it
didn't work as I expected it to.

> This also fixes a WARN in percpu_ref_reinit when the queue was unfrozen
> with requeued requests.

Oh yes, I've seen this one too! I'll test it for sure.

[PATCH 3/5] NVMe: Remove queue freezing on resets

[Sagi Grimberg]

>> NVMe submits all commands through the block layer now. This means we
>> can let requests queue at the blk-mq hardware context since there is no
>> path that bypasses this anymore. This means we don't need to freeze the
>> queues anymore and simply stop the h/w queues from running during a
>> reset.
>
> Does this have any dependencies? I tried this at some point but it
> didn't work as I expected it to.

OK, I figured out what I did wrong.


>> This also fixes a WARN in percpu_ref_reinit when the queue was unfrozen
>> with requeued requests.
>
> Oh yes, I've seen this one too! I'll test it for sure.

This works great for me,

Tested-by: Sagi Grimberg <sagig at mellanox.com>

[PATCH 4/5] NVMe: Shutdown controller only for power-off

[Keith Busch]

We don't need to shutdown a controller for a reset. A controller in a
shutdown state may take longer to become ready than one that was simply
disabled. This patch has the driver shut down a controller only if the
device is about to be powered off or being removed. When the taking
the controller down for a reset reason, the controller will be disabled
instead.

Function names have been updated in this patch to reflect their changed
semantics.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6fb65d4..34cc95b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -87,7 +87,7 @@ struct nvme_queue;
 static int nvme_reset(struct nvme_dev *dev);
 static void nvme_process_cq(struct nvme_queue *nvmeq);
 static void nvme_remove_dead_ctrl(struct nvme_dev *dev);
-static void nvme_dev_shutdown(struct nvme_dev *dev);
+static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
 
 struct async_cmd_info {
 	struct kthread_work work;
@@ -947,7 +947,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 		dev_warn(dev->dev,
 			 "I/O %d QID %d timeout, disable controller\n",
 			 req->tag, nvmeq->qid);
-		nvme_dev_shutdown(dev);
+		nvme_dev_disable(dev, false);
 		req->errors = NVME_SC_CANCELLED;
 		return BLK_EH_HANDLED;
 	}
@@ -961,7 +961,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 		dev_warn(dev->dev,
 			 "I/O %d QID %d timeout, reset controller\n",
 			 req->tag, nvmeq->qid);
-		nvme_dev_shutdown(dev);
+		nvme_dev_disable(dev, false);
 		queue_work(nvme_workq, &dev->reset_work);
 
 		/*
@@ -1080,21 +1080,18 @@ static void nvme_clear_queue(struct nvme_queue *nvmeq)
 	spin_unlock_irq(&nvmeq->q_lock);
 }
 
-static void nvme_disable_queue(struct nvme_dev *dev, int qid)
+static void nvme_disable_admin_queue(struct nvme_dev *dev, bool disable_ctrl)
 {
-	struct nvme_queue *nvmeq = dev->queues[qid];
+	struct nvme_queue *nvmeq = dev->queues[0];
 
 	if (!nvmeq)
 		return;
 	if (nvme_suspend_queue(nvmeq))
 		return;
 
-	/* Don't tell the adapter to delete the admin queue.
-	 * Don't tell a removed adapter to delete IO queues. */
-	if (qid && readl(dev->bar + NVME_REG_CSTS) != -1) {
-		adapter_delete_sq(dev, qid);
-		adapter_delete_cq(dev, qid);
-	}
+	if (disable_ctrl)
+		nvme_disable_ctrl(&dev->ctrl,
+			lo_hi_readq(dev->bar + NVME_REG_CAP));
 
 	spin_lock_irq(&nvmeq->q_lock);
 	nvme_process_cq(nvmeq);
@@ -1928,7 +1925,7 @@ static void nvme_start_queues(struct nvme_dev *dev)
 	}
 }
 
-static void nvme_dev_shutdown(struct nvme_dev *dev)
+static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 {
 	int i;
 	u32 csts = -1;
@@ -1947,8 +1944,9 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
 		}
 	} else {
 		nvme_disable_io_queues(dev);
-		nvme_shutdown_ctrl(&dev->ctrl);
-		nvme_disable_queue(dev, 0);
+		if (shutdown)
+			nvme_shutdown_ctrl(&dev->ctrl);
+		nvme_disable_admin_queue(dev, !shutdown);
 	}
 	nvme_dev_unmap(dev);
 
@@ -2007,7 +2005,7 @@ static void nvme_reset_work(struct work_struct *work)
 	 * moving on.
 	 */
 	if (dev->bar)
-		nvme_dev_shutdown(dev);
+		nvme_dev_disable(dev, false);
 
 	set_bit(NVME_CTRL_RESETTING, &dev->flags);
 
@@ -2061,7 +2059,7 @@ static void nvme_reset_work(struct work_struct *work)
 	dev->ctrl.admin_q = NULL;
 	dev->queues[0]->tags = NULL;
  disable:
-	nvme_disable_queue(dev, 0);
+	nvme_disable_admin_queue(dev, false);
  unmap:
 	nvme_dev_unmap(dev);
  out:
@@ -2195,7 +2193,7 @@ static void nvme_reset_notify(struct pci_dev *pdev, bool prepare)
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
 
 	if (prepare)
-		nvme_dev_shutdown(dev);
+		nvme_dev_disable(dev, false);
 	else
 		queue_work(nvme_workq, &dev->reset_work);
 }
@@ -2203,7 +2201,7 @@ static void nvme_reset_notify(struct pci_dev *pdev, bool prepare)
 static void nvme_shutdown(struct pci_dev *pdev)
 {
 	struct nvme_dev *dev = pci_get_drvdata(pdev);
-	nvme_dev_shutdown(dev);
+	nvme_dev_disable(dev, true);
 }
 
 static void nvme_remove(struct pci_dev *pdev)
@@ -2219,7 +2217,7 @@ static void nvme_remove(struct pci_dev *pdev)
 	flush_work(&dev->scan_work);
 	nvme_remove_namespaces(&dev->ctrl);
 	nvme_uninit_ctrl(&dev->ctrl);
-	nvme_dev_shutdown(dev);
+	nvme_dev_disable(dev, true);
 	nvme_dev_remove_admin(dev);
 	nvme_free_queues(dev, 0);
 	nvme_release_cmb(dev);
@@ -2233,7 +2231,7 @@ static int nvme_suspend(struct device *dev)
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct nvme_dev *ndev = pci_get_drvdata(pdev);
 
-	nvme_dev_shutdown(ndev);
+	nvme_dev_disable(ndev, true);
 	return 0;
 }
 
@@ -2264,7 +2262,7 @@ static pci_ers_result_t nvme_error_detected(struct pci_dev *pdev,
 	case pci_channel_io_normal:
 		return PCI_ERS_RESULT_CAN_RECOVER;
 	case pci_channel_io_frozen:
-		nvme_dev_shutdown(dev);
+		nvme_dev_disable(dev, false);
 		return PCI_ERS_RESULT_NEED_RESET;
 	case pci_channel_io_perm_failure:
 		return PCI_ERS_RESULT_DISCONNECT;
-- 
2.6.2.307.g37023ba

[PATCH 4/5] NVMe: Shutdown controller only for power-off

[Christoph Hellwig]

On Wed, Dec 30, 2015@10:27:50AM -0700, Keith Busch wrote:
> We don't need to shutdown a controller for a reset. A controller in a
> shutdown state may take longer to become ready than one that was simply
> disabled. This patch has the driver shut down a controller only if the
> device is about to be powered off or being removed. When the taking
> the controller down for a reset reason, the controller will be disabled
> instead.
> 
> Function names have been updated in this patch to reflect their changed
> semantics.

Am I missing something?  What happens to the calls to nvme_disable_queue
in nvme_disable_io_queues and nvme_wait_dq?

> -static void nvme_disable_queue(struct nvme_dev *dev, int qid)
> +static void nvme_disable_admin_queue(struct nvme_dev *dev, bool disable_ctrl)
>  {
> -	struct nvme_queue *nvmeq = dev->queues[qid];
> +	struct nvme_queue *nvmeq = dev->queues[0];
>  
>  	if (!nvmeq)
>  		return;
>  	if (nvme_suspend_queue(nvmeq))
>  		return;
>  
> -	/* Don't tell the adapter to delete the admin queue.
> -	 * Don't tell a removed adapter to delete IO queues. */
> -	if (qid && readl(dev->bar + NVME_REG_CSTS) != -1) {
> -		adapter_delete_sq(dev, qid);
> -		adapter_delete_cq(dev, qid);
> -	}
> +	if (disable_ctrl)
> +		nvme_disable_ctrl(&dev->ctrl,
> +			lo_hi_readq(dev->bar + NVME_REG_CAP));

Why can't this be done outside this function, similar to the
shutdown case?

[PATCH 4/5] NVMe: Shutdown controller only for power-off

[Keith Busch]

On Wed, Dec 30, 2015@09:58:29AM -0800, Christoph Hellwig wrote:
> On Wed, Dec 30, 2015@10:27:50AM -0700, Keith Busch wrote:
> > We don't need to shutdown a controller for a reset. A controller in a
> > shutdown state may take longer to become ready than one that was simply
> > disabled. This patch has the driver shut down a controller only if the
> > device is about to be powered off or being removed. When the taking
> > the controller down for a reset reason, the controller will be disabled
> > instead.
> > 
> > Function names have been updated in this patch to reflect their changed
> > semantics.
> 
> Am I missing something?  What happens to the calls to nvme_disable_queue
> in nvme_disable_io_queues and nvme_wait_dq?

My bad. I applied the patches in the wrong order when generating the
series. Need to swap 4/5 with 5/5.
 
> > +	if (disable_ctrl)
> > +		nvme_disable_ctrl(&dev->ctrl,
> > +			lo_hi_readq(dev->bar + NVME_REG_CAP));
> 
> Why can't this be done outside this function, similar to the
> shutdown case?

I thought it made sense to do it inline with disabling the admin queue
since there's no command to delete it. A controller disable is the only
way, and there's no need to disable it if it was shutdown prior.

[PATCH 4/5] NVMe: Shutdown controller only for power-off

[Christoph Hellwig]

On Wed, Dec 30, 2015@06:02:54PM +0000, Keith Busch wrote:
> My bad. I applied the patches in the wrong order when generating the
> series. Need to swap 4/5 with 5/5.
>  
> > > +	if (disable_ctrl)
> > > +		nvme_disable_ctrl(&dev->ctrl,
> > > +			lo_hi_readq(dev->bar + NVME_REG_CAP));
> > 
> > Why can't this be done outside this function, similar to the
> > shutdown case?
> 
> I thought it made sense to do it inline with disabling the admin queue
> since there's no command to delete it. A controller disable is the only
> way, and there's no need to disable it if it was shutdown prior.

Well, the only place ever setting disable_ctrl is nvme_dev_disable,
which will call nvme_shutdown_ctrl if it doesn't set disable.

Having the whole controller state manipulation in nvme_dev_disable
with a neat if / else and outside a function that claims to only
operate on the admin queue would seem sensible to me.  The only
thing I wondered about is if disable vs shutdown had different
interactions with the nvme_suspend_queue call, but I couldn't think
of any.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Keith Busch]

The nvme driver deletes IO queues asynchronously since this operation
may potentially take an undesirable amount of time with a large number
of queues if done serially.

The driver used to manage coordinating asynchronous deletions. This
patch simplifies that by leveraging the block layer rather than using
kthread workers and complicated callback chaining.

Beyond just being a simpler method, this also fixes a theoretical hang
if the controller stops responding while the worker thread was queued
deeper than the admin queue depth.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/pci.c | 256 ++++++++++++++++--------------------------------
 1 file changed, 86 insertions(+), 170 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 34cc95b..eb0edac 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -89,13 +89,6 @@ static void nvme_process_cq(struct nvme_queue *nvmeq);
 static void nvme_remove_dead_ctrl(struct nvme_dev *dev);
 static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown);
 
-struct async_cmd_info {
-	struct kthread_work work;
-	struct kthread_worker *worker;
-	int status;
-	void *ctx;
-};
-
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI function.
  */
@@ -125,6 +118,7 @@ struct nvme_dev {
 	u64 cmb_size;
 	u32 cmbsz;
 	unsigned long flags;
+
 #define NVME_CTRL_RESETTING    0
 
 	struct nvme_ctrl ctrl;
@@ -159,7 +153,10 @@ struct nvme_queue {
 	u16 qid;
 	u8 cq_phase;
 	u8 cqe_seen;
-	struct async_cmd_info cmdinfo;
+	union {
+		struct nvme_delete_queue dq;	/* For IO queues */
+		struct completion wait;		/* For Admin Queue */
+	};
 };
 
 /*
@@ -844,15 +841,6 @@ static void nvme_submit_async_event(struct nvme_dev *dev)
 	__nvme_submit_cmd(dev->queues[0], &c);
 }
 
-static void async_cmd_info_endio(struct request *req, int error)
-{
-	struct async_cmd_info *cmdinfo = req->end_io_data;
-
-	cmdinfo->status = req->errors;
-	queue_kthread_work(cmdinfo->worker, &cmdinfo->work);
-	blk_mq_free_request(req);
-}
-
 static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)
 {
 	struct nvme_command c;
@@ -1316,6 +1304,7 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)
 		nvmeq = nvme_alloc_queue(dev, 0, NVME_AQ_DEPTH);
 		if (!nvmeq)
 			return -ENOMEM;
+		init_completion(&nvmeq->wait);
 	}
 
 	aqa = nvmeq->q_depth - 1;
@@ -1592,6 +1581,86 @@ static void nvme_dev_scan(struct work_struct *work)
 	nvme_set_irq_hints(dev);
 }
 
+static void nvme_del_queue_end(struct request *req, int error)
+{
+	unsigned long flags;
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	struct nvme_queue *nvmeq = req->end_io_data;
+	struct nvme_queue *admin_q = iod->nvmeq;
+
+	blk_mq_free_request(req);
+
+	spin_lock_irqsave(&nvmeq->q_lock, flags);
+	nvme_process_cq(nvmeq);
+	spin_unlock_irqrestore(&nvmeq->q_lock, flags);
+
+	complete(&admin_q->wait);
+}
+
+static void nvme_del_sq_end(struct request *req, int error)
+{
+	struct nvme_queue *nvmeq = req->end_io_data;
+
+	if (error) {
+		nvme_del_queue_end(req, error);
+		return;
+	}
+
+	nvmeq->dq.opcode = nvme_admin_delete_cq;
+	req->end_io = nvme_del_queue_end;
+
+	/* Must requeue. This callback occurs in irq w/ q_lock held */
+	nvme_requeue_req(req);
+}
+
+static int nvme_delete_queue(struct nvme_queue *nvmeq, struct request_queue *q)
+{
+	struct request *req;
+
+	req = nvme_alloc_request(q, (struct nvme_command *)&nvmeq->dq,
+							BLK_MQ_REQ_NOWAIT);
+	if (IS_ERR(req))
+		return -1;
+
+	memset(&nvmeq->dq, 0, sizeof(nvmeq->dq));
+	nvmeq->dq.opcode = nvme_admin_delete_sq;
+	nvmeq->dq.qid = cpu_to_le16(nvmeq->qid);
+
+	req->end_io_data = nvmeq;
+	req->timeout = ADMIN_TIMEOUT;
+
+	blk_execute_rq_nowait(q, NULL, req, false, nvme_del_sq_end);
+	return 0;
+}
+
+static void nvme_disable_io_queues(struct nvme_dev *dev)
+{
+	struct request_queue *q = dev->ctrl.admin_q;
+	struct nvme_queue *admin_q = dev->queues[0];
+	int i = dev->queue_count - 1, sent = 0;
+	unsigned long timeout;
+
+	reinit_completion(&admin_q->wait);
+ retry:
+	timeout = ADMIN_TIMEOUT;
+	for (; i > 0; i--) {
+		struct nvme_queue *nvmeq = dev->queues[i];
+
+		nvme_suspend_queue(nvmeq);
+		if (nvme_delete_queue(nvmeq, q))
+			break;
+		++sent;
+	}
+	while (sent--) {
+		timeout = wait_for_completion_io_timeout(&admin_q->wait,
+							timeout);
+		if (timeout == 0)
+			return;
+		if (i)
+			goto retry;
+	}
+}
+
 /*
  * Return: error value if an error occurred setting up the queues or calling
  * Identify Device.  0 if these succeeded, even if adding some of the
@@ -1703,159 +1772,6 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	}
 }
 
-struct nvme_delq_ctx {
-	struct task_struct *waiter;
-	struct kthread_worker *worker;
-	atomic_t refcount;
-};
-
-static void nvme_wait_dq(struct nvme_delq_ctx *dq, struct nvme_dev *dev)
-{
-	dq->waiter = current;
-	mb();
-
-	for (;;) {
-		set_current_state(TASK_KILLABLE);
-		if (!atomic_read(&dq->refcount))
-			break;
-		if (!schedule_timeout(ADMIN_TIMEOUT) ||
-					fatal_signal_pending(current)) {
-			/*
-			 * Disable the controller first since we can't trust it
-			 * at this point, but leave the admin queue enabled
-			 * until all queue deletion requests are flushed.
-			 * FIXME: This may take a while if there are more h/w
-			 * queues than admin tags.
-			 */
-			set_current_state(TASK_RUNNING);
-			nvme_disable_ctrl(&dev->ctrl,
-				lo_hi_readq(dev->bar + NVME_REG_CAP));
-			nvme_clear_queue(dev->queues[0]);
-			flush_kthread_worker(dq->worker);
-			nvme_disable_queue(dev, 0);
-			return;
-		}
-	}
-	set_current_state(TASK_RUNNING);
-}
-
-static void nvme_put_dq(struct nvme_delq_ctx *dq)
-{
-	atomic_dec(&dq->refcount);
-	if (dq->waiter)
-		wake_up_process(dq->waiter);
-}
-
-static struct nvme_delq_ctx *nvme_get_dq(struct nvme_delq_ctx *dq)
-{
-	atomic_inc(&dq->refcount);
-	return dq;
-}
-
-static void nvme_del_queue_end(struct nvme_queue *nvmeq)
-{
-	struct nvme_delq_ctx *dq = nvmeq->cmdinfo.ctx;
-	nvme_put_dq(dq);
-
-	spin_lock_irq(&nvmeq->q_lock);
-	nvme_process_cq(nvmeq);
-	spin_unlock_irq(&nvmeq->q_lock);
-}
-
-static int adapter_async_del_queue(struct nvme_queue *nvmeq, u8 opcode,
-						kthread_work_func_t fn)
-{
-	struct request *req;
-	struct nvme_command c;
-
-	memset(&c, 0, sizeof(c));
-	c.delete_queue.opcode = opcode;
-	c.delete_queue.qid = cpu_to_le16(nvmeq->qid);
-
-	init_kthread_work(&nvmeq->cmdinfo.work, fn);
-
-	req = nvme_alloc_request(nvmeq->dev->ctrl.admin_q, &c, 0);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
-
-	req->timeout = ADMIN_TIMEOUT;
-	req->end_io_data = &nvmeq->cmdinfo;
-	blk_execute_rq_nowait(req->q, NULL, req, 0, async_cmd_info_endio);
-	return 0;
-}
-
-static void nvme_del_cq_work_handler(struct kthread_work *work)
-{
-	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue,
-							cmdinfo.work);
-	nvme_del_queue_end(nvmeq);
-}
-
-static int nvme_delete_cq(struct nvme_queue *nvmeq)
-{
-	return adapter_async_del_queue(nvmeq, nvme_admin_delete_cq,
-						nvme_del_cq_work_handler);
-}
-
-static void nvme_del_sq_work_handler(struct kthread_work *work)
-{
-	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue,
-							cmdinfo.work);
-	int status = nvmeq->cmdinfo.status;
-
-	if (!status)
-		status = nvme_delete_cq(nvmeq);
-	if (status)
-		nvme_del_queue_end(nvmeq);
-}
-
-static int nvme_delete_sq(struct nvme_queue *nvmeq)
-{
-	return adapter_async_del_queue(nvmeq, nvme_admin_delete_sq,
-						nvme_del_sq_work_handler);
-}
-
-static void nvme_del_queue_start(struct kthread_work *work)
-{
-	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue,
-							cmdinfo.work);
-	if (nvme_delete_sq(nvmeq))
-		nvme_del_queue_end(nvmeq);
-}
-
-static void nvme_disable_io_queues(struct nvme_dev *dev)
-{
-	int i;
-	DEFINE_KTHREAD_WORKER_ONSTACK(worker);
-	struct nvme_delq_ctx dq;
-	struct task_struct *kworker_task = kthread_run(kthread_worker_fn,
-					&worker, "nvme%d", dev->ctrl.instance);
-
-	if (IS_ERR(kworker_task)) {
-		dev_err(dev->dev,
-			"Failed to create queue del task\n");
-		for (i = dev->queue_count - 1; i > 0; i--)
-			nvme_disable_queue(dev, i);
-		return;
-	}
-
-	dq.waiter = NULL;
-	atomic_set(&dq.refcount, 0);
-	dq.worker = &worker;
-	for (i = dev->queue_count - 1; i > 0; i--) {
-		struct nvme_queue *nvmeq = dev->queues[i];
-
-		if (nvme_suspend_queue(nvmeq))
-			continue;
-		nvmeq->cmdinfo.ctx = nvme_get_dq(&dq);
-		nvmeq->cmdinfo.worker = dq.worker;
-		init_kthread_work(&nvmeq->cmdinfo.work, nvme_del_queue_start);
-		queue_kthread_work(dq.worker, &nvmeq->cmdinfo.work);
-	}
-	nvme_wait_dq(&dq, dev);
-	kthread_stop(kworker_task);
-}
-
 static int nvme_dev_list_add(struct nvme_dev *dev)
 {
 	bool start_thread = false;
-- 
2.6.2.307.g37023ba

[PATCH 5/5] NVMe: IO queue deletion re-write

[Christoph Hellwig]

On Wed, Dec 30, 2015@10:27:51AM -0700, Keith Busch wrote:
> The nvme driver deletes IO queues asynchronously since this operation
> may potentially take an undesirable amount of time with a large number
> of queues if done serially.
> 
> The driver used to manage coordinating asynchronous deletions. This
> patch simplifies that by leveraging the block layer rather than using
> kthread workers and complicated callback chaining.
> 
> Beyond just being a simpler method, this also fixes a theoretical hang
> if the controller stops responding while the worker thread was queued
> deeper than the admin queue depth.

I love the overall scheme, but I think some of the implementation
detail will need a few changes to be long term maintainable:

First the 'wait' workqueue is a per-controller wait, so it should
be in nvme_ctrl instead of a union inside the queue.  Second it
should really be a wait queue, and it should only wake the caller
when all pending queues have been deleted to avoid spurious wakeups.

The other things is the use of nvme_requeue_req for submitting
a different command, which looks rather hacky.  I'd suggest to
just wake a per-queue work item to do a proper command allocation
and submission, which also means we can use the normal on-stack
commands.

Btw:

> +static int nvme_delete_queue(struct nvme_queue *nvmeq, struct request_queue *q)
> +{
> +	struct request *req;
> +
> +	req = nvme_alloc_request(q, (struct nvme_command *)&nvmeq->dq,
> +							BLK_MQ_REQ_NOWAIT);
> +	if (IS_ERR(req))
> +		return -1;
> +
> +	memset(&nvmeq->dq, 0, sizeof(nvmeq->dq));
> +	nvmeq->dq.opcode = nvme_admin_delete_sq;
> +	nvmeq->dq.qid = cpu_to_le16(nvmeq->qid);
> +
> +	req->end_io_data = nvmeq;
> +	req->timeout = ADMIN_TIMEOUT;
> +
> +	blk_execute_rq_nowait(q, NULL, req, false, nvme_del_sq_end);
> +	return 0;

Please return either a Linux error code or a bool, but not a 0 vs -1
integer.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Keith Busch]

On Wed, Dec 30, 2015@10:04:30AM -0800, Christoph Hellwig wrote:
> I love the overall scheme, but I think some of the implementation
> detail will need a few changes to be long term maintainable:

Thanks! I don't think anyone liked the existing way, but it hasn't been
completely trivial to replace it and preserve functionality.

> First the 'wait' workqueue is a per-controller wait, so it should
> be in nvme_ctrl instead of a union inside the queue. 

Okay. I was trying to save a few bytes. Certainly not worth it.

> Second it
> should really be a wait queue, and it should only wake the caller
> when all pending queues have been deleted to avoid spurious wakeups.

The "completion" API counts the completions so the driver doesn't have to,
and waking the thread on each completion notifies the waiter a request
tag is available so it may continue submitting queue deletions.

> The other things is the use of nvme_requeue_req for submitting
> a different command, which looks rather hacky.  I'd suggest to
> just wake a per-queue work item to do a proper command allocation
> and submission, which also means we can use the normal on-stack
> commands.

Aww, I really liked the re-queuing! :) It encapsulates deleting a
queue-pair as a single, two-step operation.

There's a failure scenario that complicates work item handling. It has
the same flaw kthread workers had if work is queued deeper than available
tags, and then the controller stops responding. It's a mess to clean
that up and synchronize when everything times out. Issuing everything
from the main thread and irq callback is painless in comparison.

But I didn't like having to store the "struct command" in the nvme_queue
either, so I'm happy to consider alternatives if I've missed seeing an
elegant solution.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Christoph Hellwig]

> Aww, I really liked the re-queuing! :) It encapsulates deleting a
> queue-pair as a single, two-step operation.

I think it can still be nicely encapsulated when using the
workqueue.  And I think that's essential as requeuing for
a different command breaks all kinds of assumptions.

> There's a failure scenario that complicates work item handling. It has
> the same flaw kthread workers had if work is queued deeper than available
> tags, and then the controller stops responding. It's a mess to clean
> that up and synchronize when everything times out. Issuing everything
> from the main thread and irq callback is painless in comparison.

True, that's not getting simpler.

> But I didn't like having to store the "struct command" in the nvme_queue
> either, so I'm happy to consider alternatives if I've missed seeing an
> elegant solution.

Just curious:  why do we even both with the async queue shutdown.  At
least the controllers I have access to complete queue deletions very
quickly.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Keith Busch]

On Sat, Jan 02, 2016@09:07:30AM -0800, Christoph Hellwig wrote:
> Just curious:  why do we even both with the async queue shutdown.  At
> least the controllers I have access to complete queue deletions very
> quickly.

The async deletion was written for a bug reporting "hang" on a device
removal. The "hang" was the controller taking on the order of 100's msec
to delete a queue (sometimes >1sec if lots of commands queued). This
controller had 2k queues, and took ~15 minutes to remove serially. Async
deletion brought it down to ~20 seconds, so looked like a good idea.

It wasn't a controller I make, so I personally don't care about
parallelizing queue deletion. The driver's been this way for so long
though, I don't have a good way to know how beneficial this feature
is anymore.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Christoph Hellwig]

On Sat, Jan 02, 2016@09:30:09PM +0000, Keith Busch wrote:
> The async deletion was written for a bug reporting "hang" on a device
> removal. The "hang" was the controller taking on the order of 100's msec
> to delete a queue (sometimes >1sec if lots of commands queued). This
> controller had 2k queues, and took ~15 minutes to remove serially. Async
> deletion brought it down to ~20 seconds, so looked like a good idea.
> 
> It wasn't a controller I make, so I personally don't care about
> parallelizing queue deletion. The driver's been this way for so long
> though, I don't have a good way to know how beneficial this feature
> is anymore.

How about something like the lightly tested patch below.  It uses
synchronous command submission, but schedules a work item on the
system unbound workqueue for each queue, allowing the scheduler
to execture them in parallel.

---
From: Christoph Hellwig <hch@lst.de>
Date: Sun, 3 Jan 2016 12:09:36 +0100
Subject: nvme: semi-synchronous queue deletion

Replace the complex async queue deletetion scheme with a a work_item
per queue that is scheduled to the system unbound workqueue.  That
way we can use the normal synchronous command submission helpers,
but let the scheduler distribute the deletions over all available
CPUs.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 180 +++++++-----------------------------------------
 1 file changed, 25 insertions(+), 155 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index b82bbea..68ba2d4 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -89,13 +89,6 @@ static void nvme_process_cq(struct nvme_queue *nvmeq);
 static void nvme_remove_dead_ctrl(struct nvme_dev *dev);
 static void nvme_dev_shutdown(struct nvme_dev *dev);
 
-struct async_cmd_info {
-	struct kthread_work work;
-	struct kthread_worker *worker;
-	int status;
-	void *ctx;
-};
-
 /*
  * Represents an NVM Express device.  Each nvme_dev is a PCI function.
  */
@@ -128,6 +121,10 @@ struct nvme_dev {
 #define NVME_CTRL_RESETTING    0
 
 	struct nvme_ctrl ctrl;
+
+	/* for queue deletion at shutdown time */
+	atomic_t		queues_remaining;
+	wait_queue_head_t	queue_delete_wait;
 };
 
 static inline struct nvme_dev *to_nvme_dev(struct nvme_ctrl *ctrl)
@@ -159,7 +156,7 @@ struct nvme_queue {
 	u16 qid;
 	u8 cq_phase;
 	u8 cqe_seen;
-	struct async_cmd_info cmdinfo;
+	struct work_struct work; /* for queue deletion */
 };
 
 /*
@@ -844,15 +841,6 @@ static void nvme_submit_async_event(struct nvme_dev *dev)
 	__nvme_submit_cmd(dev->queues[0], &c);
 }
 
-static void async_cmd_info_endio(struct request *req, int error)
-{
-	struct async_cmd_info *cmdinfo = req->end_io_data;
-
-	cmdinfo->status = req->errors;
-	queue_kthread_work(cmdinfo->worker, &cmdinfo->work);
-	blk_mq_free_request(req);
-}
-
 static int adapter_delete_queue(struct nvme_dev *dev, u8 opcode, u16 id)
 {
 	struct nvme_command c;
@@ -1706,157 +1694,39 @@ static void nvme_dev_unmap(struct nvme_dev *dev)
 	}
 }
 
-struct nvme_delq_ctx {
-	struct task_struct *waiter;
-	struct kthread_worker *worker;
-	atomic_t refcount;
-};
-
-static void nvme_wait_dq(struct nvme_delq_ctx *dq, struct nvme_dev *dev)
-{
-	dq->waiter = current;
-	mb();
-
-	for (;;) {
-		set_current_state(TASK_KILLABLE);
-		if (!atomic_read(&dq->refcount))
-			break;
-		if (!schedule_timeout(ADMIN_TIMEOUT) ||
-					fatal_signal_pending(current)) {
-			/*
-			 * Disable the controller first since we can't trust it
-			 * at this point, but leave the admin queue enabled
-			 * until all queue deletion requests are flushed.
-			 * FIXME: This may take a while if there are more h/w
-			 * queues than admin tags.
-			 */
-			set_current_state(TASK_RUNNING);
-			nvme_disable_ctrl(&dev->ctrl,
-				lo_hi_readq(dev->bar + NVME_REG_CAP));
-			nvme_clear_queue(dev->queues[0]);
-			flush_kthread_worker(dq->worker);
-			nvme_disable_queue(dev, 0);
-			return;
-		}
-	}
-	set_current_state(TASK_RUNNING);
-}
-
-static void nvme_put_dq(struct nvme_delq_ctx *dq)
-{
-	atomic_dec(&dq->refcount);
-	if (dq->waiter)
-		wake_up_process(dq->waiter);
-}
-
-static struct nvme_delq_ctx *nvme_get_dq(struct nvme_delq_ctx *dq)
+static void nvme_disable_queue_work(struct work_struct *work)
 {
-	atomic_inc(&dq->refcount);
-	return dq;
-}
-
-static void nvme_del_queue_end(struct nvme_queue *nvmeq)
-{
-	struct nvme_delq_ctx *dq = nvmeq->cmdinfo.ctx;
-	nvme_put_dq(dq);
-
-	spin_lock_irq(&nvmeq->q_lock);
-	nvme_process_cq(nvmeq);
-	spin_unlock_irq(&nvmeq->q_lock);
-}
-
-static int adapter_async_del_queue(struct nvme_queue *nvmeq, u8 opcode,
-						kthread_work_func_t fn)
-{
-	struct request *req;
-	struct nvme_command c;
-
-	memset(&c, 0, sizeof(c));
-	c.delete_queue.opcode = opcode;
-	c.delete_queue.qid = cpu_to_le16(nvmeq->qid);
-
-	init_kthread_work(&nvmeq->cmdinfo.work, fn);
-
-	req = nvme_alloc_request(nvmeq->dev->ctrl.admin_q, &c, 0);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
-
-	req->timeout = ADMIN_TIMEOUT;
-	req->end_io_data = &nvmeq->cmdinfo;
-	blk_execute_rq_nowait(req->q, NULL, req, 0, async_cmd_info_endio);
-	return 0;
-}
-
-static void nvme_del_cq_work_handler(struct kthread_work *work)
-{
-	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue,
-							cmdinfo.work);
-	nvme_del_queue_end(nvmeq);
-}
-
-static int nvme_delete_cq(struct nvme_queue *nvmeq)
-{
-	return adapter_async_del_queue(nvmeq, nvme_admin_delete_cq,
-						nvme_del_cq_work_handler);
-}
-
-static void nvme_del_sq_work_handler(struct kthread_work *work)
-{
-	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue,
-							cmdinfo.work);
-	int status = nvmeq->cmdinfo.status;
-
-	if (!status)
-		status = nvme_delete_cq(nvmeq);
-	if (status)
-		nvme_del_queue_end(nvmeq);
-}
+	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue, work);
+	struct nvme_dev *dev = nvmeq->dev;
 
-static int nvme_delete_sq(struct nvme_queue *nvmeq)
-{
-	return adapter_async_del_queue(nvmeq, nvme_admin_delete_sq,
-						nvme_del_sq_work_handler);
-}
+	nvme_disable_queue(dev, nvmeq->qid);
 
-static void nvme_del_queue_start(struct kthread_work *work)
-{
-	struct nvme_queue *nvmeq = container_of(work, struct nvme_queue,
-							cmdinfo.work);
-	if (nvme_delete_sq(nvmeq))
-		nvme_del_queue_end(nvmeq);
+	if (atomic_dec_and_test(&dev->queues_remaining))
+		wake_up(&dev->queue_delete_wait);
 }
 
+/*
+ * Disable all I/O queues.
+ *
+ * We use the system unboud workqueue to allow parallel execution of
+ * long-running deletes.
+ */
 static void nvme_disable_io_queues(struct nvme_dev *dev)
 {
 	int i;
-	DEFINE_KTHREAD_WORKER_ONSTACK(worker);
-	struct nvme_delq_ctx dq;
-	struct task_struct *kworker_task = kthread_run(kthread_worker_fn,
-					&worker, "nvme%d", dev->ctrl.instance);
-
-	if (IS_ERR(kworker_task)) {
-		dev_err(dev->dev,
-			"Failed to create queue del task\n");
-		for (i = dev->queue_count - 1; i > 0; i--)
-			nvme_disable_queue(dev, i);
-		return;
-	}
 
-	dq.waiter = NULL;
-	atomic_set(&dq.refcount, 0);
-	dq.worker = &worker;
+	atomic_set(&dev->queues_remaining, dev->queue_count - 1);
+	init_waitqueue_head(&dev->queue_delete_wait);
+
 	for (i = dev->queue_count - 1; i > 0; i--) {
 		struct nvme_queue *nvmeq = dev->queues[i];
 
-		if (nvme_suspend_queue(nvmeq))
-			continue;
-		nvmeq->cmdinfo.ctx = nvme_get_dq(&dq);
-		nvmeq->cmdinfo.worker = dq.worker;
-		init_kthread_work(&nvmeq->cmdinfo.work, nvme_del_queue_start);
-		queue_kthread_work(dq.worker, &nvmeq->cmdinfo.work);
+		INIT_WORK(&nvmeq->work, nvme_disable_queue_work);
+		queue_work(system_unbound_wq, &nvmeq->work);
 	}
-	nvme_wait_dq(&dq, dev);
-	kthread_stop(kworker_task);
+
+	wait_event(dev->queue_delete_wait,
+		   atomic_read(&dev->queues_remaining) == 0);
 }
 
 static int nvme_dev_list_add(struct nvme_dev *dev)
-- 
1.9.1

[PATCH 5/5] NVMe: IO queue deletion re-write

[Keith Busch]

On Sun, Jan 03, 2016@03:40:52AM -0800, Christoph Hellwig wrote:
> How about something like the lightly tested patch below.  It uses
> synchronous command submission, but schedules a work item on the
> system unbound workqueue for each queue, allowing the scheduler
> to execture them in parallel.

This works if everything else works, but the failure cases are the hard
ones. This'll deadlock if the controller stops responding during a reset,
which might be why the reset occured in the first place, and we can't
invoke another reset to clean up a failed reset.

We can use "wait_event_timeout" to fix the deadlock in the reset handler.
The handler will cancel IO's, ending work queue items waiting for command
responses. But that's only half of it. You'll also need something to end
work waiting for a request when more queues exist than admin tags.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Christoph Hellwig]

On Sun, Jan 03, 2016@03:43:31PM +0000, Keith Busch wrote:
> On Sun, Jan 03, 2016@03:40:52AM -0800, Christoph Hellwig wrote:
> > How about something like the lightly tested patch below.  It uses
> > synchronous command submission, but schedules a work item on the
> > system unbound workqueue for each queue, allowing the scheduler
> > to execture them in parallel.
> 
> This works if everything else works, but the failure cases are the hard
> ones. This'll deadlock if the controller stops responding during a reset,
> which might be why the reset occured in the first place, and we can't
> invoke another reset to clean up a failed reset.

We'd get a reset for both cases, which isn't really what we what.
I think we should be setting NVME_CTRL_RESETTING before doing a shutdown
so that errors get reported in line.

> You'll also need something to end
> work waiting for a request when more queues exist than admin tags.

It's called the block layer.  blk_mq_alloc_request will block until
the tag is available unless we explicitly request non-blocking behavior.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Keith Busch]

On Sun, Jan 03, 2016@08:17:04AM -0800, Christoph Hellwig wrote:
> On Sun, Jan 03, 2016@03:43:31PM +0000, Keith Busch wrote:
> We'd get a reset for both cases, which isn't really what we what.
> I think we should be setting NVME_CTRL_RESETTING before doing a shutdown
> so that errors get reported in line.

We hold a mutex while resetting. The second reset will be stuck there.
 
> > You'll also need something to end
> > work waiting for a request when more queues exist than admin tags.
> 
> It's called the block layer.  blk_mq_alloc_request will block until
> the tag is available unless we explicitly request non-blocking behavior.

In the scenario I'm describing you _don't_ want it to succeed in getting
a request. You don't want it to wait indefinitely for one either.

[PATCH 5/5] NVMe: IO queue deletion re-write

[Keith Busch]

On Sun, Jan 03, 2016@04:26:25PM +0000, Keith Busch wrote:
> In the scenario I'm describing you _don't_ want it to succeed in getting
> a request. You don't want it to wait indefinitely for one either.

I may be doing a bad job explaining this scenario. I'm sure it'll make
sense if you synthesize it.

Here's one way: run qemu with this patch and "-smp" set to 8. In the
linux nvme driver, set NVME_AQ_DEPTH to something lower than 8, like
4. Then reset the controller in the guest. This will fake an unresponsive
controller during reset.

---
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 169e4fa..6433337 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -305,6 +305,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd *cmd)
     if (!qid || nvme_check_sqid(n, qid)) {
         return NVME_INVALID_QID | NVME_DNR;
     }
+    return NVME_NO_COMPLETE;
 
     sq = n->sq[qid];
     while (!QTAILQ_EMPTY(&sq->out_req_list)) {
--

[PATCH 5/5] NVMe: IO queue deletion re-write

[Sagi Grimberg]

On 03/01/2016 13:40, Christoph Hellwig wrote:
> On Sat, Jan 02, 2016@09:30:09PM +0000, Keith Busch wrote:
>> The async deletion was written for a bug reporting "hang" on a device
>> removal. The "hang" was the controller taking on the order of 100's msec
>> to delete a queue (sometimes >1sec if lots of commands queued). This
>> controller had 2k queues, and took ~15 minutes to remove serially. Async
>> deletion brought it down to ~20 seconds, so looked like a good idea.
>>
>> It wasn't a controller I make, so I personally don't care about
>> parallelizing queue deletion. The driver's been this way for so long
>> though, I don't have a good way to know how beneficial this feature
>> is anymore.
>
> How about something like the lightly tested patch below.  It uses
> synchronous command submission, but schedules a work item on the
> system unbound workqueue for each queue, allowing the scheduler
> to execture them in parallel.
>
> ---
> From: Christoph Hellwig <hch at lst.de>
> Date: Sun, 3 Jan 2016 12:09:36 +0100
> Subject: nvme: semi-synchronous queue deletion
>
> Replace the complex async queue deletetion scheme with a a work_item
> per queue that is scheduled to the system unbound workqueue.  That
> way we can use the normal synchronous command submission helpers,
> but let the scheduler distribute the deletions over all available
> CPUs.
>
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>   drivers/nvme/host/pci.c | 180 +++++++-----------------------------------------
>   1 file changed, 25 insertions(+), 155 deletions(-)
>
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index b82bbea..68ba2d4 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -89,13 +89,6 @@ static void nvme_process_cq(struct nvme_queue *nvmeq);
>   static void nvme_remove_dead_ctrl(struct nvme_dev *dev);
>   static void nvme_dev_shutdown(struct nvme_dev *dev);
>
> -struct async_cmd_info {
> -	struct kthread_work work;
> -	struct kthread_worker *worker;
> -	int status;
> -	void *ctx;
> -};
> -
>   /*
>    * Represents an NVM Express device.  Each nvme_dev is a PCI function.
>    */
> @@ -128,6 +121,10 @@ struct nvme_dev {
>   #define NVME_CTRL_RESETTING    0
>
>   	struct nvme_ctrl ctrl;
> +
> +	/* for queue deletion at shutdown time */
> +	atomic_t		queues_remaining;
> +	wait_queue_head_t	queue_delete_wait;

General question,

Any reason why you didn't just use a counting semaphore for this?
I've seen other places where people are moving away from those but
I didn't understand why...