NVMe: Shutdown fixes

The controller must be disabled prior to completing a presumed lost command. This patch makes the shutdown safe to simultaneous invocations using mutex, and directly calls shutdown from the timeout handler if timeout occurs.

The driver must directly set req->errors to get the appropriate status propogated to the caller, since the timeout handler already set the targeted request to "complete". The shutdown unconditionally clears all outstanding commands, so it is safe to return "EH_HANDLED" from the timeout handler and assume the command was completed.

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

 	/*
-	 * Just return an error to reset_work if we're already called from
-	 * probe context.  We don't worry about request / tag reuse as
-	 * reset_work will immeditalely unwind and remove the controller
-	 * once we fail a command.
+	 * Shutdown immediately if controller times out while starting. The
+	 * reset work will see the pci device disabled when it gets the forced

Quoted 8 lines

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

+static inline void nvme_end_req(struct request *req, int error)
+{
+	/*
+	 * The request may already be marked "complete" by blk-mq if
+	 * completion occurs during a timeout. We set req->errors
+	 * before telling blk-mq in case this is the target request
+	 * of a timeout handler. This is safe since this driver never
+	 * completes the same command twice.

drivers/nvme/host/pci.c static void nvme_complete_rq(struct request *req) · line 739

Quoted 3 lines

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

+	 */
+	req->errors = error;
+	blk_mq_complete_request(req, req->errors);

I don't think overwriting req->errors on an already completed request is a good idea - it's what we used to earlier, but I specificly changed the blk_mq_complete_request so that we don't do that. Can you explain when you want to overwrite the value exactly and why?

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

Quoted 7 lines

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

 	if (test_bit(NVME_CTRL_RESETTING, &dev->flags)) {
-		req->errors = NVME_SC_ABORT_REQ | NVME_SC_DNR;
+		dev_warn(dev->dev,
+				"I/O %d QID %d timeout, disable controller\n",
+				 req->tag, nvmeq->qid);
+		nvme_dev_shutdown(dev);
 		return BLK_EH_HANDLED;

So here you return BLK_EH_HANDLED without setting req->errors, which sounds like it might be related to the above issue.

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

Quoted 5 lines

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

+		 * Mark the request as handled, since the inline shutdown
+		 * forces all outstanding requests to complete.
 		 */
-		return BLK_EH_QUIESCED;
+		return BLK_EH_HANDLED;

Again we're not setting req->errors here.

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

Quoted 5 lines

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

@@ -2023,6 +2038,7 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
 
 	nvme_dev_list_remove(dev);
 
+	mutex_lock(&dev->shutdown_lock);

Shoudn't we also have a state check that skips all the work here if the controller has already been shut down?

drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 2141

Quoted 5 lines

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

+	 * The pci device will be disabled if a timeout occurs while setting up
+	 * IO queues, but return 0 for "result", so we have to check both.
+	 */
+	if (result || !pci_is_enabled(to_pci_dev(dev->dev)))
 		goto free_tags;

I don't think we should return zero here in that case - that seems to be a bug in setting up req->errrors in the timeout handler.

Quoted 10 lines

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

Quoted 7 lines

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

 	if (test_bit(NVME_CTRL_RESETTING, &dev->flags)) {
-		req->errors = NVME_SC_ABORT_REQ | NVME_SC_DNR;
+		dev_warn(dev->dev,
+				"I/O %d QID %d timeout, disable controller\n",
+				 req->tag, nvmeq->qid);
+		nvme_dev_shutdown(dev);
 		return BLK_EH_HANDLED;

So here you return BLK_EH_HANDLED without setting req->errors, which sounds like it might be related to the above issue.

Right, we don't want to set req->errors here. nvme_dev_shutdown unconditionally reaps every request known to the driver,

so it's safe
assume the timed out request is completed and return EH_HANDLED.

In either case, req->errors is set through the new nvme_end_req, as you surmised. We don't want to set req->errors here directly in case the request completes through process_cq. We should prefer the controller's status rather than make one up.

Yep, same reasoning as above. This also fixes this race:

CPU 0 CPU 1

blk_mq_check_expired nvme_irq

    set_bit(REQ_ATOM_COMPLETE)          nvme_process_cq
    nvme_timeout                          blk_mq_complete_request
                                            if (!test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags) ||
	                                         test_and_clear_bit(REQ_ATOM_QUIESCED, &rq->atomic_flags))
      return BLK_EH_QUIESCED
    set_bit REQ_ATOM_QUIESCED

Both checks on blk_mq_complete_request will fail since the timeout handler hasn't returned "quiesce" yet, and the request will be orphaned.

Instead we can let nvme_dev_shudtown reap the request and set req->errors directly through one of the two paths described above, then return BLK_EH_HANDLED.

Quoted 8 lines

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

Quoted 5 lines

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

@@ -2023,6 +2038,7 @@ static void nvme_dev_shutdown(struct nvme_dev *dev)
 
 	nvme_dev_list_remove(dev);
 
+	mutex_lock(&dev->shutdown_lock);

Shoudn't we also have a state check that skips all the work here if the controller has already been shut down?

This is implied through dev->bar != NULL, and the nvmeq's cq_vector value.

Quoted 8 lines

drivers/nvme/host/pci.c static void nvme_reset_work(struct work_struct *work) · line 2141

Quoted 5 lines

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

+	 * The pci device will be disabled if a timeout occurs while setting up
+	 * IO queues, but return 0 for "result", so we have to check both.
+	 */
+	if (result || !pci_is_enabled(to_pci_dev(dev->dev)))
 		goto free_tags;

I don't think we should return zero here in that case - that seems to be a bug in setting up req->errrors in the timeout handler.

To set an appropriate non-zero result, we'd need to add pci_is_enabled() checks in nvme_setup_io_queues to distinguish a non-fatal command error vs timeout. Is that preferable to the check where I have it?

Quoted 10 lines

CPU 0 CPU 1

blk_mq_check_expired nvme_irq

    set_bit(REQ_ATOM_COMPLETE)          nvme_process_cq
    nvme_timeout                          blk_mq_complete_request
                                            if (!test_and_set_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags) ||
	                                         test_and_clear_bit(REQ_ATOM_QUIESCED, &rq->atomic_flags))
      return BLK_EH_QUIESCED
    set_bit REQ_ATOM_QUIESCED

