iwarp device removal deadlock fix

Remove the complicated logic to free the cm_id resources in iw_cm event handlers vs when an application thread destroys the device. I'm not sure why this code was written, but simply allowing the last deref to free the memory is cleaner. It also prevents a deadlock when applications try to destroy cm_id's in their cm event handler function.

Quoted 7 lines

Remove the complicated logic to free the cm_id resources in iw_cm event handlers vs when an application thread destroys the device. I'm not sure why this code was written, but simply allowing the last deref to free the memory is cleaner. It also prevents a deadlock when applications try to destroy cm_id's in their cm event handler function.

The description here is misleading. we can never destroy the cm_id inside the cm_id handler. Also, I don't think the deadlock was on cm_id removal but rather on the qp referenced by the cm_id. I think the change log can be improved.

The patch looks fine to me.

Quoted 6 lines

Quoted 5 lines

Remove the complicated logic to free the cm_id resources in iw_cm event handlers vs when an application thread destroys the device. I'm not sure why this code was written, but simply allowing the last deref to free the memory is cleaner. It also prevents a deadlock when applications try to destroy cm_id's in their cm event handler function.

Quoted 5 lines

The description here is misleading. we can never destroy the cm_id inside the cm_id handler. Also, I don't think the deadlock was on cm_id removal but rather on the qp referenced by the cm_id. I think the change log can be improved.

I'll reword it.

While I'm rambling, there is still a condition that probably needs to be
addressed: if the application event handler function disconnects the cm_id that is handling the event, the iw_cm workq thread gets stuck posting a IW_CM_EVENT_CLOSE to rdma_cm. So the iw_cm workq thread is stuck in cm_close_handler() calling cm_id_priv->id.cm_handler() which is cma_iw_handler() which is blocked in cma_disable_callback() because the application is currently running its event handler for this cm_id. This block is released when the application returns from its event handler function.

But maybe cma_iw_handler() should queue the event if it cannot deliver it, vs blocking the iw_cm workq thread?

Device removal sequence may have crashed because the controller (and admin queue space) was freed before we destroyed the admin queue resources. Thus we want to destroy the admin queue and only then queue controller deletion and wait for it to complete.

More specifically we:
1. own the controller deletion (make sure we are not competing with another deletion).
2. get rid of inflight reconnects if exists (which also destroy and create queues).
3. destroy the queue.
4. safely queue controller deletion (and wait for it to complete).

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

---
 drivers/nvme/host/rdma.c | 49 ++++++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

drivers/nvme/host/rdma.c +17 -4

index 3e3ce2b..0e58450 100644
@@ -169,7 +169,6 @@ MODULE_PARM_DESC(register_always,
 static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 		struct rdma_cm_event *event);
 static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc);
-static int __nvme_rdma_del_ctrl(struct nvme_rdma_ctrl *ctrl);
 
 /* XXX: really should move to a generic header sooner or later.. */
 static inline void put_unaligned_le24(u32 val, u8 *p)

@@ -1318,37 +1317,43 @@ out_destroy_queue_ib:
  * that caught the event. Since we hold the callout until the controller
  * deletion is completed, we'll deadlock if the controller deletion will
  * call rdma_destroy_id on this queue's cm_id. Thus, we claim ownership
- * of destroying this queue before-hand, destroy the queue resources
- * after the controller deletion completed with the exception of destroying
- * the cm_id implicitely by returning a non-zero rc to the callout.
+ * of destroying this queue before-hand, destroy the queue resources,
+ * then queue the controller deletion which won't destroy this queue and
+ * we destroy the cm_id implicitely by returning a non-zero rc to the callout.
  */
 static int nvme_rdma_device_unplug(struct nvme_rdma_queue *queue)
 {
 	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
+	int ret;
 
+	/* Own the controller deletion */
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
+		return 0;
 
+	dev_warn(ctrl->ctrl.device,
+		"Got rdma device removal event, deleting ctrl\n");
 
+	/* Get rid of reconnect work if its running */
+	cancel_delayed_work_sync(&ctrl->reconnect_work);
 
+	/* Disable the queue so ctrl delete won't free it */
+	if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags)) {
+		ret = 0;
+		goto queue_delete;
 	}
 
+	/* Free this queue ourselves */
+	nvme_rdma_stop_queue(queue);

Device removal sequence may have crashed because the controller (and admin queue space) was freed before we destroyed the admin queue resources. Thus we want to destroy the admin queue and only then queue controller deletion and wait for it to complete.

More specifically we:
1. own the controller deletion (make sure we are not competing with another deletion).
2. get rid of inflight reconnects if exists (which also destroy and create queues).
3. destroy the queue.
4. safely queue controller deletion (and wait for it to complete).

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

---
 drivers/nvme/host/rdma.c | 49

 1 file changed, 27 insertions(+), 22 deletions(-)

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

index 3e3ce2b..0e58450 100644
@@ -169,7 +169,6 @@ MODULE_PARM_DESC(register_always,
 static int nvme_rdma_cm_handler(struct rdma_cm_id *cm_id,
 		struct rdma_cm_event *event);
 static void nvme_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc);
