nvme of: don't flush scan work inside reset context

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 3475

Quoted 5 lines · Ming Lei

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

-void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl, bool flush_scan)
 {
 	nvme_mpath_stop(ctrl);
 	nvme_stop_keep_alive(ctrl);

Newly added parameter 'flush_scan' is unsused. ?

When scan work is in-progress, any controller error may trigger reset, now fc, rdma and loop host tries to flush scan work inside reset context.

This way can cause deadlock easily because any IO during controler recovery(reset) can't be completed until the recovery is done.

This patch tries to address the deadlock issue by not flushing scan work inside reset context. Actually not see obvious reason to do that:

- once reset is done, a new scan will be scheduled.
- PCI NVMe doesn't do that way

Cc James Smart <james.smart at broadcom.com>

Cc Keith Busch <keith.busch at intel.com>

Cc Christoph Hellwig <hch at lst.de>

Cc Sagi Grimberg <sagi at grimberg.me>

Signed-off-by Ming Lei <ming.lei at redhat.com>

---
  drivers/nvme/host/core.c   | 4 ++--
  drivers/nvme/host/fc.c     | 2 +-
  drivers/nvme/host/nvme.h   | 2 +-
  drivers/nvme/host/pci.c    | 2 +-
  drivers/nvme/host/rdma.c   | 2 +-
  drivers/nvme/target/loop.c | 2 +-
  6 files changed, 7 insertions(+), 7 deletions(-)

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

index 2e65be8b1387..fbbb6bd8fbc5 100644
@@ -161,7 +161,7 @@ static void nvme_delete_ctrl_work(struct work_struct *work)
  		 "Removing ctrl: NQN \"%s\"\n", ctrl->opts->subsysnqn);
 
  	flush_work(&ctrl->reset_work);
-	nvme_stop_ctrl(ctrl);
+	nvme_stop_ctrl(ctrl, true);
  	nvme_remove_namespaces(ctrl);
  	ctrl->ops->delete_ctrl(ctrl);
  	nvme_uninit_ctrl(ctrl);

Quoted 41 lines · Ming Lei

When scan work is in-progress, any controller error may trigger reset, now fc, rdma and loop host tries to flush scan work inside reset context.

This way can cause deadlock easily because any IO during controler recovery(reset) can't be completed until the recovery is done.

This patch tries to address the deadlock issue by not flushing scan work inside reset context. Actually not see obvious reason to do that:

- once reset is done, a new scan will be scheduled.
- PCI NVMe doesn't do that way

Cc James Smart <james.smart at broadcom.com>

Cc Keith Busch <keith.busch at intel.com>

Cc Christoph Hellwig <hch at lst.de>

Cc Sagi Grimberg <sagi at grimberg.me>

Signed-off-by Ming Lei <ming.lei at redhat.com>

---
  drivers/nvme/host/core.c   | 4 ++--
  drivers/nvme/host/fc.c     | 2 +-
  drivers/nvme/host/nvme.h   | 2 +-
  drivers/nvme/host/pci.c    | 2 +-
  drivers/nvme/host/rdma.c   | 2 +-
  drivers/nvme/target/loop.c | 2 +-
  6 files changed, 7 insertions(+), 7 deletions(-)

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

index 2e65be8b1387..fbbb6bd8fbc5 100644
@@ -161,7 +161,7 @@ static void nvme_delete_ctrl_work(struct work_struct *work)
  		 "Removing ctrl: NQN \"%s\"\n", ctrl->opts->subsysnqn);
  	flush_work(&ctrl->reset_work);
-	nvme_stop_ctrl(ctrl);
+	nvme_stop_ctrl(ctrl, true);
  	nvme_remove_namespaces(ctrl);
  	ctrl->ops->delete_ctrl(ctrl);
  	nvme_uninit_ctrl(ctrl);

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 3475

Quoted 8 lines

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

@@ -3469,7 +3469,7 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
  }
  EXPORT_SYMBOL_GPL(nvme_complete_async_event);
