nvme-rdma: fix timeout handler

This was meant to be an RFC, not yet for inclusion. I mostly wanted Jaesoo and Nitzan to provide their feedback...

I have tested the code (backported to v4.19.9) and verified it works great for the scenarios I had originally reported. Also the code looks good to me.

Currently, we have several problems with the timeout handler:
1. If we timeout on the controller establishment flow, we will hang because we don't execute the error recovery (and we shouldn't because the create_ctrl flow needs to fail and cleanup on its own)
2. We might also hang if we get a disconnet on a queue while the controller is already deleting. This racy flow can cause the controller disable/shutdown admin command to hang.

We cannot complete a timed out request from the timeout handler without mutual exclusion from the teardown flow (e.g. nvme_rdma_error_recovery_work). So we serialize it in the timeout handler and stop the queue to guarantee that no one races with us from completing the request.

Reported-by Jaesoo Lee <jalee at purestorage.com>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
  drivers/nvme/host/rdma.c | 33 +++++++++++++++++++++++----------
  1 file changed, 23 insertions(+), 10 deletions(-)

drivers/nvme/host/rdma.c +10 -5

index 80b3113b45fb..af8f4cedabc1 100644
@@ -1080,12 +1080,13 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
  	nvme_rdma_reconnect_or_remove(ctrl);
  }
 
-static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
+static int nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
  {
  	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
-		return;
+		return -EBUSY;
 
  	queue_work(nvme_wq, &ctrl->err_work);
+	return 0;
  }
 
  static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc,

@@ -1693,18 +1694,30 @@ static enum blk_eh_timer_return
  nvme_rdma_timeout(struct request *rq, bool reserved)
  {
  	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
+	struct nvme_rdma_queue *queue = req->queue;
+	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
+
+	dev_warn(ctrl->ctrl.device, "I/O %d QID %d timeout\n",
+		 rq->tag, nvme_rdma_queue_idx(queue));
 
-	dev_warn(req->queue->ctrl->ctrl.device,
-		 "I/O %d QID %d timeout, reset controller\n",
-		 rq->tag, nvme_rdma_queue_idx(req->queue));
+	if (nvme_rdma_error_recovery(req->queue->ctrl)) {
+		union nvme_result res = {};

Currently, we have several problems with the timeout handler:
1. If we timeout on the controller establishment flow, we will hang because we don't execute the error recovery (and we shouldn't because the create_ctrl flow needs to fail and cleanup on its own)
2. We might also hang if we get a disconnet on a queue while the controller is already deleting. This racy flow can cause the controller disable/shutdown admin command to hang.

We cannot complete a timed out request from the timeout handler without mutual exclusion from the teardown flow (e.g. nvme_rdma_error_recovery_work). So we serialize it in the timeout handler and stop the queue to guarantee that no one races with us from completing the request.

Reported-by Jaesoo Lee <jalee at purestorage.com>

Signed-off-by Sagi Grimberg <sagi at grimberg.me>

---
 drivers/nvme/host/rdma.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

drivers/nvme/host/rdma.c +12 -2

index 80b3113b45fb..af8f4cedabc1 100644
@@ -1080,12 +1080,13 @@ static void nvme_rdma_error_recovery_work(struct work_struct *work)
 	nvme_rdma_reconnect_or_remove(ctrl);
 }
 
-static void nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
+static int nvme_rdma_error_recovery(struct nvme_rdma_ctrl *ctrl)
 {
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING))
-		return;
+		return -EBUSY;
 
 	queue_work(nvme_wq, &ctrl->err_work);
+	return 0;
 }
 
 static void nvme_rdma_wr_error(struct ib_cq *cq, struct ib_wc *wc,

@@ -1693,18 +1694,30 @@ static enum blk_eh_timer_return
 nvme_rdma_timeout(struct request *rq, bool reserved)
 {
 	struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
+	struct nvme_rdma_queue *queue = req->queue;
+	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
+
+	dev_warn(ctrl->ctrl.device, "I/O %d QID %d timeout\n",
+		 rq->tag, nvme_rdma_queue_idx(queue));
 
+	if (nvme_rdma_error_recovery(req->queue->ctrl)) {
+		union nvme_result res = {};
 
+		nvme_rdma_stop_queue(queue);
+		flush_work(&ctrl->err_work);

So why did nvme_rdma_error_recovery not complete this request? I think that is where our problem is, and we are just papering over it.

Quoted 3 lines

So why did nvme_rdma_error_recovery not complete this request? I think that is where our problem is, and we are just papering over it.

nvme_rdma_error_recovery in essence, kicks in when the controller is LIVE and only then sees a failure. What it does is teardown the controller and schedule a periodic reconnect work..

When we fail the connection establishment, we don't want to do that, but simply fail and let create_ctrl cleanup and fail.

Technically, nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING) is failing because we are not LIVE, but rather CONNECTING (as we are still in the create_ctrl time).

Quoted 8 lines · Sagi Grimberg

Quoted 3 lines

So why did nvme_rdma_error_recovery not complete this request? I think that is where our problem is, and we are just papering over it.

nvme_rdma_error_recovery in essence, kicks in when the controller is LIVE and only then sees a failure. What it does is teardown the controller and schedule a periodic reconnect work..

Quoted 6 lines

When we fail the connection establishment, we don't want to do that, but simply fail and let create_ctrl cleanup and fail.

Technically, nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING) is failing because we are not LIVE, but rather CONNECTING (as we are still in the create_ctrl time).

which is why I believe the transports need at least a routine that runs down the queues, terminates anything pending (error recovery may be an io timeout), and marks the queues down - regardless of the state of the controller.? That run down will terminate the connection admin commands that may be in error, allowing the fail path Sagi points out to kick in. This same path needs to occur at the start of the reset and delete as well.

Quoted 14 lines · James Smart

Quoted 6 lines · Sagi Grimberg

Quoted 2 lines

So why did nvme_rdma_error_recovery not complete this request? I think that is where our problem is, and we are just papering over it.

nvme_rdma_error_recovery in essence, kicks in when the controller is LIVE and only then sees a failure. What it does is teardown the controller and schedule a periodic reconnect work..

Quoted 6 lines

When we fail the connection establishment, we don't want to do that, but simply fail and let create_ctrl cleanup and fail.

Technically, nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_RESETTING) is failing because we are not LIVE, but rather CONNECTING (as we are still in the create_ctrl time).

