[PATCH 1/5] nvme: Request cancelling helpers

[PATCH 1/5] nvme: Request cancelling helpers

[Keith Busch]

This patch provides an API for cancelling IO requests, replacing each
driver's use of blk_mq_busy_tag_iter with a more convenient API for nvme
controllers.

The nvme_cancel_request is used only in the core now, so this patch
makes that function private.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c   | 31 +++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h   |  8 +++++++-
 drivers/nvme/host/pci.c    |  4 +---
 drivers/nvme/host/rdma.c   | 12 ++++--------
 drivers/nvme/target/loop.c |  6 ++----
 5 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fde6fd2e7eef..b9cf2bce2132 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -233,7 +233,7 @@ void nvme_complete_rq(struct request *req)
 }
 EXPORT_SYMBOL_GPL(nvme_complete_rq);
 
-void nvme_cancel_request(struct request *req, void *data, bool reserved)
+static void nvme_cancel_request(struct request *req, void *data, bool reserved)
 {
 	if (!blk_mq_request_started(req))
 		return;
@@ -245,7 +245,34 @@ void nvme_cancel_request(struct request *req, void *data, bool reserved)
 	blk_mq_complete_request(req);
 
 }
-EXPORT_SYMBOL_GPL(nvme_cancel_request);
+
+void nvme_set_iter(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set,
+		   busy_tag_iter_fn *fn)
+{
+	if (!set)
+		return;
+	blk_mq_tagset_busy_iter(set, fn, ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_set_iter);
+
+void nvme_cancel_io_requests(struct nvme_ctrl *ctrl)
+{
+	nvme_set_iter(ctrl, ctrl->tagset, nvme_cancel_request);
+}
+EXPORT_SYMBOL_GPL(nvme_cancel_io_requests);
+
+void nvme_cancel_admin_requests(struct nvme_ctrl *ctrl)
+{
+	nvme_set_iter(ctrl, ctrl->admin_tagset, nvme_cancel_request);
+}
+EXPORT_SYMBOL_GPL(nvme_cancel_admin_requests);
+
+void nvme_cancel_requests(struct nvme_ctrl *ctrl)
+{
+	nvme_cancel_io_requests(ctrl);
+	nvme_cancel_admin_requests(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_cancel_requests);
 
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8e7fc1b041b7..5fb9d600f9c0 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -349,7 +349,6 @@ static inline void nvme_put_ctrl(struct nvme_ctrl *ctrl)
 }
 
 void nvme_complete_rq(struct request *req);
-void nvme_cancel_request(struct request *req, void *data, bool reserved);
 bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 		enum nvme_ctrl_state new_state);
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
@@ -372,6 +371,13 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res);
 
+void nvme_cancel_requests(struct nvme_ctrl *ctrl);
+void nvme_cancel_io_requests(struct nvme_ctrl *ctrl);
+void nvme_cancel_admin_requests(struct nvme_ctrl *ctrl);
+
+void nvme_set_iter(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set,
+		   busy_tag_iter_fn *fn);
+
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a2ffb557b616..4d2477c3c86c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2213,9 +2213,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	}
 	nvme_pci_disable(dev);
 
