nvme timeout fixes, v3

This patch fixes races that occur with simultaneous controller resets

by synchronizing request queues prior to initializing the controller. Without this, a timeout thread may attempt disabling a controller at the same time as we're trying to enable it.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/core.c | 21 +++++++++++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  | 11 +++++++----
 3 files changed, 27 insertions(+), 6 deletions(-)

drivers/nvme/host/core.c +7 -1

index dc8aa2c1c22a..33034e469bbc 100644
@@ -3469,6 +3469,12 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 }
 EXPORT_SYMBOL_GPL(nvme_init_ctrl);
 
+static void nvme_start_queue(struct nvme_ns *ns)
+{
+	blk_mq_unquiesce_queue(ns->queue);
+	blk_mq_kick_requeue_list(ns->queue);
+}
+
 /**
  * nvme_kill_queues(): Ends all namespace queues
  * @ctrl: the dead controller that needs to end

@@ -3497,7 +3503,7 @@ void nvme_kill_queues(struct nvme_ctrl *ctrl)
 		blk_set_queue_dying(ns->queue);
 
 		/* Forcibly unquiesce queues to avoid blocking dispatch */
-		blk_mq_unquiesce_queue(ns->queue);
+		nvme_start_queue(ns);
 	}
 	up_read(&ctrl->namespaces_rwsem);
 }

drivers/nvme/host/core.c +11 -0

 }
 EXPORT_SYMBOL_GPL(nvme_start_queues);
 
+void nvme_sync_queues(struct nvme_ctrl *ctrl)
+{
+	struct nvme_ns *ns;
+
+	down_read(&ctrl->namespaces_rwsem);
+	list_for_each_entry(ns, &ctrl->namespaces, list)
+		blk_sync_queue(ns->queue);
+	up_read(&ctrl->namespaces_rwsem);
+}
+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)

drivers/nvme/host/nvme.h +1 -0

index ec6e4acc4d48..4f43918cd902 100644
@@ -403,6 +403,7 @@ 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,
 		volatile union nvme_result *res);
 
+void nvme_sync_queues(struct nvme_ctrl *ctrl);
 void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl);

drivers/nvme/host/pci.c +0 -2