-static int __nvme_rdma_del_ctrl(struct nvme_rdma_ctrl *ctrl);
 
 /* XXX: really should move to a generic header sooner or later.. */
 static inline void put_unaligned_le24(u32 val, u8 *p)

drivers/nvme/host/rdma.c +13 -18

  */
 static int nvme_rdma_device_unplug(struct nvme_rdma_queue *queue)
 {
 	struct nvme_rdma_ctrl *ctrl = queue->ctrl;
-	int ret, ctrl_deleted = 0;
+	int ret;
 
-	/* First disable the queue so ctrl delete won't free it */
-	if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags))
-		goto out;
+	/* Own the controller deletion */
+	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING))
+		return 0;
 
-	/* delete the controller */
-	ret = __nvme_rdma_del_ctrl(ctrl);
-	if (!ret) {
-		dev_warn(ctrl->ctrl.device,
-			"Got rdma device removal event, deleting ctrl\n");
-		flush_work(&ctrl->delete_work);
+	dev_warn(ctrl->ctrl.device,
+		"Got rdma device removal event, deleting ctrl\n");
 
-		/* Return non-zero so the cm_id will destroy implicitly */
-		ctrl_deleted = 1;
+	/* Get rid of reconnect work if its running */
+	cancel_delayed_work_sync(&ctrl->reconnect_work);
 
-		/* Free this queue ourselves */
-		rdma_disconnect(queue->cm_id);
-		ib_drain_qp(queue->qp);
-		nvme_rdma_destroy_queue_ib(queue);
+	/* Disable the queue so ctrl delete won't free it */
+	if (!test_and_clear_bit(NVME_RDMA_Q_CONNECTED, &queue->flags)) {
+		ret = 0;
+		goto queue_delete;
 	}
 
-out:
-	return ctrl_deleted;
+	/* Free this queue ourselves */

This RFC series attempts to address the deadlock issue discovered while testing nvmf/rdma handling rdma device removal events from the rdma_cm.

For a discussion of the deadlock that can happen, see

http://lists.infradead.org/pipermail/linux-nvme/2016-July/005440.html.

For my description of the deadlock itself, see this post in the above thread:

http://lists.infradead.org/pipermail/linux-nvme/2016-July/005465.html

In a nutshell, iw_cxgb4 and the iw_cm block during qp/cm_id destruction until all references are removed. This combined with the iwarp CM passing disconnect events up to the rdma_cm during disconnect and/or qp/cm_id destruction leads to a deadlock.

My proposed solution is to remove the need for iw_cxgb4 and iw_cm to block during object destruction for the recnts to reach 0, but rather to let the freeing of the object memory be deferred when the last deref is done. This allows all the qps/cm_ids to be destroyed without blocking, and all the object memory freeing ends up happinging when the application's device_remove event handler function returns to the rdma_cm.

Sean, I was hoping you could have a look at the iwcm.c patch particularly, to tell my why its broken. :) I spent some time trying to figure out why we really need the CALLBACK_DESTROY flag, but I concluded it really isn't needed. The one side effect I see with my change, is that the application could possibly get a cm_id event after it has destroyed the cm_id. There probably is a way to discard events that have a reference on the cm_id but get processed after the app has destoyed the cm_id by having a new flag indicating "destroyed by app".

Quoted 3 lines

This RFC series attempts to address the deadlock issue discovered while testing nvmf/rdma handling rdma device removal events from the rdma_cm.

Thanks for doing this Steve!

Quoted 5 lines

For a discussion of the deadlock that can happen, see

http://lists.infradead.org/pipermail/linux-nvme/2016-July/005440.html.

For my description of the deadlock itself, see this post in the above

Quoted 6 lines

http://lists.infradead.org/pipermail/linux-nvme/2016-July/005465.html

In a nutshell, iw_cxgb4 and the iw_cm block during qp/cm_id destruction until all references are removed. This combined with the iwarp CM passing disconnect events up to the rdma_cm during disconnect and/or qp/cm_id

destruction

Quoted 2 lines

leads to a deadlock.

Quoted 6 lines

My proposed solution is to remove the need for iw_cxgb4 and iw_cm to block during object destruction for the recnts to reach 0, but rather to let the freeing of the object memory be deferred when the last deref is done. This allows all the qps/cm_ids to be destroyed without blocking, and all the object memory freeing ends up happinging when the application's device_remove event handler function returns to the rdma_cm.

This sounds like a very good approach moving forward.

Quoted 8 lines

Sean, I was hoping you could have a look at the iwcm.c patch particularly, to tell my why its broken. :) I spent some time trying to figure out why we really need the CALLBACK_DESTROY flag, but I concluded it really isn't needed. The one side effect I see with my change, is that the application could possibly get a cm_id event after it has destroyed the cm_id. There probably is a way to discard events that have a reference on the cm_id but get processed after the app has destoyed the cm_id by having a new flag indicating "destroyed by app".

That sounds easy enough. Does this mean that iwcm relies on the driver to do this or is it inter-operable with the existing logic? If not this will need to take care of all the iWARP drivers.