-	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
-	blk_mq_tagset_busy_iter(&dev->admin_tagset, nvme_cancel_request, &dev->ctrl);
-
+	nvme_cancel_requests(&dev->ctrl);
 	/*
 	 * The driver will not be starting up queues again if shutting down so
 	 * must flush all entered requests to their failed completion to avoid
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 38e183461d9d..71070eedb773 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -957,14 +957,12 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 
 	if (ctrl->ctrl.queue_count > 1) {
 		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-					nvme_cancel_request, &ctrl->ctrl);
+		nvme_cancel_io_requests(&ctrl->ctrl);
 		nvme_rdma_destroy_io_queues(ctrl, false);
 	}
 
 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
-	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
-				nvme_cancel_request, &ctrl->ctrl);
+	nvme_cancel_admin_requests(&ctrl->ctrl);
 	nvme_rdma_destroy_admin_queue(ctrl, false);
 
 	/*
@@ -1721,8 +1719,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 
 	if (ctrl->ctrl.queue_count > 1) {
 		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-					nvme_cancel_request, &ctrl->ctrl);
+		nvme_cancel_io_requests(&ctrl->ctrl);
 		nvme_rdma_destroy_io_queues(ctrl, shutdown);
 	}
 
@@ -1732,8 +1729,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 		nvme_disable_ctrl(&ctrl->ctrl, ctrl->ctrl.cap);
 
 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
-	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
-				nvme_cancel_request, &ctrl->ctrl);
+	nvme_cancel_admin_requests(&ctrl->ctrl);
 	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
 	nvme_rdma_destroy_admin_queue(ctrl, shutdown);
 }
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 7991ec3a17db..5dd7834a35da 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -439,8 +439,7 @@ static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 {
 	if (ctrl->ctrl.queue_count > 1) {
 		nvme_stop_queues(&ctrl->ctrl);
-		blk_mq_tagset_busy_iter(&ctrl->tag_set,
-					nvme_cancel_request, &ctrl->ctrl);
+		nvme_cancel_io_requests(&ctrl->ctrl);
 		nvme_loop_destroy_io_queues(ctrl);
 	}
 
@@ -448,8 +447,7 @@ static void nvme_loop_shutdown_ctrl(struct nvme_loop_ctrl *ctrl)
 		nvme_shutdown_ctrl(&ctrl->ctrl);
 
 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
-	blk_mq_tagset_busy_iter(&ctrl->admin_tag_set,
-				nvme_cancel_request, &ctrl->ctrl);
+	nvme_cancel_admin_requests(&ctrl->ctrl);
 	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
 	nvme_loop_destroy_admin_queue(ctrl);
 }
-- 
2.14.3

[PATCH 2/5] nvme: Ending failed unstarted requests

[Keith Busch]

This patch provides new nvme driver APIs for directly ending unstarted
requests when they need to be failed. Previously, drivers needed to
temporarily quiesce request queues while setting up the IO path to take on
the responsibilty of error handling, then restart those queues. Handling
these errors should be done directly in the error handling path, freeing
up the IO path to not concern itself with such failure cases.

Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 44 +++++++++++++++++++++++++++++++++++---------
 drivers/nvme/host/nvme.h |  4 ++++
 drivers/nvme/host/pci.c  |  4 ++--
 drivers/nvme/host/rdma.c |  8 +++-----
 4 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b9cf2bce2132..ae4349a4f3a8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -246,6 +246,17 @@ static void nvme_cancel_request(struct request *req, void *data, bool reserved)
 
 }
 
+static void nvme_end_unstarted_request(struct request *req, void *data,
+				       bool reserved)
+{
+	if (blk_mq_request_started(req))
+		return;
+
+	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
+				"Ending I/O %d", req->tag);
+	blk_mq_end_request(req, BLK_STS_IOERR);
+}
+
 void nvme_set_iter(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set,
 		   busy_tag_iter_fn *fn)
 {
@@ -255,6 +266,25 @@ void nvme_set_iter(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set,
 }
 EXPORT_SYMBOL_GPL(nvme_set_iter);
 
+void nvme_end_io_requests(struct nvme_ctrl *ctrl)
+{
+	nvme_set_iter(ctrl, ctrl->tagset, nvme_end_unstarted_request);
+}
+EXPORT_SYMBOL_GPL(nvme_end_io_requests);
+
+void nvme_end_admin_requests(struct nvme_ctrl *ctrl)
+{
+	nvme_set_iter(ctrl, ctrl->admin_tagset, nvme_end_unstarted_request);
+}
+EXPORT_SYMBOL_GPL(nvme_end_admin_requests);
+
+void nvme_end_requests(struct nvme_ctrl *ctrl)
+{
+	nvme_end_io_requests(ctrl);
+	nvme_end_admin_requests(ctrl);
+}
+EXPORT_SYMBOL_GPL(nvme_end_requests);
+
 void nvme_cancel_io_requests(struct nvme_ctrl *ctrl)
 {
 	nvme_set_iter(ctrl, ctrl->tagset, nvme_cancel_request);
@@ -3215,8 +3245,10 @@ void nvme_remove_namespaces(struct nvme_ctrl *ctrl)
 	 * removing the namespaces' disks; fail all the queues now to avoid
 	 * potentially having to clean up the failed sync later.
 	 */