index 917e1714f7d9..9da28e10d942 100644
@@ -2317,11 +2317,14 @@ static void nvme_reset_work(struct work_struct *work)
 		goto out;
 
 	/*
-	 * If we're called to reset a live controller first shut it down before
-	 * moving on.

Quoted 2 lines · Keith Busch

This patch fixes races that occur with simultaneous controller resets

Wait.. How do we end up with simultaneous controller resets? We not only have the NVME_CTRL_RESETTING resetting state, but also execute all resets from ctrl->reset_work, so they are implicitly single threaded.

Quoted 8 lines · Christoph Hellwig

Quoted 2 lines · Keith Busch

This patch fixes races that occur with simultaneous controller resets

Wait.. How do we end up with simultaneous controller resets? We not only have the NVME_CTRL_RESETTING resetting state, but also execute all resets from ctrl->reset_work, so they are implicitly single threaded.

Right, the bring up is single threaded, but the NVMe Controller Level Reset (CC.EN 1 -> 0) can happen through a timeout. This patch is really just working with the way blk-mq's timeout handler claims requests and prevents the driver from completing them. The reset_work operates under the assumption that there are no outstanding commands after nvme_dev_disable, so this patch just ensures that's the case.

Ok, so we are talking about simultaneous nvme_dev_disable calls, which makes more sense.

That being said I really like the idea that Jianchao floated about always returning BLK_EH_RESET_TIMER and just letting the reset work do the work. I hope it actually works and doesn't have hidden pitfalls..

Quoted 8 lines · Keith Busch

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Keith Busch

This patch fixes races that occur with simultaneous controller resets

Wait.. How do we end up with simultaneous controller resets? We not only have the NVME_CTRL_RESETTING resetting state, but also execute all resets from ctrl->reset_work, so they are implicitly single threaded.

Quoted 6 lines

Right, the bring up is single threaded, but the NVMe Controller Level Reset (CC.EN 1 -> 0) can happen through a timeout. This patch is really just working with the way blk-mq's timeout handler claims requests and prevents the driver from completing them. The reset_work operates under the assumption that there are no outstanding commands after nvme_dev_disable, so this patch just ensures that's the case.

Ok, so we are talking about simultaneous nvme_dev_disable calls, which makes more sense.

That being said I really like the idea that Jianchao floated about always returning BLK_EH_RESET_TIMER and just letting the reset work do the work. I hope it actually works and doesn't have hidden pitfalls..

Quoted 18 lines · Christoph Hellwig

Quoted 8 lines · Keith Busch

Quoted 7 lines · Christoph Hellwig

Quoted 2 lines · Keith Busch

This patch fixes races that occur with simultaneous controller resets

Wait.. How do we end up with simultaneous controller resets? We not only have the NVME_CTRL_RESETTING resetting state, but also execute all resets from ctrl->reset_work, so they are implicitly single threaded.

Quoted 6 lines

Right, the bring up is single threaded, but the NVMe Controller Level Reset (CC.EN 1 -> 0) can happen through a timeout. This patch is really just working with the way blk-mq's timeout handler claims requests and prevents the driver from completing them. The reset_work operates under the assumption that there are no outstanding commands after nvme_dev_disable, so this patch just ensures that's the case.

Ok, so we are talking about simultaneous nvme_dev_disable calls, which makes more sense.

Quoted 5 lines

That being said I really like the idea that Jianchao floated about always returning BLK_EH_RESET_TIMER and just letting the reset work do the work. I hope it actually works and doesn't have hidden pitfalls..

I came to this same conclusion and this is how FC works.

At least in the current blk-mq timeout handling, returning RESET_TIMER presents other challenges for the reset handler: the timer may have reclaimed the request that reset_work is trying to complete.

Quoted 4 lines · Keith Busch

At least in the current blk-mq timeout handling, returning RESET_TIMER presents other challenges for the reset handler: the timer may have reclaimed the request that reset_work is trying to complete.

why would that be true if BLK_EH_RESET_TIMER was returned ?

Quoted 5 lines

Quoted 4 lines

At least in the current blk-mq timeout handling, returning RESET_TIMER presents other challenges for the reset handler: the timer may have reclaimed the request that reset_work is trying to complete.

Quoted 1 line

why would that be true if BLK_EH_RESET_TIMER was returned ?

That return rearms the timer for triggering timeout handling, and when that timeout handling triggers, your reset work won't be able to complete the request. You're basically relying on the timer being high enough that your reset work beats the timer to the completion. That may be sufficient in practice most of the time.

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

At least in the current blk-mq timeout handling, returning RESET_TIMER presents other challenges for the reset handler: the timer may have reclaimed the request that reset_work is trying to complete.

why would that be true if BLK_EH_RESET_TIMER was returned ?

Quoted 5 lines

That return rearms the timer for triggering timeout handling, and when that timeout handling triggers, your reset work won't be able to complete the request. You're basically relying on the timer being high enough that your reset work beats the timer to the completion. That may be sufficient in practice most of the time.

Why? the timeout handler would reset the timer yet again.

Quoted 10 lines

Quoted 7 lines

Right, the bring up is single threaded, but the NVMe Controller Level Reset (CC.EN 1 -> 0) can happen through a timeout. This patch is really just working with the way blk-mq's timeout handler claims requests and prevents the driver from completing them. The reset_work operates under the assumption that there are no outstanding commands after nvme_dev_disable, so this patch just ensures that's the case.

Ok, so we are talking about simultaneous nvme_dev_disable calls, which makes more sense.

Quoted 5 lines

That being said I really like the idea that Jianchao floated about always returning BLK_EH_RESET_TIMER and just letting the reset work do the work.? I hope it actually works and doesn't have hidden pitfalls..

I came to this same conclusion and this is how FC works.

IO may be retryable, so don't wait for them in the reset path.

These
commands may trigger a reset if that IO expires without a completion,

placing it on the requeue list, so waiting for these would deadlock the reset handler.

To fix the theoretical deadlock, this patch unblocks IO submission from

the reset_work as before, but moves the waiting to the scan_work, where waiting for IO is safe so that reset_work may proceed to completion. Since unfreezing the queues now happens in the controller LIVE state, nvme_dev now tracks if the queues were frozen now to prevent incorrect freeze depths.

This patch is also renaming the function 'nvme_dev_add' to a more appropriate name that describes what it's actually doing:
nvme_alloc_io_tags.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/core.c |  2 ++
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/pci.c  | 46 +++++++++++++++++++++++++++++++---------------
 3 files changed, 34 insertions(+), 15 deletions(-)

drivers/nvme/host/core.c +0 -0

index 33034e469bbc..0f0eb85c64b8 100644

Quoted 1 line · Keith Busch

IO may be retryable, so don't wait for them in the reset path.

Can't parse this.

Quoted 2 lines

These
commands may trigger a reset if that IO expires without a completion,

What are "these commands"?

Quoted 4 lines

placing it on the requeue list, so waiting for these would deadlock the reset handler.

To fix the theoretical deadlock, this patch unblocks IO submission from

How did you find it if it is theoretical?

Quoted 3 lines

This patch is also renaming the function 'nvme_dev_add' to a more appropriate name that describes what it's actually doing:
nvme_alloc_io_tags.

Can you split this out into a separate patch?

drivers/nvme/host/core.c static void nvme_scan_work(struct work_struct *work) · line 3179

Quoted 6 lines

drivers/nvme/host/core.c +2 -0

@@ -3175,6 +3175,8 @@ static void nvme_scan_work(struct work_struct *work)
 	struct nvme_id_ctrl *id;
 	unsigned nn;
 
+	if (ctrl->ops->update_hw_ctx)
+		ctrl->ops->update_hw_ctx(ctrl);

nvme_scan_work gets kicked from all kinds of places including ioctls and AERs. I don't think the code you added below should be called from all of them.

drivers/nvme/host/pci.c static void nvme_disable_io_queues(struct nvme_dev *dev) · line 2086

Quoted 8 lines

drivers/nvme/host/pci.c +8 -0

+static void nvme_pci_update_hw_ctx(struct nvme_ctrl *ctrl)
+{
+	struct nvme_dev *dev = to_nvme_dev(ctrl);
+	bool unfreeze;
+
+	mutex_lock(&dev->shutdown_lock);
+	unfreeze = dev->queues_froze;
+	mutex_unlock(&dev->shutdown_lock);

No need to take a mutex here is you sample as single <= register sized value.

drivers/nvme/host/pci.c static void nvme_disable_io_queues(struct nvme_dev *dev) · line 2089

Quoted 2 lines

drivers/nvme/host/pci.c +2 -0

+	if (!unfreeze)
+		return;

But this whole scheme stinks to me. For one we are adding more ad-hoc state outside the state machine, second it all seems very "ad-hoc".

drivers/nvme/host/pci.c static void nvme_disable_io_queues(struct nvme_dev *dev) · line 2098

Quoted 9 lines

drivers/nvme/host/pci.c +9 -0

+
+	nvme_wait_freeze(&dev->ctrl);
+	blk_mq_update_nr_hw_queues(ctrl->tagset, dev->online_queues - 1);
+	nvme_free_queues(dev, dev->online_queues);
+	nvme_unfreeze(&dev->ctrl);
+
+	mutex_lock(&dev->shutdown_lock);
+	dev->queues_froze = false;
+	mutex_unlock(&dev->shutdown_lock);

Same here. Simple READ_ONCE/WRITE_ONCE will give you the right memory barriers with no need for the lock.

Also except for the nvme_free_queues this all is generic code, so I think we want this in the core.

And I wonder where this would fit better than the scan work, but I can't think of anything else but an entirely new work_struct, which isn't all that great either.

Quoted 4 lines

drivers/nvme/host/pci.c +0 -0

@@ -2211,7 +2228,10 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	     dev->ctrl.state == NVME_CTRL_RESETTING)) {
 		u32 csts = readl(dev->bar + NVME_REG_CSTS);
 

drivers/nvme/host/pci.c static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown) · line 2234

Quoted 5 lines

drivers/nvme/host/pci.c +4 -1

-		nvme_start_freeze(&dev->ctrl);
+		if (!dev->queues_froze)	{
+			nvme_start_freeze(&dev->ctrl);
+			dev->queues_froze = true;
+		}

And this sounds like another indicator for a new FROZEN state. Once the ctrl already is frozen we really shouldn't even end up in here anymore.

The block layer's timeout handling currently prevents drivers from completing commands outside the timeout callback once blk-mq decides they've expired. If a device breaks, this could potentially create many thousands of timed out commands. There's nothing of value to be gleaned from observing each of those messages, so this patch adds a rate limit on them.

The block layer's timeout handling currently prevents drivers from completing commands outside the timeout callback once blk-mq decides they've expired. If a device breaks, this could potentially create many thousands of timed out commands. There's nothing of value to be gleaned from observing each of those messages, so this patch adds a rate limit on them.

Signed-off-by Keith Busch <keith.busch at intel.com>

IO is always quiesced in the CONNECTING state, so any any timeout for an IO command had already been completed.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/nvme/host/pci.c +0 -0

index 6be88f662e7d..54e22b964385 100644

Quoted 18 lines · Keith Busch

IO is always quiesced in the CONNECTING state, so any any timeout for an IO command had already been completed.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/nvme/host/pci.c +2 -0

index 6be88f662e7d..54e22b964385 100644
@@ -1227,6 +1227,14 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
 	 */
 	switch (dev->ctrl.state) {
 	case NVME_CTRL_CONNECTING:
+		/*
+		 * IO is never dispatched from the connecting state. If an IO

drivers/nvme/host/pci.c static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved) · line 1236

Quoted 5 lines

drivers/nvme/host/pci.c +5 -0

+		 * queue timed out here, the block layer missed the completion
+		 * the driver already requested, so return handled.
+		 */
+		if (nvmeq->qid)
+			return BLK_EH_HANDLED;

How can we hit this case? This just looks a lot like papering over the real issue..

Quoted 2 lines

How can we hit this case? This just looks a lot like papering over the real issue..

It'll most likley never really happen. The conditions are a pretty obscure timeout handling cases mixed with other errors that could theoretically hit it, requiring two or more namespaces.

The real fix, IMO, is wahat the blk-mq timeout enhancements are working toward, so I've no problem dropping this patch.

This patch ensures the nvme namsepace request queues are not quiesced on a surprise removal. It's possible the queues were previously killed in a failed reset, so the queues need to be unquiesced to ensure all requests are flushed to completion.

Signed-off-by Keith Busch <keith.busch at intel.com>

If the reset failed due to a non-fatal error, this patch will attempt to reset the controller again, with a maximum of 4 attempts.

Quoted 2 lines · Keith Busch

If the reset failed due to a non-fatal error, this patch will attempt to reset the controller again, with a maximum of 4 attempts.

What kind of non-fatal errors do you see that this tries to handle?

If the reset failed due to a non-fatal error, this patch will attempt to reset the controller again, with a maximum of 4 attempts.

Quoted 2 lines

If the reset failed due to a non-fatal error, this patch will attempt to reset the controller again, with a maximum of 4 attempts.

curious, where did 4 came from?

This patch sets the nvmeq's cq_vector only after the cq was successfully created. This way a device reset doesn't mistakenly believe that the vector is allocated. In case a reset does occur during queue creation, this patch will return status immediately instead of trying to unwind the created queues since the device won't be able to delete queues anyway.

This patch will also handle device reported failures correctly. These errors are reported as positive nvme status codes, which were previously not handled at all.

Quoted 6 lines · Keith Busch

This patch sets the nvmeq's cq_vector only after the cq was successfully created. This way a device reset doesn't mistakenly believe that the vector is allocated. In case a reset does occur during queue creation, this patch will return status immediately instead of trying to unwind the created queues since the device won't be able to delete queues anyway.

Quoted 3 lines

This patch will also handle device reported failures correctly. These errors are reported as positive nvme status codes, which were previously not handled at all.

I applied the original patch this morning with some minor fixup. Can you send a relative patch to the nvme-4.18-2 tree?

An nvme controller reset can't depend on the timeout handling to complete timed out commands since we're already trying to disable the controller. The HMB disabling is the only command in this path that was not handling its own timeout, so this patch fixes that by putting a time limit on how long it will wait for completion.

drivers/nvme/host/pci.c +20 -1

+static void nvme_set_host_mem_end_io(struct request *rq, blk_status_t sts)
+{
+	struct completion *wait = rq->end_io_data;
+
+	rq->end_io_data = NULL;
+	blk_mq_free_request(rq);
+	complete(wait);
+}
+
+/*
+ * Use 'wait' when sending this command in a context can't complete blocks the
+ * reset handler, as required for device shutdown.
+ */
+static int nvme_set_host_mem(struct nvme_dev *dev, u32 bits,
+			     struct completion *wait)
 {
 	u64 dma_addr = dev->host_mem_descs_dma;
+	struct request_queue *q = dev->ctrl.admin_q;
+	struct request *req;
 	struct nvme_command c;
 	int ret;

@@ -1780,7 +1796,19 @@ static int nvme_set_host_mem(struct nvme_dev *dev, u32 bits)
 	c.features.dword14	= cpu_to_le32(upper_32_bits(dma_addr));
 	c.features.dword15	= cpu_to_le32(dev->nr_host_mem_descs);
 
-	ret = nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0);
+	if (!wait) {
+		ret = nvme_submit_sync_cmd(q, &c, NULL, 0);
+	} else {

None of this is intimately related to the HMB code. If we really have to we could handle this either in __nvme_submit_sync_cmd or all the way down in blk_execute_rq,

but why doesn't the block layer timeout
code kill the command once we've reached the timeout?

Also if we really need to reset the controller submitting any command just doesn't seem very helpful. We might as well just skip trying to disable the HMB, as the controller needs to come up in a clean state anyway.

Quoted 4 lines

Also if we really need to reset the controller submitting any command just doesn't seem very helpful. We might as well just skip trying to disable the HMB, as the controller needs to come up in a clean state anyway.

That's also fine with me.

Does it mean just need to disable the controller with transiting CC.EN ?1? to ?0? without sending any admin commands including deleting cq/sq ?
The specification indeed says that
when Controller Reset (CC.EN transitions from ?1? to ?0
- The controller stops processing any outstanding Admin or I/O commands.
- All I/O Submission Queues are deleted.
- All I/O Completion Queues are deleted.
- The controller is brought to an Idle state. When this is complete, CSTS.RDY is cleared to ?0?.
- The Admin Queue registers (AQA, ASQ, or ACQ) are not reset as part of a controller reset. All
other controller registers defined in section 3 and internal controller state are reset.

Then for the timeout case at least, for LIVE state, nvme_timeout could hand over all the things to reset_work and return BLK_EH_RESET_TIMER, when reset_work is ongoing, just disable the controller with setting CC.EN and disable the related pci things and return BLK_EH_HANDLED to complete the admin commands to wake up the reset_work. (certainly with your patch to move update_hw_nr to scan_work) nvme_timeout doesn't need to invoke nvme_dev_disable any more and life will be simpler :).

Quoted 4 lines

Also if we really need to reset the controller submitting any command just doesn't seem very helpful. We might as well just skip trying to disable the HMB, as the controller needs to come up in a clean state anyway.

That's also fine with me.

v2 -> v3:

The main difference is getting rid of the bad idea that we could do a CONNECTING -> RESETTING transtion that breaks the state machine, so that patch is dropped. The series maintains the concept of retrying a reset when an init admin command times out. It just does so in the same context instead of requeueing itself.

This series also includes two additional patches to handle other unlikely errors on queue initialization and HMB teardown.

Keith Busch (9):
nvme: Sync request queues on reset
nvme-pci: Fix queue freeze criteria on reset
nvme: Move all IO out of controller reset
nvme-pci: Rate limit the nvme timeout warnings
nvme-pci: End IO requests immediately in CONNECTING state
nvme-pci: Unquiesce dead controller queues
nvme-pci: Attempt reset retry for IO failures
nvme-pci: Queue creation error handling
nvme-pci: Don't wait for HMB completion on shutdown

 drivers/nvme/host/core.c |  23 ++++++-
 drivers/nvme/host/nvme.h |   2 +
 drivers/nvme/host/pci.c  | 173 ++++++++++++++++++++++++++++++++++-------------
 3 files changed, 150 insertions(+), 48 deletions(-)

Quoted 11 lines · Keith Busch

Keith Busch (9):
nvme: Sync request queues on reset
nvme-pci: Fix queue freeze criteria on reset
nvme: Move all IO out of controller reset
nvme-pci: Rate limit the nvme timeout warnings
nvme-pci: End IO requests immediately in CONNECTING state
nvme-pci: Unquiesce dead controller queues
nvme-pci: Attempt reset retry for IO failures
nvme-pci: Queue creation error handling
nvme-pci: Don't wait for HMB completion on shutdown

Quoted 4 lines

 drivers/nvme/host/core.c |  23 ++++++-
 drivers/nvme/host/nvme.h |   2 +
 drivers/nvme/host/pci.c  | 173 ++++++++++++++++++++++++++++++++++-------------
 3 files changed, 150 insertions(+), 48 deletions(-)

Hello Keith,

I guess there will be V4 for addressing the reported issue?