-void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
+void nvme_stop_ctrl(struct nvme_ctrl *ctrl, bool flush_scan)
  {
  	nvme_mpath_stop(ctrl);
  	nvme_stop_keep_alive(ctrl);
 

Keith: This was missing a snippet in nvme_stop_ctrl() for

?-????????????? flush_work(&ctrl->scan_work); ?+???????????? if (flush_scan)
?+ ????????????????? flush_work(&ctrl->scan_work);

I also believe Ming's patch isn't enough.? The issue for the transports are several of the flush cases require an io to complete to finish flushing - not just scan_work.?? nvme_mpath_stop() syncs with ctrl->ana_work, which may be waiting for an ana log to complete.? fw_act_queues() may be in the midst of polling the CSTS bit register or doing a fw_slot log read, or a stop_ctrl() routine for the transport may be in the middle of a start_ctrl call.

Give the transport may be at a point where it's lost connectivity to the controller (never really a case for pci unless you consider hot unplug), before calling ctlr stop, the transport has to terminate/return outstanding io and stop further io from being accepted (usually by marking the queue as not connected).

rdma seems to pretty much do the right thing for it's error handling path - where it schedules the err_work handler to do this. But that's missing from the reset controller path (and is nooped if reset is in progress and stuck in one of these flushes when a timeout occurs).

fc resets the device, thus calls the flush routines, when it resets or has an error. So it never gets to call delete_association to teardown/mark things unconnected befpre waiting on the flushes.

I think we need to have the reset_work routines call the routine that deletes/tearsdown before it calls nvme_stop_ctrl().? These routines will likely call nvme_stop_keep_alive right before doing so as well.

Quoted 8 lines

I also believe Ming's patch isn't enough.? The issue for the transports are several of the flush cases require an io to complete to finish flushing - not just scan_work.?? nvme_mpath_stop() syncs with ctrl->ana_work, which may be waiting for an ana log to complete.? fw_act_queues() may be in the midst of polling the CSTS bit register or doing a fw_slot log read, or a stop_ctrl() routine for the transport may be in the middle of a start_ctrl call.

These commands won't be retried since REQ_FAILFAST_DRIVER is set for any request allocated via nvme_alloc_request().

So the above flush cases you mentioned should be easier to handle than IO from scan context. However, looks the patch we discussed before by moving nvme_stop_ctrl() after nvme_rdma_shutdown_ctrl() is still needed for avoiding hang on flush in these commands.

Quoted 6 lines

Give the transport may be at a point where it's lost connectivity to the controller (never really a case for pci unless you consider hot unplug), before calling ctlr stop, the transport has to terminate/return outstanding io and stop further io from being accepted (usually by marking the queue as not connected).

This patch doesn't change this point, all in-flight IO is still terminate/return. With or without this patch, all these IO will be retried too after controller is recovered.

Or you mean all these IOs have to terminate/return before calling ctrl->ops->stop_ctrl().

Quoted 11 lines · Ming Lei

Quoted 8 lines

I also believe Ming's patch isn't enough.? The issue for the transports are several of the flush cases require an io to complete to finish flushing - not just scan_work.?? nvme_mpath_stop() syncs with ctrl->ana_work, which may be waiting for an ana log to complete.? fw_act_queues() may be in the midst of polling the CSTS bit register or doing a fw_slot log read, or a stop_ctrl() routine for the transport may be in the middle of a start_ctrl call.

These commands won't be retried since REQ_FAILFAST_DRIVER is set for any request allocated via nvme_alloc_request().

Quoted 4 lines

So the above flush cases you mentioned should be easier to handle than IO from scan context. However, looks the patch we discussed before by moving nvme_stop_ctrl() after nvme_rdma_shutdown_ctrl() is still needed for avoiding hang on flush in these commands.

Agree - nvme_stop_ctrl() has to move so the transport routines that stop/return the io can run.? Leaving it in the same place with an argument won't be enough.

Quoted 12 lines

Quoted 8 lines

Give the transport may be at a point where it's lost connectivity to the controller (never really a case for pci unless you consider hot unplug), before calling ctlr stop, the transport has to terminate/return outstanding io and stop further io from being accepted (usually by marking the queue as not connected).

This patch doesn't change this point, all in-flight IO is still terminate/return. With or without this patch, all these IO will be retried too after controller is recovered.

Quoted 2 lines

Or you mean all these IOs have to terminate/return before calling ctrl->ops->stop_ctrl().

Patch as is, that calls the flush routines pointed out, will deadlock as the io won't be terminated and returned by the transport. The flush routines, thus stop_ctrl, has to be called after the transport stops the queues and aborts/returns the ios. Retries after that will do what they do.

But the concern I have is - if things like ns_revalidate requires normal io to be completed to be "flushed", then we have an issue. The non-normal io, generated by the core layer and those issued by multipath will fast-fail. But the normal io, whether non-multipath or last-path with multipath, may not be failed as they will be continually requeued or wait for a new path. So I don't know how it exits this deadlock.

Quoted 12 lines

Quoted 12 lines

transports are
several of the flush cases require an io to complete to finish flushing -
not just scan_work.?? nvme_mpath_stop() syncs with ctrl->ana_work, which may
be waiting for an ana log to complete.? fw_act_queues() may be in the midst
of polling the CSTS bit register or doing a fw_slot log read, or a stop_ctrl() routine for the transport may be in the middle of a start_ctrl
call.

Quoted 8 lines

These commands won't be retried since REQ_FAILFAST_DRIVER is set for any request allocated via nvme_alloc_request().

So the above flush cases you mentioned should be easier to handle than IO from
scan context. However, looks the patch we discussed before by moving nvme_stop_ctrl() after nvme_rdma_shutdown_ctrl() is still needed for avoiding hang on flush in these commands.

Agree - nvme_stop_ctrl() has to move so the transport routines that stop/return the io can run.? Leaving it in the same place with an argument won't be enough.

Quoted 22 lines

Quoted 16 lines

Quoted 12 lines

transports are
several of the flush cases require an io to complete to finish flushing -
not just scan_work.?? nvme_mpath_stop() syncs with ctrl->ana_work, which may
be waiting for an ana log to complete.? fw_act_queues() may be in the midst
of polling the CSTS bit register or doing a fw_slot log read, or a stop_ctrl() routine for the transport may be in the middle of a start_ctrl
call.

These commands won't be retried since REQ_FAILFAST_DRIVER is set for any
request allocated via nvme_alloc_request().

Quoted 5 lines

So the above flush cases you mentioned should be easier to handle than IO from
scan context. However, looks the patch we discussed before by moving nvme_stop_ctrl() after nvme_rdma_shutdown_ctrl() is still needed for avoiding hang on flush in these commands.

Quoted 3 lines

Agree - nvme_stop_ctrl() has to move so the transport routines that stop/return the io can run.? Leaving it in the same place with an argument won't be enough.

nvme_stop_ctrl() cannot move to after queue termination, it has to barrier before moving forward. The things that must flush before are async_event_work that does not go via the normal request path that is quiesced and ->stop_ctrl() at least for the rdma case make sure that no error recovery is concurrent with the reset/delete flow.

Perhaps we can move scan_work flush into nvme_remove_namespaces() instead? I guess it makes sense that when we remove namespaces we don't want a scan to run concurrently?

When scan work is in-progress, any controller error may trigger reset, now fc, rdma and loop host tries to flush scan work inside reset context.

This way can cause deadlock easily because any IO during controler recovery(reset) can't be completed until the recovery is done.

Ming,

Quoted 6 lines

When scan work is in-progress, any controller error may trigger reset, now fc, rdma and loop host tries to flush scan work inside reset context.

This way can cause deadlock easily because any IO during controler recovery(reset) can't be completed until the recovery is done.

Did you encounter this deadlock? or is it theoretical?

The point of nvme_stop_ctrl is to quiesce everything before moving forward with tearing down the controller instead of trying to handle concurrent incoming I/O.

I'm not sure I understand why you say that I/O can only be completed when the reset is done? if the transport entered

a failed state either the inflight I/O is drain or one of the scan work I/O operations times out.

Quoted 2 lines

Did you encounter this deadlock? or is it theoretical?

There are several such reports in Red Hat Bugzilla.

Quoted 6 lines

The point of nvme_stop_ctrl is to quiesce everything before moving forward with tearing down the controller instead of trying to handle concurrent incoming I/O.

I'm not sure I understand why you say that I/O can only be completed when the reset is done? if the transport entered

Please see nvme_rdma_teardown_io_queues(), in which each in-flight request is canceled via nvme_cancel_request(), which just calls nvme_complete_rq() to requeue request(normal IO) to blk-mq sw queue or scheduler queue.

During reset, block request queues are quiesced, so the requeued requests can't be dispatched to nvme driver until reset is done.

That is why all normal I/O can only be completed after reset is done.

Quoted 2 lines

a failed state either the inflight I/O is drain or one of the scan work I/O operations times out.

Timeout only works for in-flight request, as mentioned above, all these requests are canceled and put back into blk-mq sw queue or scheduler queue during reset, so timeout handler can't cover them at all.

Quoted 4 lines

Quoted 2 lines

Did you encounter this deadlock? or is it theoretical?

There are several such reports in Red Hat Bugzilla.

Is there any way to see the reports? I'm looking for the scenario because error recovery does not usually go through the reset flow.

Quoted 5 lines

Quoted 5 lines

The point of nvme_stop_ctrl is to quiesce everything before moving forward with tearing down the controller instead of trying to handle concurrent incoming I/O.

Quoted 7 lines

Quoted 2 lines

I'm not sure I understand why you say that I/O can only be completed when the reset is done? if the transport entered

Please see nvme_rdma_teardown_io_queues(), in which each in-flight request is canceled via nvme_cancel_request(), which just calls nvme_complete_rq() to requeue request(normal IO) to blk-mq sw queue or scheduler queue.

I'm pretty familiar with what nvme_rdma_teardown_io_queues() is doing.

Quoted 2 lines

During reset, block request queues are quiesced, so the requeued requests can't be dispatched to nvme driver until reset is done.

That's fine, they have no reason to be dispatched until the reset is done, it has no chance to complete.

Quoted 1 line

That is why all normal I/O can only be completed after reset is done.

I'm still not getting your point, resets should be able to complete without pending I/Os to complete with a failed status.
That is why I want to see the tickets details, I want to understand what is the issue that this solves.

Quoted 8 lines

Quoted 3 lines

a failed state either the inflight I/O is drain or one of the scan work I/O operations times out.

Timeout only works for in-flight request, as mentioned above, all these requests are canceled and put back into blk-mq sw queue or scheduler queue during reset, so timeout handler can't cover them at all.

Its not supposed to. This patch says it specifically addresses the scan work.

I think you need to explain your patch better to get across exactly what it is fixing.

https://bugzilla.redhat.com/show_bug.cgi?id=1622487
https://bugzilla.redhat.com/show_bug.cgi?id=1628100

Especially you may find some details in the following comment:

https://bugzilla.redhat.com/show_bug.cgi?id=1628100#c7

Quoted 7 lines

https://bugzilla.redhat.com/show_bug.cgi?id=1622487
https://bugzilla.redhat.com/show_bug.cgi?id=1628100

Especially you may find some details in the following comment:

https://bugzilla.redhat.com/show_bug.cgi?id=1628100#c7

I don't have permissions to see the tickets.

Quoted 7 lines

https://bugzilla.redhat.com/show_bug.cgi?id=1622487
https://bugzilla.redhat.com/show_bug.cgi?id=1628100

Especially you may find some details in the following comment:

https://bugzilla.redhat.com/show_bug.cgi?id=1628100#c7

I don't have permissions to see the tickets.