Skip to contentSkip to conversation

Second attempt at blk-mq + nvme hotplug fixes

PATCH Superseded Keith Busch 3 participants 20 messages Jan 7, 2015

Subsystems (2)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c |    1 +
 1 file changed, 1 insertion(+)
block/blk-mq.c +1 -0
index 1a41d7a..6d83ee6 100644
@@ -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
#2 lore raw

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
index 1a41d7a..6d83ee6 100644

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
#3 lore raw

PATCH v2 2/10 blk-mq: Export test for started requests

In reply to Keith Busch

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

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c         |    6 ++++++
 include/linux/blk-mq.h |    1 +
 2 files changed, 7 insertions(+)
block/blk-mq.c +6 -0
index 6d83ee6..82930c0 100644
@@ -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
index 3b43f50..8bbd082 100644
@@ -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
#4 lore raw

PATCH v2 3/10 blk-mq: Let drivers cancel requeue_work

In reply to Keith Busch

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

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c         |    6 ++++++
 include/linux/blk-mq.h |    1 +
 2 files changed, 7 insertions(+)
block/blk-mq.c +6 -0
index 82930c0..a976db4 100644
@@ -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
index 8bbd082..b509ef5 100644
@@ -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
#5 lore raw

PATCH v2 4/10 blk-mq: Export abort requeue list

In reply to Keith Busch

This patch lets a driver abort all requeued requests in case there are any pending, but the h/w queue will never become available again.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c         |   20 ++++++++++++++++++++
 include/linux/blk-mq.h |    1 +
 2 files changed, 21 insertions(+)
block/blk-mq.c +20 -0
index a976db4..f6e1225 100644
@@ -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
index b509ef5..a09728e 100644
@@ -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
#6 lore raw

PATCH v2 5/10 blk-mq: Allow requests to never expire

In reply to 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.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c         |    8 +++++---
 include/linux/blkdev.h |    1 +
 2 files changed, 6 insertions(+), 3 deletions(-)
block/blk-mq.c +5 -3
index f6e1225..5dbd315b 100644
@@ -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
index 92f4b4b..096b4f7f 100644
@@ -198,6 +198,7 @@ struct request {
 
 	unsigned long deadline;
 	struct list_head timeout_list;
+#define REQ_NO_TIMEOUT UINT_MAX
 	unsigned int timeout;
 	int retries;
 
Signature · 1 line
1.7.10.4
#7 lore raw

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
#8 lore raw

PATCH v2 6/10 blk-mq: End unstarted requests on a dying queue

In reply to Keith Busch

Requests that haven't been started prior to a queue dying can be ended in error without having to wait for them to time out.

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 block/blk-mq.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
block/blk-mq.c +6 -1
index 5dbd315b..1443c77 100644
@@ -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
#9 lore raw

PATCH v2 7/10 NVMe: Start driver allocated requests

In reply to Keith Busch

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

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/block/nvme-core.c +12 -4
index f7d083b..ff3012b 100644
@@ -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
#10 lore raw

PATCH v2 8/10 NVMe: Start and stop h/w queues on reset

In reply to Keith Busch

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

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)
drivers/block/nvme-core.c +39 -3
index ff3012b..571577c 100644
@@ -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
#11 lore raw

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;
}

#12 lore raw

PATCH v2 9/10 NVMe: Admin queue error handling

In reply to Keith Busch

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

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   67 +++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)
drivers/block/nvme-core.c +35 -32
index 571577c..f20e6c6 100644
@@ -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
#13 lore raw

PATCH v2 10/10 NVMe: Command abort handling fixes

In reply to Keith Busch

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

Signed-off-by Keith Busch <keith.busch at intel.com>
---
 drivers/block/nvme-core.c |   17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/block/nvme-core.c +13 -4
index f20e6c6..a7f663c 100644
@@ -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
#14 lore raw

v2 cover Second attempt at blk-mq + nvme hotplug fixes

In reply to Keith Busch
Quoted 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_reinit WARN_ON_ONCE when the driver restarts a request_queue with 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=foobar

foobar: (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

#15 lore raw

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=foobar

foobar: (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?

#16 lore raw

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=foobar

foobar: (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.

Quoted 7 lines

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?

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?

#17 lore raw

v2 cover Second attempt at blk-mq + nvme hotplug fixes

In reply to Keith Busch
Quoted 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_reinit WARN_ON_ONCE when the driver restarts a request_queue with 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
#18 lore raw

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_reinit WARN_ON_ONCE when the driver restarts a request_queue with 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.

#19 lore raw

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_reinit WARN_ON_ONCE when the driver restarts a request_queue with 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