Skip to contentSkip to conversation

nvme-rdma: fix error flow during controller reset

PATCH New Max Gurtovoy 3 participants 4 messages Jun 17, 2018 – Jun 19, 2018 NVMe

lore
Share & tools
lore

Max Gurtovoy

author
#0 lore raw

PATCH 1/1 nvme-rdma: fix error flow during controller reset

In case the IO queues configuration fails, we never destroy the admin queue.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/rdma.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/nvme/host/rdma.c +3 -1
index cef24ad..c193f61 100644
@@ -1781,7 +1781,7 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 	if (ctrl->ctrl.queue_count > 1) {
 		ret = nvme_rdma_configure_io_queues(ctrl, false);
 		if (ret)
-			goto out_fail;
+			goto destroy_admin;
 	}
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);

@@ -1795,6 +1795,8 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 
 	return;
 
+destroy_admin:
+	nvme_rdma_destroy_admin_queue(ctrl, false);
 out_fail:
 	++ctrl->ctrl.nr_reconnects;
 	nvme_rdma_reconnect_or_remove(ctrl);
Signature · 1 line
1.8.3.1
maintainer
#1 lore raw

PATCH 1/1 nvme-rdma: fix error flow during controller reset

Quoted 8 lines
drivers/nvme/host/rdma.c +1 -1
 		ret = nvme_rdma_configure_io_queues(ctrl, false);
 		if (ret)
-			goto out_fail;
+			goto destroy_admin;
 	}
 
 	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);

drivers/nvme/host/rdma.c static void nvme_rdma_reset_ctrl_work(struct work_struct *work) · line 1802

Quoted 8 lines
drivers/nvme/host/rdma.c +2 -0
@@ -1795,6 +1795,8 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 
 	return;
 
+destroy_admin:
+	nvme_rdma_destroy_admin_queue(ctrl, false);
 out_fail:
 	++ctrl->ctrl.nr_reconnects;
 	nvme_rdma_reconnect_or_remove(ctrl);

What about:

nvme_rdma_reconnect_or_remove
-> nvme_delete_ctrl
-> nvme_rdma_delete_ctrl
-> nvme_rdma_shutdown_ctrl

    -> nvme_rdma_destroy_admin_queue

?

author
#2 lore raw

PATCH 1/1 nvme-rdma: fix error flow during controller reset

Quoted 19 lines · Christoph Hellwig
Quoted 8 lines
  		ret = nvme_rdma_configure_io_queues(ctrl, false);
  		if (ret)
drivers/nvme/host/rdma.c +1 -1
-			goto out_fail;
+			goto destroy_admin;
  	}
 
  	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);

drivers/nvme/host/rdma.c static void nvme_rdma_reset_ctrl_work(struct work_struct *work) · line 1802

Quoted 8 lines
drivers/nvme/host/rdma.c +2 -0
@@ -1795,6 +1795,8 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 
  	return;
 
+destroy_admin:
+	nvme_rdma_destroy_admin_queue(ctrl, false);
  out_fail:
  	++ctrl->ctrl.nr_reconnects;
  	nvme_rdma_reconnect_or_remove(ctrl);

What about:

Quoted 8 lines

nvme_rdma_reconnect_or_remove
-> nvme_delete_ctrl
-> nvme_rdma_delete_ctrl

    -> nvme_rdma_shutdown_ctrl
     -> nvme_rdma_destroy_admin_queue

?

if you worry about calling nvme_rdma_destroy_admin_queue twice so it's fine since we check test_and_clear_bit in nvme_rdma_stop_queue and nvme_rdma_free_queue

maintainer
#3 lore raw

PATCH 1/1 nvme-rdma: fix error flow during controller reset

In reply to Christoph Hellwig
Quoted 20 lines
Quoted 9 lines
  		ret = nvme_rdma_configure_io_queues(ctrl, false);
  		if (ret)
drivers/nvme/host/rdma.c +1 -1
-			goto out_fail;
+			goto destroy_admin;
  	}
 
  	changed = nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_LIVE);

drivers/nvme/host/rdma.c static void nvme_rdma_reset_ctrl_work(struct work_struct *work) · line 1802

Quoted 8 lines
drivers/nvme/host/rdma.c +2 -0
@@ -1795,6 +1795,8 @@ static void nvme_rdma_reset_ctrl_work(struct work_struct *work)
 
  	return;
 
+destroy_admin:
+	nvme_rdma_destroy_admin_queue(ctrl, false);
  out_fail:
  	++ctrl->ctrl.nr_reconnects;
  	nvme_rdma_reconnect_or_remove(ctrl);

What about:

Quoted 7 lines

nvme_rdma_reconnect_or_remove
-> nvme_delete_ctrl
-> nvme_rdma_delete_ctrl

    -> nvme_rdma_shutdown_ctrl
     -> nvme_rdma_destroy_admin_queue

?

I think this is a result of code duplicated in different flows which drifted although the logic should remain the same...