-	if (ctrl->state == NVME_CTRL_DEAD)
+	if (ctrl->state == NVME_CTRL_DEAD) {
 		nvme_kill_queues(ctrl);
+		nvme_end_admin_requests(ctrl);
+	}
 
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list)
 		nvme_ns_remove(ns);
@@ -3467,11 +3499,6 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	mutex_lock(&ctrl->namespaces_mutex);
-
-	/* Forcibly unquiesce queues to avoid blocking dispatch */
-	if (ctrl->admin_q)
-		blk_mq_unquiesce_queue(ctrl->admin_q);
-
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		/*
 		 * Revalidating a dead namespace sets capacity to 0. This will
@@ -3481,11 +3508,10 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 			continue;
 		revalidate_disk(ns->disk);
 		blk_set_queue_dying(ns->queue);
-
-		/* Forcibly unquiesce queues to avoid blocking dispatch */
-		blk_mq_unquiesce_queue(ns->queue);
 	}
 	mutex_unlock(&ctrl->namespaces_mutex);
+
+	nvme_end_io_requests(ctrl);
 }
 EXPORT_SYMBOL_GPL(nvme_kill_queues);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5fb9d600f9c0..96fbc233cbb2 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -371,6 +371,10 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		union nvme_result *res);
 
+void nvme_end_requests(struct nvme_ctrl *ctrl);
+void nvme_end_io_requests(struct nvme_ctrl *ctrl);
+void nvme_end_admin_requests(struct nvme_ctrl *ctrl);
+
 void nvme_cancel_requests(struct nvme_ctrl *ctrl);
 void nvme_cancel_io_requests(struct nvme_ctrl *ctrl);
 void nvme_cancel_admin_requests(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 4d2477c3c86c..63b477bfef37 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2216,11 +2216,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	nvme_cancel_requests(&dev->ctrl);
 	/*
 	 * The driver will not be starting up queues again if shutting down so
-	 * must flush all entered requests to their failed completion to avoid
+	 * must end all entered requests to their failed completion to avoid
 	 * deadlocking blk-mq hot-cpu notifier.
 	 */
 	if (shutdown)
-		nvme_start_queues(&dev->ctrl);
+		nvme_end_requests(&dev->ctrl);
 	mutex_unlock(&dev->shutdown_lock);
 }
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 71070eedb773..e51a84d1d732 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -966,11 +966,9 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 	nvme_rdma_destroy_admin_queue(ctrl, false);
 
 	/*
-	 * queues are not a live anymore, so restart the queues to fail fast
-	 * new IO
+	 * queues are not a live anymore, so end all unstarted requests.
 	 */
-	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
-	nvme_start_queues(&ctrl->ctrl);
+	nvme_end_requests(&ctrl->ctrl);
 
 	nvme_rdma_reconnect_or_remove(ctrl);
 }
@@ -1730,7 +1728,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
 
 	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
 	nvme_cancel_admin_requests(&ctrl->ctrl);
-	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
+	nvme_end_admin_requests(&ctrl->ctrl);
 	nvme_rdma_destroy_admin_queue(ctrl, shutdown);
 }
 
-- 
2.14.3

[PATCH 3/5] nvme/pci: End stopped queue requests directly

[Keith Busch]

For requests that have entered a queue that is not able to be brought
back online, this patch will end those requests directly rather than
rely on the IO path to handle this error.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 63b477bfef37..9e9e3a59bfb3 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2272,6 +2272,19 @@ static void nvme_remove_dead_ctrl(struct nvme_dev *dev, int status)
 		nvme_put_ctrl(&dev->ctrl);
 }
 
