Second attempt at blk-mq + nvme hotplug fixes
Subsystems (2)
v2 cover Second attempt at blk-mq + nvme hotplug fixes
Second try, this time tested against many more scenarios than before with error injection and surprise hot-removal and intermittent resets.
I'm adding a lot of stuff outside the driver, but I didn't find a cleaner way to a lot of these things. This makes me a little nervous, so please let me know if anything seems amiss here. I don't think any of the blk-mq changes could possibly be harmful to anyone else since nvme is the only driver that uses most of the additions.
The only issue remaining I found is unfreezing queues might tigger the percpu_ref_reinit WARN_ON_ONCE when the driver restarts a request_queue with queued up IO's.
This is against linux-block/for-next.
Jens,
I believe fourth one ("abort requeue list") is from you, but I didn't find the patch.
Keith Busch (10):
blk-mq: Wake tasks entering a dying queue
blk-mq: Export test for started requests
blk-mq: Let drivers cancel requeue_work
blk-mq: Export abort requeue list
blk-mq: Allow requests to never expire
blk-mq: End unstarted requests on a dying queue
NVMe: Start driver allocated requests
NVMe: Start and stop h/w queues on reset
NVMe: Admin queue error handling
NVMe: Command abort handling fixes
block/blk-mq.c | 46 ++++++++++++++- drivers/block/nvme-core.c | 142 +++++++++++++++++++++++++++++++-------------- include/linux/blk-mq.h | 3 + include/linux/blkdev.h | 1 + 4 files changed, 146 insertions(+), 46 deletions(-)
Signature · 1 line
1.7.10.4
PATCH v2 1/10 blk-mq: Wake tasks entering queue on dying
When the queue is set to dying, wake up tasks that are waiting on frozen queue so they realize it is dying and abandon their request.
--- block/blk-mq.c | 1 + 1 file changed, 1 insertion(+)
block/blk-mq.c +1 -0
@@ -160,6 +160,7 @@ void blk_mq_wake_waiters(struct request_queue *q)queue_for_each_hw_ctx (q, hctx, i)if (blk_mq_hw_queue_mapped (hctx))blk_mq_tag_wakeup_all (hctx->tags, true); +wake_up_all (&q->mq_freeze_wq); }bool blk_mq_can_queue (struct blk_mq_hw_ctx *hctx)
Signature · 1 line
1.7.10.4
PATCH v2 1/10 blk-mq: Wake tasks entering queue on dying
Quoted 13 lines · Keith Busch
When the queue is set to dying, wake up tasks that are waiting on frozen queue so they realize it is dying and abandon their request.
Signed-off-by Keith Busch <keith.busch at intel.com>--- block/blk-mq.c | 1 + 1 file changed, 1 insertion(+)block/blk-mq.c +0
-0
block/blk-mq.c void blk_mq_wake_waiters(struct request_queue *q) · line 166
Quoted 7 lines
block/blk-mq.c +1
-0@@ -160,6 +160,7 @@ void blk_mq_wake_waiters(struct request_queue *q)queue_for_each_hw_ctx (q, hctx, i)if (blk_mq_hw_queue_mapped (hctx))blk_mq_tag_wakeup_all (hctx->tags, true); +wake_up_all (&q->mq_freeze_wq); }bool blk_mq_can_queue (struct blk_mq_hw_ctx *hctx)
That looks sane, thanks.
Signature · 1 line
Jens Axboe
PATCH v2 2/10 blk-mq: Export test for started requests
In reply to Keith BuschDrivers can iterate over all allocated request tags, but their callback needs a way to know if the driver started the request in the first place.
--- block/blk-mq.c | 6 ++++++ include/linux/blk-mq.h | 1 + 2 files changed, 7 insertions(+)
block/blk-mq.c +6 -0
@@ -398,6 +398,12 @@ void blk_mq_complete_request(struct request *rq) }EXPORT_SYMBOL (blk_mq_complete_request); +int blk_mq_request_started (struct request *rq) +{ +return test_bit (REQ_ATOM_STARTED , &rq->atomic_flags); +} +EXPORT_SYMBOL_GPL (blk_mq_request_started); +void blk_mq_start_request (struct request *rq) {struct request_queue *q = rq->q;
include/linux/blk-mq.h +1 -0
@@ -195,6 +195,7 @@ static inline u16 blk_mq_unique_tag_to_tag(u32 unique_tag)struct blk_mq_hw_ctx *blk_mq_map_queue (struct request_queue *,const int ctx_index);struct blk_mq_hw_ctx *blk_mq_alloc_single_hw_queue (struct blk_mq_tag_set *,unsigned int ,int ); +int blk_mq_request_started (struct request *rq);void blk_mq_start_request (struct request *rq);void blk_mq_end_request (struct request *rq,int error);void __blk_mq_end_request (struct request *rq,int error);
Signature · 1 line
1.7.10.4
PATCH v2 3/10 blk-mq: Let drivers cancel requeue_work
In reply to Keith BuschRequeueing requests run h/w queues in a work_queue, which may alter the driver's requested state to temporarily stop them. This patch exports a method to cancel the q->requeue_work so a driver can be assured it's stopped h/w queues won't be started up when it is not ready.
--- block/blk-mq.c | 6 ++++++ include/linux/blk-mq.h | 1 + 2 files changed, 7 insertions(+)
block/blk-mq.c +6 -0
@@ -521,6 +521,12 @@ void blk_mq_add_to_requeue_list(struct request *rq, bool at_head) }EXPORT_SYMBOL (blk_mq_add_to_requeue_list); +void blk_mq_cancel_requeue_work (struct request_queue *q) +{ +cancel_work_sync (&q->requeue_work); +} +EXPORT_SYMBOL_GPL (blk_mq_cancel_requeue_work); +void blk_mq_kick_requeue_list (struct request_queue *q) {kblockd_schedule_work (&q->requeue_work);
include/linux/blk-mq.h +1 -0
@@ -202,6 +202,7 @@ void __blk_mq_end_request(struct request *rq, int error);void blk_mq_requeue_request (struct request *rq);void blk_mq_add_to_requeue_list (struct request *rq,bool at_head); +void blk_mq_cancel_requeue_work (struct request_queue *q);void blk_mq_kick_requeue_list (struct request_queue *q);void blk_mq_complete_request (struct request *rq);
Signature · 1 line
1.7.10.4
PATCH v2 4/10 blk-mq: Export abort requeue list
In reply to Keith BuschThis patch lets a driver abort all requeued requests in case there are any pending, but the h/w queue will never become available again.
--- block/blk-mq.c | 20 ++++++++++++++++++++ include/linux/blk-mq.h | 1 + 2 files changed, 21 insertions(+)
block/blk-mq.c +20 -0
@@ -533,6 +533,26 @@ void blk_mq_kick_requeue_list(struct request_queue *q) }EXPORT_SYMBOL (blk_mq_kick_requeue_list); +void blk_mq_abort_requeue_list (struct request_queue *q) +{ +unsigned long flags; +LIST_HEAD (rq_list); + +spin_lock_irqsave (&q->requeue_lock, flags); +list_splice_init (&q->requeue_list, &rq_list); +spin_unlock_irqrestore (&q->requeue_lock, flags); + +while (!list_empty (&rq_list)) { +struct request *rq; + + rq = list_first_entry(&rq_list, struct request, queuelist); +list_del_init (&rq->queuelist); + rq->errors = -EIO ; +blk_mq_end_request (rq, rq->errors); + } +} +EXPORT_SYMBOL (blk_mq_abort_requeue_list); +static inline bool is_flush_request (struct request *rq,struct blk_flush_queue *fq,unsigned int tag) {
include/linux/blk-mq.h +1 -0
@@ -204,6 +204,7 @@ void blk_mq_requeue_request(struct request *rq);void blk_mq_add_to_requeue_list (struct request *rq,bool at_head);void blk_mq_cancel_requeue_work (struct request_queue *q);void blk_mq_kick_requeue_list (struct request_queue *q); +void blk_mq_abort_requeue_list (struct request_queue *q);void blk_mq_complete_request (struct request *rq);void blk_mq_stop_hw_queue (struct blk_mq_hw_ctx *hctx);
Signature · 1 line
1.7.10.4
PATCH v2 5/10 blk-mq: Allow requests to never expire
In reply to Keith BuschSome requests may be started but have no gaurantee they'll ever complete. This defines a special timeout value that a driver can use so the request will never be timed out.
--- block/blk-mq.c | 8 +++++--- include/linux/blkdev.h | 1 + 2 files changed, 6 insertions(+), 3 deletions(-)
block/blk-mq.c +5 -3
@@ -414,7 +414,8 @@ void blk_mq_start_request(struct request *rq)if (unlikely (blk_bidi_rq (rq))) rq->next_rq->resid_len =blk_rq_bytes (rq->next_rq); -blk_add_timer (rq); +if (rq->timeout !=REQ_NO_TIMEOUT ) +blk_add_timer (rq);/* * Ensure that ->deadline is visible before set the started
@@ -613,13 +614,14 @@ void blk_mq_rq_timed_out(struct request *req, bool reserved)break ; } } - +static void blk_mq_check_expired (struct blk_mq_hw_ctx *hctx,struct request *rq,void *priv,bool reserved) {struct blk_mq_timeout_data *data = priv; -if (!test_bit (REQ_ATOM_STARTED , &rq->atomic_flags)) +if (!test_bit (REQ_ATOM_STARTED , &rq->atomic_flags) || + rq->timeout ==REQ_NO_TIMEOUT )return ;if (time_after_eq (jiffies, rq->deadline)) {
include/linux/blkdev.h +1 -0
@@ -198,6 +198,7 @@ struct request {unsigned long deadline;struct list_head timeout_list; +#define REQ_NO_TIMEOUT UINT_MAXunsigned int timeout;int retries;
Signature · 1 line
1.7.10.4
PATCH v2 5/10 blk-mq: Allow requests to never expire
Quoted 3 lines · Keith Busch
Some requests may be started but have no gaurantee they'll ever complete. This defines a special timeout value that a driver can use so the request will never be timed out.
What if the timeout just happens to be UINT_MAX for the regular case? I think it'd be a lot safer to add a specific no-timeout flag to the request instead.
Signature · 1 line
Jens Axboe
PATCH v2 6/10 blk-mq: End unstarted requests on a dying queue
In reply to Keith BuschRequests that haven't been started prior to a queue dying can be ended in error without having to wait for them to time out.
--- block/blk-mq.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
block/blk-mq.c +6 -1
@@ -621,8 +621,13 @@ static void blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,struct blk_mq_timeout_data *data = priv;if (!test_bit (REQ_ATOM_STARTED , &rq->atomic_flags) || - rq->timeout ==REQ_NO_TIMEOUT ) + rq->timeout ==REQ_NO_TIMEOUT ) { +if (unlikely (blk_queue_dying (rq->q))) { + rq->errors = -EIO ; +blk_mq_complete_request (rq); + }return ; + }if (time_after_eq (jiffies, rq->deadline)) {if (!blk_mark_rq_complete (rq))
Signature · 1 line
1.7.10.4
PATCH v2 7/10 NVMe: Start driver allocated requests
In reply to Keith BuschOnce the nvme callback is set for a request, the driver can start it and make it available for timeout handling. For timed out commands on a device that is not initialized, this fixes potential deadlocks that can occur on startup and shutdown when a device is unresponsive since they can now be cancelled.
Asynchronous requests do not have any expected timeout, so these are using the new "REQ_NO_TIMEOUT".
--- drivers/block/nvme-core.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/block/nvme-core.c +12 -4
@@ -215,6 +215,7 @@ static void nvme_set_info(struct nvme_cmd_info *cmd, void *ctx, cmd->fn = handler; cmd->ctx = ctx; cmd->aborted =0 ; +blk_mq_start_request (blk_mq_rq_from_pdu (cmd)); }/* Special values must be less than 0x1000 */
@@ -664,8 +665,6 @@ static int nvme_queue_rq(struct blk_mq_hw_ctx *hctx, } } -blk_mq_start_request (req); -nvme_set_info (cmd, iod, req_completion);spin_lock_irq (&nvmeq->q_lock);if (req->cmd_flags &REQ_DISCARD )
@@ -835,6 +834,7 @@ static int nvme_submit_async_admin_req(struct nvme_dev *dev)if (IS_ERR (req))return PTR_ERR (req); + req->timeout =REQ_NO_TIMEOUT ; cmd_info =blk_mq_rq_to_pdu (req);nvme_set_info (cmd_info, req, async_req_completion);
@@ -1086,8 +1086,16 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)dev_warn (nvmeq->q_dmadev,"Timeout I/O %d QID %d\n" , req->tag, nvmeq->qid); -if (nvmeq->dev->initialized) -nvme_abort_req (req); + +if (!nvmeq->dev->initialized) { +/* +* Force cancelled command frees the request, which requires we +* return BLK_EH_NOT_HANDLED. +*/ +nvme_cancel_queue_ios (nvmeq->hctx, req, nvmeq, reserved); +return BLK_EH_NOT_HANDLED ; + } +nvme_abort_req (req);/* * The aborted req will be completed on receiving the abort req.
Signature · 1 line
1.7.10.4
PATCH v2 8/10 NVMe: Start and stop h/w queues on reset
In reply to Keith BuschThis freezes and stops all the queues on device shutdown and restarts them on resume, and fixes hotplug and reset issues when the controller is actively being used.
--- drivers/block/nvme-core.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-)
drivers/block/nvme-core.c +39 -3
@@ -433,7 +433,10 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,if (!(status &NVME_SC_DNR ||blk_noretry_request (req)) && (jiffies - req->start_time) < req->timeout) {blk_mq_requeue_request (req); -blk_mq_kick_requeue_list (req->q); +spin_lock (req->q->queue_lock); +if (!blk_queue_stopped (req->q)) +blk_mq_kick_requeue_list (req->q); +spin_unlock (req->q->queue_lock);return ; } req->errors =nvme_error_status (status);
@@ -2391,6 +2394,34 @@ static void nvme_dev_list_remove(struct nvme_dev *dev)kthread_stop (tmp); } +static void nvme_freeze_queues (struct nvme_dev *dev) +{ +struct nvme_ns *ns; + +list_for_each_entry (ns, &dev->namespaces, list) { +blk_mq_freeze_queue_start (ns->queue); + +spin_lock (ns->queue->queue_lock); +queue_flag_set (QUEUE_FLAG_STOPPED , ns->queue); +spin_unlock (ns->queue->queue_lock); + +blk_mq_cancel_requeue_work (ns->queue); +blk_mq_stop_hw_queues (ns->queue); + } +} + +static void nvme_unfreeze_queues (struct nvme_dev *dev) +{ +struct nvme_ns *ns; + +list_for_each_entry (ns, &dev->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); + } +} +static void nvme_dev_shutdown (struct nvme_dev *dev) {int i;
@@ -2399,8 +2430,10 @@ static void nvme_dev_shutdown(struct nvme_dev *dev) dev->initialized =0 ;nvme_dev_list_remove (dev); -if (dev->bar) +if (dev->bar) { +nvme_freeze_queues (dev); csts =readl (&dev->bar->csts); + }if (csts &NVME_CSTS_CFS || !(csts &NVME_CSTS_RDY )) {for (i = dev->queue_count -1 ; i >=0 ; i--) {struct nvme_queue *nvmeq = dev->queues[i];
@@ -2654,6 +2687,9 @@ static int nvme_dev_resume(struct nvme_dev *dev) dev->reset_workfn = nvme_remove_disks;queue_work (nvme_workq, &dev->reset_work);spin_unlock (&dev_list_lock); + }else { +nvme_unfreeze_queues (dev); +nvme_set_irq_hints (dev); } dev->initialized =1 ;return 0 ;
@@ -2791,8 +2827,8 @@ static void nvme_remove(struct pci_dev *pdev)pci_set_drvdata (pdev,NULL );flush_work (&dev->reset_work);misc_deregister (&dev->miscdev); -nvme_dev_remove (dev);nvme_dev_shutdown (dev); +nvme_dev_remove (dev);nvme_dev_remove_admin (dev);nvme_free_queues (dev,0 );nvme_free_admin_tags (dev);
Signature · 1 line
1.7.10.4
PATCH v2 8/10 NVMe: Start and stop h/w queues on reset
drivers/block/nvme-core.c static void req_completion(struct nvme_queue *nvmeq, void *ctx, · line 441
Quoted 11 lines · Keith Busch
drivers/block/nvme-core.c +4
-1@@ -433,7 +433,10 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,if (!(status &NVME_SC_DNR ||blk_noretry_request (req)) && (jiffies - req->start_time) < req->timeout) {blk_mq_requeue_request (req); -blk_mq_kick_requeue_list (req->q); +spin_lock (req->q->queue_lock); +if (!blk_queue_stopped (req->q)) +blk_mq_kick_requeue_list (req->q); +spin_unlock (req->q->queue_lock);return ; }
Ugh, been at this too long and screwed up my hand-merge when squashing and splitting into managable patches. The above should use irq save/restore locks.
@@ -433,7 +433,10 @@ static void req_completion(struct nvme_queue *nvmeq, void *ctx,
if (!(status & NVME_SC_DNR || blk_noretry_request(req))
&& (jiffies - req->start_time) < req->timeout) {
+ int flags;
blk_mq_requeue_request(req);
- blk_mq_kick_requeue_list(req->q);
+ spin_lock_irqsave(req->q->queue_lock, flags);
+ if (!blk_queue_stopped(req->q))
+ blk_mq_kick_requeue_list(req->q);
+ spin_unlock_irqrestore(req->q->queue_lock, flags);
return;
}
PATCH v2 9/10 NVMe: Admin queue error handling
In reply to Keith BuschThis protects the admin queue access in a variety of scenarios:
Its request_queue is reference counted once it is allocated so it can't be deleted while there is an open reference on the controller and fixes a use-after-free error on some hot-remove scenarios.
The queue is frozen on reset after all IO queues have been deleted since the controller cannot accept commands after this anyway, so new requests will block until the reset completes. Since the queue has to be unfrozen, the function doing that was moved to the point after the h/w queue was initialized, which is probably where it should have gone in the first place.
Special handling is done if the controller becomes unresponsive on a shutdown to forcefully cancel commands, and then wait for the remaining queue deletion workers to finish.
This patch also removed the unnecessary software signals that were previously used to signal the worker thread to die, but that mechanism is not used in this path anymore, and fixed the signedness on cq_vector so we don't disable a queue twice.
--- drivers/block/nvme-core.c | 67 +++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 32 deletions(-)
drivers/block/nvme-core.c +35 -32
@@ -106,7 +106,7 @@ struct nvme_queue {dma_addr_t cq_dma_addr;u32 __iomem *q_db;u16 q_depth; -u16 cq_vector; +s16 cq_vector;u16 sq_head;u16 sq_tail;u16 cq_head;
@@ -1186,6 +1186,8 @@ static void nvme_disable_queue(struct nvme_dev *dev, int qid)adapter_delete_sq (dev, qid);adapter_delete_cq (dev, qid); } +if (!qid) +blk_mq_freeze_queue_start (dev->admin_q);nvme_clear_queue (nvmeq); }
@@ -1372,6 +1374,14 @@ static struct blk_mq_ops nvme_mq_ops = { .timeout = nvme_timeout, }; +static void nvme_dev_remove_admin (struct nvme_dev *dev) +{ +if (dev->admin_q && !blk_queue_dying (dev->admin_q)) { +blk_cleanup_queue (dev->admin_q); +blk_mq_free_tag_set (&dev->admin_tagset); + } +} +static int nvme_alloc_admin_tags (struct nvme_dev *dev) {if (!dev->admin_q) {
@@ -1391,17 +1401,16 @@ static int nvme_alloc_admin_tags(struct nvme_dev *dev)blk_mq_free_tag_set (&dev->admin_tagset);return -ENOMEM ; } - } +if (!blk_get_queue (dev->admin_q)) { +nvme_dev_remove_admin (dev); +return -ENODEV ; + } + }else +blk_mq_unfreeze_queue (dev->admin_q);return 0 ; } -static void nvme_free_admin_tags (struct nvme_dev *dev) -{ -if (dev->admin_q) -blk_mq_free_tag_set (&dev->admin_tagset); -} -static int nvme_configure_admin_queue (struct nvme_dev *dev) {int result;
@@ -1456,19 +1465,13 @@ static int nvme_configure_admin_queue(struct nvme_dev *dev)if (result) goto free_nvmeq; - result =nvme_alloc_admin_tags (dev); -if (result) - goto free_nvmeq; - nvmeq->cq_vector =0 ; result =queue_request_irq (dev, nvmeq, nvmeq->irqname);if (result) - goto free_tags; + goto free_nvmeq;return result; - free_tags: -nvme_free_admin_tags (dev); free_nvmeq:nvme_free_queues (dev,0 );return result;
@@ -2251,15 +2254,19 @@ static void nvme_wait_dq(struct nvme_delq_ctx *dq, struct nvme_dev *dev)set_current_state (TASK_KILLABLE );if (!atomic_read (&dq->refcount))break ; -if (!schedule_timeout (ADMIN_TIMEOUT ) || -fatal_signal_pending (current)) { +if (!schedule_timeout (ADMIN_TIMEOUT )) { +/* +* 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. This +* may take a while if there are more h/w queues than +* admin tags. +*/ set_current_state (TASK_RUNNING ); -nvme_disable_ctrl (dev,readq (&dev->bar->cap)); -nvme_disable_queue (dev,0 ); - -send_sig (SIGKILL , dq->worker->task,1 ); +nvme_clear_queue (dev->queues[0 ]);flush_kthread_worker (dq->worker); +nvme_disable_queue (dev,0 );return ; } }
@@ -2336,7 +2343,6 @@ static void nvme_del_queue_start(struct kthread_work *work) { struct nvme_queue *nvmeq = container_of(work, struct nvme_queue, cmdinfo.work); -allow_signal (SIGKILL );if (nvme_delete_sq (nvmeq))nvme_del_queue_end (nvmeq); }
@@ -2448,12 +2454,6 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)nvme_dev_unmap (dev); } -static void nvme_dev_remove_admin (struct nvme_dev *dev) -{ -if (dev->admin_q && !blk_queue_dying (dev->admin_q)) -blk_cleanup_queue (dev->admin_q); -} -static void nvme_dev_remove (struct nvme_dev *dev) {struct nvme_ns *ns;
@@ -2543,6 +2543,7 @@ static void nvme_free_dev(struct kref *kref)nvme_free_namespaces (dev);nvme_release_instance (dev);blk_mq_free_tag_set (&dev->tagset); +blk_put_queue (dev->admin_q);kfree (dev->queues);kfree (dev->entry);kfree (dev);
@@ -2639,15 +2640,18 @@ static int nvme_dev_start(struct nvme_dev *dev) }nvme_init_queue (dev->queues[0 ],0 ); - - result =nvme_setup_io_queues (dev); + result =nvme_alloc_admin_tags (dev);if (result) goto disable; -nvme_set_irq_hints (dev); + result =nvme_setup_io_queues (dev); +if (result) + goto free_tags;return result; + free_tags: +nvme_dev_remove_admin (dev); disable:nvme_disable_queue (dev,0 );nvme_dev_list_remove (dev);
@@ -2831,7 +2835,6 @@ static void nvme_remove(struct pci_dev *pdev)nvme_dev_remove (dev);nvme_dev_remove_admin (dev);nvme_free_queues (dev,0 ); -nvme_free_admin_tags (dev);nvme_release_prp_pools (dev);kref_put (&dev->kref, nvme_free_dev); }
Signature · 1 line
1.7.10.4
PATCH v2 10/10 NVMe: Command abort handling fixes
In reply to Keith BuschAborts all requeued commands prior to killing the request_queue. For commands that time out on a dying request queue, set the "Do Not Retry" bit on the command status so the command cannot be requeued. Finanally, if the driver is requested to abort a command it did not start, do nothing.
--- drivers/block/nvme-core.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/block/nvme-core.c +13 -4
@@ -1067,15 +1067,22 @@ static void nvme_cancel_queue_ios(struct blk_mq_hw_ctx *hctx,void *ctx;nvme_completion_fn fn;struct nvme_cmd_info *cmd; -static struct nvme_completion cqe = { - .status =cpu_to_le16 (NVME_SC_ABORT_REQ <<1 ), - }; +struct nvme_completion cqe; + +if (!blk_mq_request_started (req)) +return ; cmd =blk_mq_rq_to_pdu (req);if (cmd->ctx ==CMD_CTX_CANCELLED )return ; +if (blk_queue_dying (req->q)) + cqe.status =cpu_to_le16 ((NVME_SC_ABORT_REQ |NVME_SC_DNR ) <<1 ); +else + cqe.status =cpu_to_le16 (NVME_SC_ABORT_REQ <<1 ); + +dev_warn (nvmeq->q_dmadev,"Cancelling I/O %d QID %d\n" , req->tag, nvmeq->qid); ctx =cancel_cmd_info (cmd, &fn);
@@ -2461,8 +2468,10 @@ static void nvme_dev_remove(struct nvme_dev *dev)list_for_each_entry (ns, &dev->namespaces, list) {if (ns->disk->flags &GENHD_FL_UP )del_gendisk (ns->disk); -if (!blk_queue_dying (ns->queue)) +if (!blk_queue_dying (ns->queue)) { +blk_mq_abort_requeue_list (ns->queue);blk_cleanup_queue (ns->queue); + } } }
Signature · 1 line
1.7.10.4
v2 cover Second attempt at blk-mq + nvme hotplug fixes
In reply to Keith BuschQuoted 9 lines · Keith Busch
Second try, this time tested against many more scenarios than before with error injection and surprise hot-removal and intermittent resets.
I'm adding a lot of stuff outside the driver, but I didn't find a cleaner way to a lot of these things. This makes me a little nervous, so please let me know if anything seems amiss here. I don't think any of the blk-mq changes could possibly be harmful to anyone else since nvme is the only driver that uses most of the additions.
Quoted 5 lines
The only issue remaining I found is unfreezing queues might tigger the
percpu_ref_reinitWARN_ON_ONCEwhen the driver restarts arequest_queuewith queued up IO's.This is against linux-block/for-next.
Hi Keith,
Tested with qemu-nvme. Hotplug seems work., but has some issues.
On guest: run fio for a while
root at block:~# fio --name=global --filename=/dev/nvme0n1 --direct=1 --bs=4k \
--rw=randrw --ioengine=libaio --iodepth=128 --name=foobarfoobar: (g=0): rw=randrw, bs=4K-4K/4K-4K, ioengine=libaio, iodepth=128 2.0.8
Starting 1 process
Jobs: 1 (f=1): [m] [10.3% done] [3746K/3980K /s] [936 /995 iops] [eta 08m:52s]s]
On guest: then remove nvme device
root at block:~# time echo 1 > /sys/devices/pci0000:00/0000:00:04.0/remove real 1m0.040s
user 0m0.000s
sys 0m0.012
It works, but took long time(1 minutes) to return.
And during this time frame, the whole qemu system was not responsible. Ping from host to guest also failed.
mlin at minggr:~$ ping 192.168.122.89
PING 192.168.122.89 (192.168.122.89) 56(84) bytes of data.
Quoted 2 lines
From 192.168.122.1 icmp_seq=15 Destination Host Unreachable From 192.168.122.1 icmp_seq=16 Destination Host Unreachable
Thanks,
Ming
v2 cover Second attempt at blk-mq + nvme hotplug fixes
Quoted 19 lines · Ming Lin
Hi Keith,
Tested with qemu-nvme. Hotplug seems work., but has some issues.
On guest: run fio for a while
root at block:~# fio --name=global --filename=/dev/nvme0n1 --direct=1 --bs=4k \ --rw=randrw --ioengine=libaio --iodepth=128 --name=foobarfoobar: (g=0): rw=randrw, bs=4K-4K/4K-4K, ioengine=libaio, iodepth=128 2.0.8
Starting 1 process
Jobs: 1 (f=1): [m] [10.3% done] [3746K/3980K /s] [936 /995 iops] [eta 08m:52s]s]On guest: then remove nvme device
root at block:~# time echo 1 > /sys/devices/pci0000:00/0000:00:04.0/remove real 1m0.040s
user 0m0.000s
sys 0m0.012
Quoted 3 lines
It works, but took long time(1 minutes) to return.
And during this time frame, the whole qemu system was not responsible. Ping from host to guest also failed.
Thanks for the feedback. Is there anything special about your emulated device? I haven't been able to duplicate these results. When running the same fio job, I get ~0.4s on qemu, and ~2.2s on real h/w.
A 1 mintute removal sounds like you're hitting the admin timeout on queue deletion.
On the other hand, your namespace backing storage appears to be really slow, so maybe it's stuck in blk_flush on shutdown. You say the guest was unresponsive during removal, so that sounds plausible since qemu's nvme shutdown is down synchronously with the CC register MMIO write.
What if you try this using a ramdisk or tmpfs file for the namespace storage instead of what you're currently using?
Or add some prints in both the driver's and emualted target's shutdown path and see where you're getting stuck?
v2 cover Second attempt at blk-mq + nvme hotplug fixes
Quoted 34 lines · Keith Busch
Quoted 21 lines · Ming Lin
Hi Keith,
Tested with qemu-nvme. Hotplug seems work., but has some issues.
On guest: run fio for a while
root at block:~# fio --name=global --filename=/dev/nvme0n1 --direct=1 --bs=4k\ --rw=randrw --ioengine=libaio --iodepth=128 --name=foobarfoobar: (g=0): rw=randrw, bs=4K-4K/4K-4K, ioengine=libaio, iodepth=128 2.0.8
Starting 1 process
Jobs: 1 (f=1): [m] [10.3% done] [3746K/3980K /s] [936 /995 iops] [eta 08m:52s]s]On guest: then remove nvme device
root at block:~# time echo 1 > /sys/devices/pci0000:00/0000:00:04.0/remove real 1m0.040s
user 0m0.000s
sys 0m0.012Quoted 3 lines
It works, but took long time(1 minutes) to return.
And during this time frame, the whole qemu system was not responsible. Ping from host to guest also failed.Thanks for the feedback. Is there anything special about your emulated device? I haven't been able to duplicate these results. When running the same fio job, I get ~0.4s on qemu, and ~2.2s on real h/w.
A 1 mintute removal sounds like you're hitting the admin timeout on queue deletion.
Quoted 7 lines
On the other hand, your namespace backing storage appears to be really slow, so maybe it's stuck in
blk_flushon shutdown. You say the guest was unresponsive during removal, so that sounds plausible since qemu's nvme shutdown is down synchronously with the CC register MMIO write.What if you try this using a ramdisk or tmpfs file for the namespace storage instead of what you're currently using?
It's good with tmpfs file.
root at block:~# time echo 1 > /sys/devices/pci0000:00/0000:00:04.0/remove real 0m0.403s
user 0m0.000s
sys 0m0.008s
Thanks.
Quoted 3 lines
Or add some prints in both the driver's and emualted target's shutdown path and see where you're getting stuck?
v2 cover Second attempt at blk-mq + nvme hotplug fixes
In reply to Keith BuschQuoted 9 lines · Keith Busch
Second try, this time tested against many more scenarios than before with error injection and surprise hot-removal and intermittent resets.
I'm adding a lot of stuff outside the driver, but I didn't find a cleaner way to a lot of these things. This makes me a little nervous, so please let me know if anything seems amiss here. I don't think any of the blk-mq changes could possibly be harmful to anyone else since nvme is the only driver that uses most of the additions.
Quoted 5 lines
The only issue remaining I found is unfreezing queues might tigger the
percpu_ref_reinitWARN_ON_ONCEwhen the driver restarts arequest_queuewith queued up IO's.This is against linux-block/for-next.
Series looks sane to me, apart from the timeout change I replied to separately. Could you turn that into a request flag and respin the series on top of that?
Quoted 3 lines
Jens,
I believe fourth one ("abort requeue list") is from you, but I didn't find the patch.
Yeah it is, sent to you on 12/23 as part of the "nvme-blkmq fixes" thread.
Signature · 1 line
Jens Axboe
v2 cover Second attempt at blk-mq + nvme hotplug fixes
Quoted 14 lines · Jens Axboe
Quoted 13 lines · Keith Busch
Second try, this time tested against many more scenarios than before with error injection and surprise hot-removal and intermittent resets.
I'm adding a lot of stuff outside the driver, but I didn't find a cleaner way to a lot of these things. This makes me a little nervous, so please let me know if anything seems amiss here. I don't think any of the blk-mq changes could possibly be harmful to anyone else since nvme is the only driver that uses most of the additions.
The only issue remaining I found is unfreezing queues might tigger the
percpu_ref_reinitWARN_ON_ONCEwhen the driver restarts arequest_queuewith queued up IO's.
Quoted 5 lines
Quoted 1 line
This is against linux-block/for-next.
Series looks sane to me, apart from the timeout change I replied to separately. Could you turn that into a request flag and respin the series on top of that?
Thanks a bunch, I'll respin the series with the no-timeout request flag, correct irq spin locking I found in [8/10], and fix attribution in [4/10].
Quoted 5 lines
Quoted 3 lines
Jens,
I believe fourth one ("abort requeue list") is from you, but I didn't find the patch.Yeah it is, sent to you on 12/23 as part of the "nvme-blkmq fixes" thread.
v2 cover Second attempt at blk-mq + nvme hotplug fixes
Quoted 15 lines · Keith Busch
Quoted 14 lines · Jens Axboe
Quoted 13 lines · Keith Busch
Second try, this time tested against many more scenarios than before with error injection and surprise hot-removal and intermittent resets.
I'm adding a lot of stuff outside the driver, but I didn't find a cleaner way to a lot of these things. This makes me a little nervous, so please let me know if anything seems amiss here. I don't think any of the blk-mq changes could possibly be harmful to anyone else since nvme is the only driver that uses most of the additions.
The only issue remaining I found is unfreezing queues might tigger the
percpu_ref_reinitWARN_ON_ONCEwhen the driver restarts arequest_queuewith queued up IO's.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
This is against linux-block/for-next.
Series looks sane to me, apart from the timeout change I replied to separately. Could you turn that into a request flag and respin the series on top of that?
Thanks a bunch, I'll respin the series with the no-timeout request flag, correct irq spin locking I found in [8/10], and fix attribution in [4/10].
Awesome, thanks Keith! And thanks for taking the time to get this sorted.
Signature · 1 line
Jens Axboe