Quoted 6 lines

which is why I believe the transports need at least a routine that runs down the queues, terminates anything pending (error recovery may be an io timeout), and marks the queues down - regardless of the state of the controller.? That run down will terminate the connection admin commands that may be in error, allowing the fail path Sagi points out to kick in. This same path needs to occur at the start of the reset and delete as well.

That was my impression as well. Or in fact just allow a transition from connecting to reset (or a special connect terminate state that is very similar except for not recovering)

Quoted 4 lines

That was my impression as well.? Or in fact just allow a transition from connecting to reset (or a special connect terminate state that is very similar except for not recovering)

:)? I had proposed that originally, but it was rejected.? I still think that state transition is valid, but the pci guys disagreed.

I don't think it needs a new state.?? I think a transport callback that can be called from the common layers is sufficient.? rdma already has it in nvme_rdma_teardown_io_queues() and nvme_rdma_teardown_admin_queue(). And fc has it in __nvme_fc_terminate_io().

Quoted 7 lines

which is why I believe the transports need at least a routine that runs down the queues, terminates anything pending (error recovery may be an io timeout), and marks the queues down - regardless of the state of the controller.? That run down will terminate the connection admin commands that may be in error, allowing the fail path Sagi points out to kick in. This same path needs to occur at the start of the reset and delete as well.

That was my impression as well. Or in fact just allow a transition from connecting to reset (or a special connect terminate state that is very similar except for not recovering)

Quoted 9 lines

Quoted 8 lines

which is why I believe the transports need at least a routine that runs down the queues, terminates anything pending (error recovery may be an io
timeout), and marks the queues down - regardless of the state of the controller.? That run down will terminate the connection admin commands that may be in error, allowing the fail path Sagi points out to kick in. This same path needs to occur at the start of the reset and delete as well.

Quoted 3 lines

That was my impression as well.? Or in fact just allow a transition from connecting to reset (or a special connect terminate state that is very similar except for not recovering)

I don't think this is a good idea. Introducing a different state for connect failure is a bit cumbersome for the state machine and not trivial to detect from the .timeout handler.

Moreover, its not the only case where we have an issue. In RDMA (and TCP) at least, we have also the issue that Jaesoo pointed out.

The scenario goes as follows:
1. the host triggers a delete sequence
2. host disconnect I/O queues
3. host issues prop_set (nvme_shutdown_ctrl)
4. target disconnects the admin queue // race with (3)
5. (3) hangs because no one can complete the admin commands it comes after we pass step (3).

So the flows are slightly less trivial.

Moreover, the behavior proposed is not any different than what we do in pci which disables the device (and terminates IO) directly from the timeout handler:

Signature · 52 lines

 ??????? /*
 ???????? * Shutdown immediately if controller times out while starting. 
The
 ???????? * reset work will see the pci device disabled when it gets the 
forced
 ???????? * cancellation error. All outstanding requests are completed on
 ???????? * shutdown, so we return BLK_EH_DONE.
 ???????? */
 ??????? switch (dev->ctrl.state) {
 ??????? case NVME_CTRL_CONNECTING:
 ??????? case NVME_CTRL_RESETTING:
 ??????????????? dev_warn_ratelimited(dev->ctrl.device,
 ???????????????????????? "I/O %d QID %d timeout, disable controller\n",
 ???????????????????????? req->tag, nvmeq->qid);
 ??????????????? nvme_dev_disable(dev, false);
 ??????????????? nvme_req(req)->flags |= NVME_REQ_CANCELLED;
 ??????????????? return BLK_EH_DONE;
 ??????? default:
 ??????????????? break;
 ??????? }
-- 

nvme_dev_disable eventually calls blk_mq_tagset_busy_iter() on all
inflight requests, so it effectively does the same thing.

Would you prefer if we do the below instead (which match pci exactly)?
-- 
static enum blk_eh_timer_return
nvme_rdma_timeout(struct request *rq, bool reserved)
{
 ??????? struct nvme_rdma_request *req = blk_mq_rq_to_pdu(rq);
 ??????? struct nvme_rdma_queue *queue = req->queue;
 ??????? struct nvme_rdma_ctrl *ctrl = queue->ctrl;

 ??????? dev_warn(ctrl->ctrl.device, "I/O %d QID %d timeout\n",
 ???????????????? rq->tag, nvme_rdma_queue_idx(queue));

 ??????? if (nvme_rdma_error_recovery(ctrl)) {
 ??????????????? /*
 ???????????????? * Shutdown immediately if controller times out
 ???????????????? * while starting. All outstanding requests are
 ???????????????? * completed on shutdown, so we return BLK_EH_DONE.
 ???????????????? */
 ??????????????? flush_work(&ctrl->err_work);
 ??????????????? nvme_rdma_stop_queue(queue);
 ??????????????? nvme_rdma_shutdown_ctrl(queue->ctrl, false);
 ??????????????? return BLK_EH_DONE;
 ??????? }

 ??????? return BLK_EH_RESET_TIMER;
}
--