+static void nvme_end_stopped_queue_request(struct request *req, void *data,
+				   bool reserved)
+{
+	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+	struct nvme_queue *nvmeq = iod->nvmeq;
+
+	if (nvmeq->cq_vector != -1)
+		return;
+	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
+			    "Ending I/O %d QID:%d", req->tag, nvmeq->qid);
+	blk_mq_end_request(req, BLK_STS_IOERR);
+}
+
 static void nvme_reset_work(struct work_struct *work)
 {
 	struct nvme_dev *dev =
@@ -2344,6 +2357,8 @@ static void nvme_reset_work(struct work_struct *work)
 		nvme_remove_namespaces(&dev->ctrl);
 		new_state = NVME_CTRL_ADMIN_ONLY;
 	} else {
+		nvme_set_iter(&dev->ctrl, dev->ctrl.tagset,
+			      nvme_end_stopped_queue_request);
 		nvme_start_queues(&dev->ctrl);
 		nvme_wait_freeze(&dev->ctrl);
 		/* hit this only when allocate tagset fails */
-- 
2.14.3

[PATCH 4/5] nvme/pci: Remove cq_vector checks in io path

[Keith Busch]

We don't need to check for if the queue is suspended in the submission or
completion path. Requests to suspended queues are no longer submitted, and
we don't unmap the doorbell while requests are active.

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

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 9e9e3a59bfb3..99ac0de43206 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -889,11 +889,6 @@ static blk_status_t nvme_queue_rq(struct blk_mq_hw_ctx *hctx,
 	blk_mq_start_request(req);
 
 	spin_lock_irq(&nvmeq->q_lock);
-	if (unlikely(nvmeq->cq_vector < 0)) {
-		ret = BLK_STS_IOERR;
-		spin_unlock_irq(&nvmeq->q_lock);
-		goto out_cleanup_iod;
-	}
 	__nvme_submit_cmd(nvmeq, &cmnd);
 	nvme_process_cq(nvmeq);
 	spin_unlock_irq(&nvmeq->q_lock);
@@ -924,11 +919,9 @@ static inline void nvme_ring_cq_doorbell(struct nvme_queue *nvmeq)
 {
 	u16 head = nvmeq->cq_head;
 
-	if (likely(nvmeq->cq_vector >= 0)) {
-		if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
-						      nvmeq->dbbuf_cq_ei))
-			writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
-	}
+	if (nvme_dbbuf_update_and_check_event(head, nvmeq->dbbuf_cq_db,
+					      nvmeq->dbbuf_cq_ei))
+		writel(head, nvmeq->q_db + nvmeq->dev->db_stride);
 }
 
 static inline void nvme_handle_cqe(struct nvme_queue *nvmeq,
-- 
2.14.3

[PATCH 5/5] nvme: Sync queues on controller resets

[Keith Busch]

This patch has the nvme pci driver synchronize request queues to ensure
that starting up the controller is not racing with a previously running
timeout handler.

Reported-by: Jianchao Wang <jianchao.w.wang at oracle.com>
Signed-off-by: Keith Busch <keith.busch at intel.com>
---
 drivers/nvme/host/core.c | 15 ++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  |  4 +++-
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ae4349a4f3a8..e8826fab54b8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3578,12 +3578,25 @@ void nvme_start_queues(struct nvme_ctrl *ctrl)
 	struct nvme_ns *ns;
 
 	mutex_lock(&ctrl->namespaces_mutex);
-	list_for_each_entry(ns, &ctrl->namespaces, list)
+	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		blk_mq_unquiesce_queue(ns->queue);
+		blk_mq_kick_requeue_list(ns->queue);
+	}
 	mutex_unlock(&ctrl->namespaces_mutex);
 }
 EXPORT_SYMBOL_GPL(nvme_start_queues);
 