Quoted 2 lines

Both checks on blk_mq_complete_request will fail since the timeout handler hasn't returned "quiesce" yet, and the request will be orphaned.

BLK_EH_QUIESCED is never returned with your patch applied..

Quoted 3 lines

Instead we can let nvme_dev_shudtown reap the request and set req->errors directly through one of the two paths described above, then return BLK_EH_HANDLED.

nvme_dev_shutdown will not complete the command that we timed out, blk_mq_complete_request will skip it because REQ_ATOM_COMPLETE is set, and blk_mq_rq_timed_out will complete after we returned from the timeout handler.

Quoted 1 line

This is implied through dev->bar != NULL, and the nvmeq's cq_vector value.

Well, we're not doing anything in the low-level functions at the moment (at the checks in nvme_dev_unmap, and all requests with REQ_ATOM_COMPLETE to the list), but it still seems rather fragile to rely on these low level functions deep down the stack to get everything right.

Quoted 3 lines

To set an appropriate non-zero result, we'd need to add pci_is_enabled() checks in nvme_setup_io_queues to distinguish a non-fatal command error vs timeout. Is that preferable to the check where I have it?

No, that's not my preference. My preference is to figure out why you get a zero req->errors on timed request. That really shouldn't happen to start with.

Quoted 4 lines

nvme_dev_shutdown will not complete the command that we timed out, blk_mq_complete_request will skip it because REQ_ATOM_COMPLETE is set, and blk_mq_rq_timed_out will complete after we returned from the timeout handler.

I'm not saying nvme_dev_shutdown "completes" the command. I'm just saying it reaps it and sets an appropriate req->errors. The actual blk-mq completion happens as you described through nvme_timeout's return code of BLK_EH_HANDLED.

Quoted 4 lines

Quoted 3 lines

To set an appropriate non-zero result, we'd need to add pci_is_enabled() checks in nvme_setup_io_queues to distinguish a non-fatal command error vs timeout. Is that preferable to the check where I have it?

Quoted 3 lines

No, that's not my preference. My preference is to figure out why you get a zero req->errors on timed request. That really shouldn't happen to start with.

req->errors wouldn't be 0, but nvme_set_queue_count returns "0" queues if req->errors is non-zero. It's not a fatal error to have 0 queues so we don't want to propogate that error to the initialization unless it's a timeout.

We also don't return error codes on create sq/cq commands because an error here isn't fatal either unless, of course, it's a timeout.

Quoted 4 lines · Keith Busch

I'm not saying nvme_dev_shutdown "completes" the command. I'm just saying it reaps it and sets an appropriate req->errors. The actual blk-mq completion happens as you described through nvme_timeout's return code of BLK_EH_HANDLED.

So let's get rid of the nvme_end_req thing and return the real errors.

Quoted 4 lines

req->errors wouldn't be 0, but nvme_set_queue_count returns "0" queues if req->errors is non-zero. It's not a fatal error to have 0 queues so we don't want to propogate that error to the initialization unless it's a timeout.

Ok, step back here, I think that set_queue_count beast is getting too confusing. What do you 'failing' devices return in the NVMe CQE status and result fields? Do we get a failure in status, or do we get a zero status and a zero result?

Quoted 2 lines

We also don't return error codes on create sq/cq commands because an error here isn't fatal either unless, of course, it's a timeout.

So make sure that we a) don't mix up the status and result return values by making the count parameter of set_queue_count a pointer and thus keeping it separate from the status.

