NVMe fixes and updates for 4.5
cover NVMe fixes and updates for 4.5
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(-)
Signature · 1 line
2.6.2.307.g37023ba
PATCH 1/5 NVMe: Fix admin queue ring wrap
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.
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -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);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 1/5 NVMe: Fix admin queue ring wrap
Quoted 3 lines · 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.
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
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · 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.
You'll need to change
NVME_AQ_DEPTHto 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.
Quoted 2 lines
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
In reply to Keith BuschQuoted 14 lines · 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(-)drivers/nvme/host/pci.c +0
-0
drivers/nvme/host/pci.c static int nvme_alloc_admin_tags(struct nvme_dev *dev) · line 1277
Quoted 8 lines
drivers/nvme/host/pci.c +1
-1@@ -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.
Signature · 1 line
Jens Axboe
PATCH 1/5 NVMe: Fix admin queue ring wrap
Quoted 2 lines · Jens Axboe
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
In reply to Keith BuschUse a fake status that can potentially be retried on reset.
--- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/pci.c +1 -1
@@ -1017,7 +1017,7 @@ static void nvme_cancel_queue_ios(struct request *req, void *data, bool reserveddev_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);
Signature · 1 line
2.6.2.307.g37023ba
PATCH 2/5 NVMe: Use a retryable error code on reset
Quoted 1 line · Keith Busch
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
Quoted 10 lines · Christoph Hellwig
Quoted 1 line · Keith Busch
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_CANCELLEDisn't retryable,nvme_req_needs_retrydoesn'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.
Quoted 6 lines
Which btw brings me to a bug I noticed a while ago but didn't have time to fully understand:
nvme_req_needs_retrylooks atreq->start_timevsreq->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
Quoted 10 lines · Keith Busch
Quoted 7 lines
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_CANCELLEDisn't retryable,nvme_req_needs_retrydoesn'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_retrychecks 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.
Quoted 3 lines
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.
Quoted 7 lines
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
In reply to Keith BuschNVMe 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.
--- drivers/nvme/host/pci.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
drivers/nvme/host/pci.c +6 -9
@@ -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); }
Signature · 1 line
2.6.2.307.g37023ba
PATCH 3/5 NVMe: Remove queue freezing on resets
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.
PATCH 3/5 NVMe: Remove queue freezing on resets
Quoted 4 lines
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
In reply to Keith BuschQuoted 5 lines
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.
Quoted 2 lines
This also fixes a WARN in
percpu_ref_reinitwhen 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
Quoted 9 lines
Quoted 6 lines
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.
Quoted 4 lines
Quoted 2 lines
This also fixes a WARN in
percpu_ref_reinitwhen 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,
PATCH 4/5 NVMe: Shutdown controller only for power-off
In reply to Keith BuschWe 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.
--- drivers/nvme/host/pci.c | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-)
drivers/nvme/host/pci.c +19 -21
@@ -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 ;
Signature · 1 line
2.6.2.307.g37023ba
PATCH 4/5 NVMe: Shutdown controller only for power-off
Quoted 9 lines · 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.
Am I missing something? What happens to the calls to nvme_disable_queue in nvme_disable_io_queues and nvme_wait_dq?
Quoted 10 lines
drivers/nvme/host/pci.c +2
-2-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 ;
drivers/nvme/host/pci.c static void nvme_clear_queue(struct nvme_queue *nvmeq) · line 1094
Quoted 10 lines
drivers/nvme/host/pci.c +3
-6-/* 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
Quoted 8 lines · Christoph Hellwig
Quoted 7 lines · 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.
Quoted 5 lines
Quoted 2 lines
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_queueinnvme_disable_io_queuesandnvme_wait_dq?
My bad. I applied the patches in the wrong order when generating the series. Need to swap 4/5 with 5/5.
Quoted 6 lines
drivers/nvme/host/pci.c static void nvme_clear_queue(struct nvme_queue *nvmeq) · line 1094
Quoted 3 lines
drivers/nvme/host/pci.c +3
-0+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
Quoted 10 lines · Keith Busch
My bad. I applied the patches in the wrong order when generating the series. Need to swap 4/5 with 5/5.
Quoted 6 lines
drivers/nvme/host/pci.c static void nvme_clear_queue(struct nvme_queue *nvmeq) · line 1094
Quoted 3 lines
drivers/nvme/host/pci.c +3
-0+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?
Quoted 3 lines
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
In reply to Keith BuschThe 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.
--- drivers/nvme/host/pci.c | 256 ++++++++++++++++-------------------------------- 1 file changed, 86 insertions(+), 170 deletions(-)
drivers/nvme/host/pci.c +86 -170
@@ -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 0struct 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;
Signature · 1 line
2.6.2.307.g37023ba
PATCH 5/5 NVMe: IO queue deletion re-write
Quoted 11 lines · 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.
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:
Quoted 8 lines
drivers/nvme/host/pci.c +8
-0+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 ;
drivers/nvme/host/pci.c static void nvme_dev_scan(struct work_struct *work) · line 1633
Quoted 10 lines
drivers/nvme/host/pci.c +10
-0+ +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
Quoted 2 lines · Christoph Hellwig
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.
Quoted 2 lines
First the 'wait' workqueue is a per-controller wait, so it should be in
nvme_ctrlinstead of a union inside the queue.
Okay. I was trying to save a few bytes. Certainly not worth it.
Quoted 3 lines
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.
Quoted 5 lines
The other things is the use of
nvme_requeue_reqfor 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
Quoted 2 lines
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.
Quoted 5 lines
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.
Quoted 3 lines
But I didn't like having to store the "struct command" in the
nvme_queueeither, 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
Quoted 3 lines · Christoph Hellwig
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
Quoted 10 lines · Keith Busch
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 <[email protected]>
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.
--- drivers/nvme/host/pci.c | 180 +++++++----------------------------------------- 1 file changed, 25 insertions(+), 155 deletions(-)
drivers/nvme/host/pci.c +25 -155
@@ -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 0struct 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)
Signature · 1 line
1.9.1
PATCH 5/5 NVMe: IO queue deletion re-write
Quoted 4 lines · Christoph Hellwig
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
Quoted 10 lines · Keith Busch
Quoted 4 lines · Christoph Hellwig
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.
Quoted 2 lines
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
Quoted 4 lines · Christoph Hellwig
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 settingNVME_CTRL_RESETTINGbefore doing a shutdown so that errors get reported in line.
We hold a mutex while resetting. The second reset will be stuck there.
Quoted 5 lines
Quoted 2 lines
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_requestwill 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
Quoted 2 lines · Keith Busch
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.
---
hw/block/nvme.c +1 -0
@@ -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
In reply to Christoph HellwigQuoted 52 lines · Christoph Hellwig
Quoted 10 lines · Keith Busch
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 deletionReplace the complex async queue deletetion scheme with a a
work_itemper 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(-)drivers/nvme/host/pci.c +0
-7@@ -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. */
drivers/nvme/host/pci.c struct nvme_dev { · line 127
Quoted 7 lines
drivers/nvme/host/pci.c +4
-0@@ -128,6 +121,10 @@ struct nvme_dev {#define NVME_CTRL_RESETTING 0struct 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...