+void nvme_sync_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	mutex_lock(&ctrl->namespaces_mutex);
+	list_for_each_entry(ns, &ctrl->namespaces, list)
+		blk_sync_queue(ns->queue);
+	mutex_unlock(&ctrl->namespaces_mutex);
+}
+EXPORT_SYMBOL_GPL(nvme_sync_queues);
+
 int nvme_reinit_tagset(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set)
 {
 	if (!ctrl->ops->reinit_request)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 96fbc233cbb2..6e80912fe707 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -384,6 +384,7 @@ void nvme_set_iter(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set,
 
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
+void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl);
 void nvme_unfreeze(struct nvme_ctrl *ctrl);
 void nvme_wait_freeze(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 99ac0de43206..2de40d1780c1 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2293,8 +2293,10 @@ static void nvme_reset_work(struct work_struct *work)
 	 * If we're called to reset a live controller first shut it down before
 	 * moving on.
 	 */
-	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE)
+	if (dev->ctrl.ctrl_config & NVME_CC_ENABLE) {
 		nvme_dev_disable(dev, false);
+		nvme_sync_queues(&dev->ctrl);
+	}
 
 	result = nvme_pci_enable(dev);
 	if (result)
-- 
2.14.3

[PATCH 2/5] nvme: Ending failed unstarted requests

[jianchao.wang]

Hi Keith


On 01/23/2018 05:56 AM, Keith Busch wrote:
> This patch provides new nvme driver APIs for directly ending unstarted
> requests when they need to be failed. Previously, drivers needed to
> temporarily quiesce request queues while setting up the IO path to take on
> the responsibilty of error handling, then restart those queues. Handling
> these errors should be done directly in the error handling path, freeing
> up the IO path to not concern itself with such failure cases.

Yes, we indeed need to ensure the entered requests to be drained after nvme_dev_disable
returns for shutdown case.

> Signed-off-by: Keith Busch <keith.busch at intel.com>
> ---
>  drivers/nvme/host/core.c | 44 +++++++++++++++++++++++++++++++++++---------
>  drivers/nvme/host/nvme.h |  4 ++++
>  drivers/nvme/host/pci.c  |  4 ++--
>  drivers/nvme/host/rdma.c |  8 +++-----
>  4 files changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index b9cf2bce2132..ae4349a4f3a8 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -246,6 +246,17 @@ static void nvme_cancel_request(struct request *req, void *data, bool reserved)
>  
>  }
>  
> +static void nvme_end_unstarted_request(struct request *req, void *data,
> +				       bool reserved)
> +{
> +	if (blk_mq_request_started(req))
> +		return;
> +
> +	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
> +				"Ending I/O %d", req->tag);
> +	blk_mq_end_request(req, BLK_STS_IOERR);
> +}
> +

This unstarted requests have not been dequeued from the blk-mq queue.
We cannot end them directly.

Thanks
Jianchao

[PATCH 2/5] nvme: Ending failed unstarted requests

[jianchao.wang]

Hi Keith

On 01/23/2018 05:56 AM, Keith Busch wrote:
>  	nvme_cancel_requests(&dev->ctrl);
>  	/*
>  	 * The driver will not be starting up queues again if shutting down so
> -	 * must flush all entered requests to their failed completion to avoid
> +	 * must end all entered requests to their failed completion to avoid
>  	 * deadlocking blk-mq hot-cpu notifier.
>  	 */
>  	if (shutdown)
> -		nvme_start_queues(&dev->ctrl);
> +		nvme_end_requests(&dev->ctrl);
The blk-mq cpuhp notifier has been changed by Christoph's 4b855ad (blk-mq: Create hctx for each present CPU),
the deadlocking here should have been fixed here.
So the comment here should be modified at least to avoid confusion.
On the other hand, we needn't freeze the queue for reset case, the device will be setup soon later, just 
quiesce the blk-mq queue should be ok. And regarding to the shutdown case, frozen and drained queue will
be safer. To drain the queue, we have to unquiesce it.
Finally, we get a quiesced queue for reset case, and frozen and drained queue for shutdown case.

Thanks
Jianchao

[PATCH 1/5] nvme: Request cancelling helpers

[Sagi Grimberg]

> +void nvme_set_iter(struct nvme_ctrl *ctrl, struct blk_mq_tag_set *set,
> +		   busy_tag_iter_fn *fn)

Nit: maybe nvme_tagset_iter?


Other than that looks good!

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 2/5] nvme: Ending failed unstarted requests

[Sagi Grimberg]

>> +static void nvme_end_unstarted_request(struct request *req, void *data,
>> +				       bool reserved)
>> +{
>> +	if (blk_mq_request_started(req))
>> +		return;
>> +
>> +	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
>> +				"Ending I/O %d", req->tag);
>> +	blk_mq_end_request(req, BLK_STS_IOERR);
>> +}
>> +
> 
> This unstarted requests have not been dequeued from the blk-mq queue.
> We cannot end them directly.

I think we cannot complete them. I think its fine to end them.

[PATCH 2/5] nvme: Ending failed unstarted requests

[Sagi Grimberg]