And then explicitly check for a timeout on create sq/cq.

And for
set_queue_count I disagree that it shouldn't be non-fatal, it's the way how the hardware tells you how many queues are supported. If some hardware gets that wrong we'll have to handle it for better or worse, but we should document that it's buggy and probably even log a message.

And a failing create sq/cq actually is fatal for us except for the initial probe as well as blk-mq will keep using it once we set up the queue count.

Quoted 1 line

So let's get rid of the nvme_end_req thing and return the real errors.

nvme_end_req _does_ set the real error.

We can't set it in the timeout handler. How would you even know to set an error?

Maybe the controller posted success status, so we should let nvme_dev_shutdown reap the command and set req->errors.

Quoted 4 lines

Ok, step back here, I think that set_queue_count beast is getting too confusing. What do you 'failing' devices return in the NVMe CQE status and result fields? Do we get a failure in status, or do we get a zero status and a zero result?

Such devices return status 6, internal device error, and the senario requires user management to rectify.

How do you explicitly check for a timeout? We used to have negative error codes for these, but you changed that to fake NVMe status codes.

Quoted 4 lines

set_queue_count I disagree that it shouldn't be non-fatal, it's the way how the hardware tells you how many queues are supported. If some hardware gets that wrong we'll have to handle it for better or worse, but we should document that it's buggy and probably even log a message.

It's not "wrong" to return an appropriate error code to a command if the device is in a state that can't support it. The h/w may be in a non-optimal condition, but we handle that correctly today.

Quoted 6 lines · Keith Busch

Quoted 1 line

So let's get rid of the nvme_end_req thing and return the real errors.

nvme_end_req _does_ set the real error.

We can't set it in the timeout handler. How would you even know to set an error?

Because the command timed out, which means we're done with it. That's the whole point of the timeout - after that the command is fenced off and we are done with it.

Quoted 2 lines

Maybe the controller posted success status, so we should let nvme_dev_shutdown reap the command and set req->errors.

If the controller had posted success status in time we would not have timed out the command.

What you basically seem to care about is handling the race where the controller still completes the command after the timeout handler is running, and you want the controller to win over the timeout handler.

But as I said above the race also works the other way around: the controller completed the command just before we shut down the controller, and now we overwrite req->errors with the wrong value in nvme_cancel_queue_ios.

Quoted 2 lines

Such devices return status 6, internal device error, and the senario requires user management to rectify.

Eww, ok. So what do we want to do with the devices? Just ensure the admin quue is up to be able to send vendor specific admin commands to fix it?

Quoted 2 lines

How do you explicitly check for a timeout? We used to have negative error codes for these, but you changed that to fake NVMe status codes.

The aborted one is pretty obvious for NVMe. Anyway, I'm finally understanding what the magic negative values are for. Without the long back story that you actually want to support broken devices that need to come up without I/O queues and thus we have to ignore the Set Features field there was absolutely no way to figure them out.

I'm perfectly happy to add back a:

(quoted diff) +2 -0

+       /* Linux driver specific value: */
+       NVME_SC_TIMEOUT                 = -1;

and use it specificly for this case. But only as a trade in for documenting in detail why we even bother with all this special casing.

I'll give this a spin the next day. I assume you and Jns will be out for a long weekend starting tomorrow, so I should have a new series ready for review. I'll fold a modified version of your patch in, as I think we should get this right from the start.

Quoted 4 lines · Christoph Hellwig

I'll give this a spin the next day. I assume you and Jns will be out for a long weekend starting tomorrow, so I should have a new series ready for review. I'll fold a modified version of your patch in, as I think we should get this right from the start.

Can you review the code in
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-req.11
and see if this fits your intention?

I've only sent out the hopefully uncontroversial first part for now to limit the amount of patch traffic on the list.

Quoted 3 lines

Can you review the code in
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-req.11
and see if this fits your intention?

Looking good so far, passing all tests.

For something new not related to this work, I was recently copied on an issue and wanted to check with you for your opinion on how to fix.

We probe in a workqueue to improve startup time, but not blocking probe messes up some user space software looking for expected mount points. I was comparing how SCSI works, and it looks like it relies on async_schedule and wait_for_device_probe.

We didn't use async_shedule in nvme because nvme_remove can't synchronize its own work. It doesn't look like much trouble to make "remove" not require flushing startup, so I'm thinking we can just remove all the work queues and just use the async API instead. Does that sound reasonable, or any other ideas?

Quoted 3 lines

Can you review the code in
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-req.11
and see if this fits your intention?

Looking good so far, passing all tests.

Quoted 5 lines · Keith Busch

Quoted 3 lines

Can you review the code in
http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/nvme-req.11
and see if this fits your intention?

Looking good so far, passing all tests.

Can you Ack the remaining patches on the list? I'd like to get them into the safe harbour of Jens' tree before starting on the round of updates.

Looks good, thanks! For the series:

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