> Hi Keith
> 
> On 01/23/2018 05:56 AM, Keith Busch wrote:
>>   	nvme_cancel_requests(&dev->ctrl);
>>   	/*
>>   	 * The driver will not be starting up queues again if shutting down so
>> -	 * must flush all entered requests to their failed completion to avoid
>> +	 * must end all entered requests to their failed completion to avoid
>>   	 * deadlocking blk-mq hot-cpu notifier.
>>   	 */
>>   	if (shutdown)
>> -		nvme_start_queues(&dev->ctrl);
>> +		nvme_end_requests(&dev->ctrl);
> The blk-mq cpuhp notifier has been changed by Christoph's 4b855ad (blk-mq: Create hctx for each present CPU),
> the deadlocking here should have been fixed here.
> So the comment here should be modified at least to avoid confusion.
> On the other hand, we needn't freeze the queue for reset case, the device will be setup soon later, just
> quiesce the blk-mq queue should be ok. And regarding to the shutdown case, frozen and drained queue will
> be safer. To drain the queue, we have to unquiesce it.

I think that this is exactly the point of the patch, not to unquiesce
and rely on .queue_rq to fail the requests.

[PATCH 2/5] nvme: Ending failed unstarted requests

[jianchao.wang]

Hi Sagi


On 01/23/2018 09:02 PM, Sagi Grimberg wrote:
> 
>>> +static void nvme_end_unstarted_request(struct request *req, void *data,
>>> +?????????????????????? bool reserved)
>>> +{
>>> +??? if (blk_mq_request_started(req))
>>> +??????? return;
>>> +
>>> +??? dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
>>> +??????????????? "Ending I/O %d", req->tag);
>>> +??? blk_mq_end_request(req, BLK_STS_IOERR);
>>> +}
>>> +
>>
>> This unstarted requests have not been dequeued from the blk-mq queue.
>> We cannot end them directly.
> 
> I think we cannot complete them. I think its fine to end them.
> 
The end here which is done by blk_mq_end_request is to free it,

blk_mq_end_request()
  -> __blk_mq_end_request()
    -> blk_mq_free_request()

The requests are still on the blk-mq queues.
How can we free them ?

Thanks
Jianchao

[PATCH 2/5] nvme: Ending failed unstarted requests

[Sagi Grimberg]

> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 71070eedb773..e51a84d1d732 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -966,11 +966,9 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
>   	nvme_rdma_destroy_admin_queue(ctrl, false);
>   
>   	/*
> -	 * queues are not a live anymore, so restart the queues to fail fast
> -	 * new IO
> +	 * queues are not a live anymore, so end all unstarted requests.
>   	 */
> -	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
> -	nvme_start_queues(&ctrl->ctrl);
> +	nvme_end_requests(&ctrl->ctrl);
>   
>   	nvme_rdma_reconnect_or_remove(ctrl);
>   }
> @@ -1730,7 +1728,7 @@ static void nvme_rdma_shutdown_ctrl(struct nvme_rdma_ctrl *ctrl, bool shutdown)
>   
>   	blk_mq_quiesce_queue(ctrl->ctrl.admin_q);
>   	nvme_cancel_admin_requests(&ctrl->ctrl);
> -	blk_mq_unquiesce_queue(ctrl->ctrl.admin_q);
> +	nvme_end_admin_requests(&ctrl->ctrl);
>   	nvme_rdma_destroy_admin_queue(ctrl, shutdown);
>   }

I think we need to end_io_requests a little above as well.

[PATCH 3/5] nvme/pci: End stopped queue requests directly

[Sagi Grimberg]

> +static void nvme_end_stopped_queue_request(struct request *req, void *data,
> +				   bool reserved)

nvme_pci_end_stopped_queue_request

> +{
> +	struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
> +	struct nvme_queue *nvmeq = iod->nvmeq;
> +
> +	if (nvmeq->cq_vector != -1)
> +		return;
> +	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
> +			    "Ending I/O %d QID:%d", req->tag, nvmeq->qid);
> +	blk_mq_end_request(req, BLK_STS_IOERR);
> +}
> +
>   static void nvme_reset_work(struct work_struct *work)
>   {
>   	struct nvme_dev *dev =
> @@ -2344,6 +2357,8 @@ static void nvme_reset_work(struct work_struct *work)
>   		nvme_remove_namespaces(&dev->ctrl);
>   		new_state = NVME_CTRL_ADMIN_ONLY;
>   	} else {
> +		nvme_set_iter(&dev->ctrl, dev->ctrl.tagset,
> +			      nvme_end_stopped_queue_request);

It would have been good if we could reuse that in other transports
where we maintain state flags (NVME_*_Q_LIVE).