Deadlock on device removal event for NVMeF target

Deadlock on device removal event for NVMeF target

[Shiraz Saleem]

Hi Sagi/Christoph,

I am seeing a deadlock for a device removal event on NVMeF target.

The sequence of events leading to the deadlock are as follows,

1. i40iw posts IW_CM_EVENT_CLOSE events for all QPs causing the corresponding 
NVMet RDMA Queues to disconnect and schedule release of any pending work on WQ
2. i40iw triggers device removal 
	ib_unregister_device
	[..]
	cma_remove_id_dev (takes a handler lock before calling the event handler)
	nvmet_rdma_cm_handler
	nvmet_rdma_device_removal (queue->state = NVMET_RDMA_Q_DISCONNECTING due to 1.)
	flush_scheduled_work (blocks till all scheduled work is drained from WQ)
	nvmet_rdma_release_queue_work (queue->state = NVMET_RDMA_Q_DISCONNECTING)
	rdma_destroy_id (waits on the same handler lock as cma_remove_id_dev causing the deadlock)
     
So this problem can occur when there is a device removal event while the queue is in 
disconnect state with the some oustanding work that hasnt been drained from the WQ at the
time flush_scheduled_work is called.

Here is a call trace with some debug logs. The problematic cm_id is ffff8806e9924d18.

[371.577371] nvmet_rdma: nvmet_rdma_cm_handler: disconnected (10): cm_id ffff8806de8acd18
[371.577378] nvmet_rdma: __nvmet_rdma_queue_disconnect: cm_id= ffff8806de8acd18 queue->state= 1 queue->idx = 1
[371.577381] nvmet_rdma: __nvmet_rdma_queue_disconnect: rdma_disconnect cm_id= ffff8806de8acd18
[371.577501] nvmet_rdma: nvmet_rdma_cm_handler: disconnected (10): cm_id ffff8806e9924d18
[371.577502] nvmet_rdma: nvmet_rdma_cm_handler: device removal (11): cm_id ffff8806e6b6d5a8
[371.577504] nvmet_rdma: nvmet_rdma_device_removal: queue           (null)
[371.577506] nvmet_rdma: __nvmet_rdma_queue_disconnect: cm_id= ffff8806e9924d18 queue->state= 1 queue->idx = 0
[371.577507] rdma_destroy_id id ffff8806e6b6d5a8
[371.577509] nvmet_rdma: __nvmet_rdma_queue_disconnect: rdma_disconnect cm_id= ffff8806e9924d18
[371.577870] nvmet_rdma: nvmet_rdma_cm_handler: device removal (11): cm_id ffff8806e9924d18
[371.577877] nvmet_rdma: nvmet_rdma_device_removal: calling flush_scheduled_work queue state 2 idx 0 cm_id ffff8806e9924d18
[371.580174] nvmet_rdma: nvmet_rdma_queue_response_post_send: 2 opcode 2 queue_state 2 queue_idx 0
[371.580198] nvmet_rdma: sending cmd response failed
[371.596356] nvmet_rdma: nvmet_rdma_release_queue_work: calling rdma_destroy_id cm_id ffff8806de8acd18 queue_idx 1 state 2
[371.596358] rdma_destroy_id id ffff8806de8acd18
[371.596512] nvmet_rdma: nvmet_rdma_release_queue_work: calling rdma_destroy_id cm_id ffff8806e9924d18 queue_idx 0 state 2
[371.596514] rdma_destroy_id id ffff8806e9924d18                    ------------------------------------------------> Hang                                                             
[614.390162] INFO: task kworker/6:1:4429 blocked for more than 120 seconds.
[614.390165]       Tainted: G           O    4.11.0-rc8+ #11
[614.390166] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[614.390167] kworker/6:1     D30016  4429      2 0x00000000
[614.390176] Workqueue: events nvmet_rdma_release_queue_work [nvmet_rdma]
[614.390178] Call Trace:
[614.390182]  __schedule+0x378/0xbf0
[614.390185]  schedule+0x38/0x90
[614.390186]  schedule_preempt_disabled+0x10/0x20
[614.390188]  __mutex_lock+0x2ac/0x960
[614.390190]  ? __mutex_lock+0xb6/0x960
[614.390193]  ? rdma_destroy_id+0x6a/0x2e0 [rdma_cm]
[614.390196]  mutex_lock_nested+0x16/0x20
[614.390198]  rdma_destroy_id+0x6a/0x2e0 [rdma_cm]
[614.390201]  nvmet_rdma_release_queue_work+0x78/0x80 [nvmet_rdma]
[614.390203]  process_one_work+0x203/0x710
[614.390204]  ? process_one_work+0x16f/0x710
[614.390207]  worker_thread+0x126/0x4a0
[614.390210]  kthread+0x112/0x150
[614.390212]  ? process_one_work+0x710/0x710
[614.390213]  ? kthread_create_on_node+0x40/0x40
[614.390216]  ret_from_fork+0x2e/0x40
[614.390219] INFO: lockdep is turned off.
[614.390239] INFO: task rmmod:8443 blocked for more than 120 seconds.
[614.390241]       Tainted: G           O    4.11.0-rc8+ #11
[614.390242] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[614.390243] rmmod           D29880  8443   7036 0x00000080
[614.390248] Call Trace:
[614.390250]  __schedule+0x378/0xbf0
[614.390252]  schedule+0x38/0x90
[614.390254]  schedule_timeout+0x235/0x4f0
[614.390257]  ? mark_held_locks+0x71/0x90
[614.390259]  ? _raw_spin_unlock_irq+0x27/0x40
[614.390261]  wait_for_completion+0xb4/0x120
[614.390264]  ? wake_up_q+0x70/0x70
[614.390267]  flush_workqueue+0x211/0x6c0
[614.390269]  ? flush_workqueue+0x100/0x6c0
[614.390271]  ? vprintk_emit+0x211/0x2e0
[614.390275]  ? printk+0x43/0x45
[614.390277]  ? wait_for_completion+0x36/0x120
[614.390280]  nvmet_rdma_cm_handler+0x34f/0xf50 [nvmet_rdma]
[614.390282]  ? cma_comp+0x39/0x50 [rdma_cm]
[614.390284]  ? mark_held_locks+0x71/0x90
[614.390286]  ? _raw_spin_unlock_irqrestore+0x31/0x50
[614.390289]  ? trace_hardirqs_on+0xd/0x10
[614.390292]  cma_remove_one+0x1fc/0x220 [rdma_cm]
[614.390299]  ib_unregister_device+0xd8/0x170 [ib_core]
[614.390303]  i40iw_destroy_rdma_device+0x55/0x160 [i40iw]
[614.390307]  i40iw_deinit_device+0x96/0x460 [i40iw]
[614.390311]  i40iw_close+0x54/0xc0 [i40iw]
[614.390316]  i40e_unregister_client+0xaf/0x1b0 [i40e]
[614.390320]  i40iw_exit_module+0x10/0xaa4 [i40iw]
[614.390322]  SyS_delete_module+0x16a/0x230
[614.390325]  do_syscall_64+0x59/0x1a0
[614.390327]  entry_SYSCALL64_slow_path+0x25/0x25
[614.390329] RIP: 0033:0x7f955841d697
[614.390339] INFO: lockdep is turned off

This patch fixed the problem but I am not sure if it is the correct fix.

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 9e45cde..d0fb307 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1349,6 +1349,12 @@ static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
                spin_lock_irqsave(&queue->state_lock, flags);
                if (queue->state != NVMET_RDMA_Q_DISCONNECTING)
                        queue->state = NVMET_RDMA_IN_DEVICE_REMOVAL;
+               else {
+                       /*queue is disconnecting; so cm_id and queues will be destroyed*/
+                       spin_unlock_irqrestore(&queue->state_lock, flags);
+                       return 0;
+               }
+
                spin_unlock_irqrestore(&queue->state_lock, flags);
                nvmet_rdma_queue_disconnect(queue);
                flush_scheduled_work();


Shiraz
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Deadlock on device removal event for NVMeF target

[Sagi Grimberg]

> Hi Sagi/Christoph,

Hi Shiraz,

Please CC linux-nvme for nvme-rdma related stuff.

> I am seeing a deadlock for a device removal event on NVMeF target.
> 
> The sequence of events leading to the deadlock are as follows,
> 
> 1. i40iw posts IW_CM_EVENT_CLOSE events for all QPs causing the corresponding
> NVMet RDMA Queues to disconnect and schedule release of any pending work on WQ
> 2. i40iw triggers device removal
> 	ib_unregister_device
> 	[..]
> 	cma_remove_id_dev (takes a handler lock before calling the event handler)
> 	nvmet_rdma_cm_handler
> 	nvmet_rdma_device_removal (queue->state = NVMET_RDMA_Q_DISCONNECTING due to 1.)
> 	flush_scheduled_work (blocks till all scheduled work is drained from WQ)
> 	nvmet_rdma_release_queue_work (queue->state = NVMET_RDMA_Q_DISCONNECTING)
> 	rdma_destroy_id (waits on the same handler lock as cma_remove_id_dev causing the deadlock)
>       
> So this problem can occur when there is a device removal event while the queue is in
> disconnect state with the some oustanding work that hasnt been drained from the WQ at the
> time flush_scheduled_work is called.

This indeed looks like a bug (thanks for reporting!). We indeed don't
have sufficient information on where the queue release procedure is by
only looking at the queue state, we can't tell if rdma_destroy_id was
invoked and we can deadlock with rdma_destroy_id.

> This patch fixed the problem but I am not sure if it is the correct fix.
> 
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 9e45cde..d0fb307 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1349,6 +1349,12 @@ static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
>                  spin_lock_irqsave(&queue->state_lock, flags);
>                  if (queue->state != NVMET_RDMA_Q_DISCONNECTING)
>                          queue->state = NVMET_RDMA_IN_DEVICE_REMOVAL;
> +               else {
> +                       /*queue is disconnecting; so cm_id and queues will be destroyed*/
> +                       spin_unlock_irqrestore(&queue->state_lock, flags);
> +                       return 0;
> +               }
> +
>                  spin_unlock_irqrestore(&queue->state_lock, flags);
>                  nvmet_rdma_queue_disconnect(queue);
>                  flush_scheduled_work();
> 

The problem with your patch, is that it introduces a race in the
opposite direction. The queue state does not indicate that the queue
release work has _started_, it only indicates that it was queued.
The DEVICE_REMOVAL event expects that by the end of the callout _all_
associated resources have been freed and the device can safely continue
with its teardown flow.

How about the (untested) alternative below:
--
[PATCH] nvmet-rdma: register ib_client to not deadlock in device
  removal

We can deadlock in case we got to a device removal
event on a queue which is already in the process of
destroying the cm_id is this is blocking until all
events on this cm_id will drain. On the other hand
we cannot guarantee that rdma_destroy_id was invoked
as we only have indication that the queue disconnect
flow has been queued (the queue state is updated before
the realease work has been queued).

So, we leave all the queue removal to a separate ib_client
to avoid this deadlock as ib_client device removal is in
a different context than the cm_id itself.

Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
  drivers/nvme/target/rdma.c | 100 
++++++++++++++++++++++++++++++---------------
  1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 32aa10b521c8..56a4cba690b5 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1307,53 +1307,44 @@ static void nvmet_rdma_queue_connect_fail(struct 
rdma_cm_id *cm_id,

  /**
   * nvme_rdma_device_removal() - Handle RDMA device removal
+ * @cm_id:     rdma_cm id, used for nvmet port
   * @queue:      nvmet rdma queue (cm id qp_context)
- * @addr:      nvmet address (cm_id context)
   *
   * DEVICE_REMOVAL event notifies us that the RDMA device is about
- * to unplug so we should take care of destroying our RDMA resources.
- * This event will be generated for each allocated cm_id.
+ * to unplug. Note that this event can be generated on a normal
+ * queue cm_id and/or a device bound listener cm_id (where in this
+ * case queue will be null).
   *
- * Note that this event can be generated on a normal queue cm_id
- * and/or a device bound listener cm_id (where in this case
- * queue will be null).
- *
- * we claim ownership on destroying the cm_id. For queues we move
- * the queue state to NVMET_RDMA_IN_DEVICE_REMOVAL and for port
+ * We registered an ib_client to handle device removal for queues,
+ * so we only need to handle the listening port cm_ids. In this case
   * we nullify the priv to prevent double cm_id destruction and destroying
   * the cm_id implicitely by returning a non-zero rc to the callout.
   */
  static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
                 struct nvmet_rdma_queue *queue)
  {
-       unsigned long flags;
-
-       if (!queue) {
-               struct nvmet_port *port = cm_id->context;
+       struct nvmet_port *port;

+       if (queue) {
                 /*
-                * This is a listener cm_id. Make sure that
-                * future remove_port won't invoke a double
-                * cm_id destroy. use atomic xchg to make sure
-                * we don't compete with remove_port.
-                */
-               if (xchg(&port->priv, NULL) != cm_id)
-                       return 0;
-       } else {
-               /*
-                * This is a queue cm_id. Make sure that
-                * release queue will not destroy the cm_id
-                * and schedule all ctrl queues removal (only
-                * if the queue is not disconnecting already).
+                * This is a queue cm_id. we have registered
+                * an ib_client to handle queues removal
+                * so don't interfear and just return.
                  */
-               spin_lock_irqsave(&queue->state_lock, flags);
-               if (queue->state != NVMET_RDMA_Q_DISCONNECTING)
-                       queue->state = NVMET_RDMA_IN_DEVICE_REMOVAL;
-               spin_unlock_irqrestore(&queue->state_lock, flags);
-               nvmet_rdma_queue_disconnect(queue);
-               flush_scheduled_work();
+               return 0;
         }

+       port = cm_id->context;
+
+       /*
+        * This is a listener cm_id. Make sure that
+        * future remove_port won't invoke a double
+        * cm_id destroy. use atomic xchg to make sure
+        * we don't compete with remove_port.
+        */
+       if (xchg(&port->priv, NULL) != cm_id)
+               return 0;
+
         /*
          * We need to return 1 so that the core will destroy
          * it's own ID.  What a great API design..
@@ -1519,9 +1510,51 @@ static struct nvmet_fabrics_ops nvmet_rdma_ops = {
         .delete_ctrl            = nvmet_rdma_delete_ctrl,
  };

+static void nvmet_rdma_add_one(struct ib_device *ib_device)
+{
+}
+
+static void nvmet_rdma_remove_one(struct ib_device *ib_device, void 
*client_data)
+{
+       struct nvmet_rdma_queue *queue;
+
+       /* Device is being removed, delete all queues using this device */
+       mutex_lock(&nvmet_rdma_queue_mutex);
+       list_for_each_entry(queue, &nvmet_rdma_queue_list, queue_list) {
+               if (queue->dev->device != ib_device)
+                       continue;
+
+               pr_info("Removing queue %d\n", queue->idx);
+               __nvmet_rdma_queue_disconnect(queue);
+       }
+       mutex_unlock(&nvmet_rdma_queue_mutex);
+
+       flush_scheduled_work();
+}
+
+static struct ib_client nvmet_rdma_ib_client = {
+       .name   = "nvmet_rdma",
+       .add = nvmet_rdma_add_one,
+       .remove = nvmet_rdma_remove_one
+};
+
  static int __init nvmet_rdma_init(void)
  {
-       return nvmet_register_transport(&nvmet_rdma_ops);
+       int ret;
+
+       ret = ib_register_client(&nvmet_rdma_ib_client);
+       if (ret)
+               return ret;
+
+       ret = nvmet_register_transport(&nvmet_rdma_ops);
+       if (ret)
+               goto err_ib_client;
+
+       return 0;
+
+err_ib_client:
+       ib_unregister_client(&nvmet_rdma_ib_client);
+       return ret;
  }

  static void __exit nvmet_rdma_exit(void)
@@ -1544,6 +1577,7 @@ static void __exit nvmet_rdma_exit(void)
         mutex_unlock(&nvmet_rdma_queue_mutex);

         flush_scheduled_work();
+       ib_unregister_client(&nvmet_rdma_ib_client);
         ida_destroy(&nvmet_rdma_queue_ida);
  }

--

Unfortunately, the patch failed to produce a clear diff :/ sorry...
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deadlock on device removal event for NVMeF target

[Sagi Grimberg]

> Hi Sagi/Christoph,

Hi Shiraz,

Please CC linux-nvme for nvme-rdma related stuff.

> I am seeing a deadlock for a device removal event on NVMeF target.
> 
> The sequence of events leading to the deadlock are as follows,
> 
> 1. i40iw posts IW_CM_EVENT_CLOSE events for all QPs causing the corresponding
> NVMet RDMA Queues to disconnect and schedule release of any pending work on WQ
> 2. i40iw triggers device removal
> 	ib_unregister_device
> 	[..]
> 	cma_remove_id_dev (takes a handler lock before calling the event handler)
> 	nvmet_rdma_cm_handler
> 	nvmet_rdma_device_removal (queue->state = NVMET_RDMA_Q_DISCONNECTING due to 1.)
> 	flush_scheduled_work (blocks till all scheduled work is drained from WQ)
> 	nvmet_rdma_release_queue_work (queue->state = NVMET_RDMA_Q_DISCONNECTING)
> 	rdma_destroy_id (waits on the same handler lock as cma_remove_id_dev causing the deadlock)
>       
> So this problem can occur when there is a device removal event while the queue is in
> disconnect state with the some oustanding work that hasnt been drained from the WQ at the
> time flush_scheduled_work is called.

This indeed looks like a bug (thanks for reporting!). We indeed don't
have sufficient information on where the queue release procedure is by
only looking at the queue state, we can't tell if rdma_destroy_id was
invoked and we can deadlock with rdma_destroy_id.

> This patch fixed the problem but I am not sure if it is the correct fix.
> 
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 9e45cde..d0fb307 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -1349,6 +1349,12 @@ static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
>                  spin_lock_irqsave(&queue->state_lock, flags);
>                  if (queue->state != NVMET_RDMA_Q_DISCONNECTING)
>                          queue->state = NVMET_RDMA_IN_DEVICE_REMOVAL;
> +               else {
> +                       /*queue is disconnecting; so cm_id and queues will be destroyed*/
> +                       spin_unlock_irqrestore(&queue->state_lock, flags);
> +                       return 0;
> +               }
> +
>                  spin_unlock_irqrestore(&queue->state_lock, flags);
>                  nvmet_rdma_queue_disconnect(queue);
>                  flush_scheduled_work();
> 

The problem with your patch, is that it introduces a race in the
opposite direction. The queue state does not indicate that the queue
release work has _started_, it only indicates that it was queued.
The DEVICE_REMOVAL event expects that by the end of the callout _all_
associated resources have been freed and the device can safely continue
with its teardown flow.

How about the (untested) alternative below:
--
[PATCH] nvmet-rdma: register ib_client to not deadlock in device
  removal

We can deadlock in case we got to a device removal
event on a queue which is already in the process of
destroying the cm_id is this is blocking until all
events on this cm_id will drain. On the other hand
we cannot guarantee that rdma_destroy_id was invoked
as we only have indication that the queue disconnect
flow has been queued (the queue state is updated before
the realease work has been queued).

So, we leave all the queue removal to a separate ib_client
to avoid this deadlock as ib_client device removal is in
a different context than the cm_id itself.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
  drivers/nvme/target/rdma.c | 100 
++++++++++++++++++++++++++++++---------------
  1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 32aa10b521c8..56a4cba690b5 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1307,53 +1307,44 @@ static void nvmet_rdma_queue_connect_fail(struct 
rdma_cm_id *cm_id,

  /**
   * nvme_rdma_device_removal() - Handle RDMA device removal
+ * @cm_id:     rdma_cm id, used for nvmet port
   * @queue:      nvmet rdma queue (cm id qp_context)
- * @addr:      nvmet address (cm_id context)
   *
   * DEVICE_REMOVAL event notifies us that the RDMA device is about
- * to unplug so we should take care of destroying our RDMA resources.
- * This event will be generated for each allocated cm_id.
+ * to unplug. Note that this event can be generated on a normal
+ * queue cm_id and/or a device bound listener cm_id (where in this
+ * case queue will be null).
   *
- * Note that this event can be generated on a normal queue cm_id
- * and/or a device bound listener cm_id (where in this case
- * queue will be null).
- *
- * we claim ownership on destroying the cm_id. For queues we move
- * the queue state to NVMET_RDMA_IN_DEVICE_REMOVAL and for port
+ * We registered an ib_client to handle device removal for queues,
+ * so we only need to handle the listening port cm_ids. In this case
   * we nullify the priv to prevent double cm_id destruction and destroying
   * the cm_id implicitely by returning a non-zero rc to the callout.
   */
  static int nvmet_rdma_device_removal(struct rdma_cm_id *cm_id,
                 struct nvmet_rdma_queue *queue)
  {
-       unsigned long flags;
-
-       if (!queue) {
-               struct nvmet_port *port = cm_id->context;
+       struct nvmet_port *port;

+       if (queue) {
                 /*
-                * This is a listener cm_id. Make sure that
-                * future remove_port won't invoke a double
-                * cm_id destroy. use atomic xchg to make sure
-                * we don't compete with remove_port.
-                */
-               if (xchg(&port->priv, NULL) != cm_id)
-                       return 0;
-       } else {
-               /*
-                * This is a queue cm_id. Make sure that
-                * release queue will not destroy the cm_id
-                * and schedule all ctrl queues removal (only
-                * if the queue is not disconnecting already).
+                * This is a queue cm_id. we have registered
+                * an ib_client to handle queues removal
+                * so don't interfear and just return.
                  */
-               spin_lock_irqsave(&queue->state_lock, flags);
-               if (queue->state != NVMET_RDMA_Q_DISCONNECTING)
-                       queue->state = NVMET_RDMA_IN_DEVICE_REMOVAL;
-               spin_unlock_irqrestore(&queue->state_lock, flags);
-               nvmet_rdma_queue_disconnect(queue);
-               flush_scheduled_work();
+               return 0;
         }

+       port = cm_id->context;
+
+       /*
+        * This is a listener cm_id. Make sure that
+        * future remove_port won't invoke a double
+        * cm_id destroy. use atomic xchg to make sure
+        * we don't compete with remove_port.
+        */
+       if (xchg(&port->priv, NULL) != cm_id)
+               return 0;
+
         /*
          * We need to return 1 so that the core will destroy
          * it's own ID.  What a great API design..
@@ -1519,9 +1510,51 @@ static struct nvmet_fabrics_ops nvmet_rdma_ops = {
         .delete_ctrl            = nvmet_rdma_delete_ctrl,
  };

+static void nvmet_rdma_add_one(struct ib_device *ib_device)
+{
+}
+
+static void nvmet_rdma_remove_one(struct ib_device *ib_device, void 
*client_data)
+{
+       struct nvmet_rdma_queue *queue;
+
+       /* Device is being removed, delete all queues using this device */
+       mutex_lock(&nvmet_rdma_queue_mutex);
+       list_for_each_entry(queue, &nvmet_rdma_queue_list, queue_list) {
+               if (queue->dev->device != ib_device)
+                       continue;
+
+               pr_info("Removing queue %d\n", queue->idx);
+               __nvmet_rdma_queue_disconnect(queue);
+       }
+       mutex_unlock(&nvmet_rdma_queue_mutex);
+
+       flush_scheduled_work();
+}
+
+static struct ib_client nvmet_rdma_ib_client = {
+       .name   = "nvmet_rdma",
+       .add = nvmet_rdma_add_one,
+       .remove = nvmet_rdma_remove_one
+};
+
  static int __init nvmet_rdma_init(void)
  {
-       return nvmet_register_transport(&nvmet_rdma_ops);
+       int ret;
+
+       ret = ib_register_client(&nvmet_rdma_ib_client);
+       if (ret)
+               return ret;
+
+       ret = nvmet_register_transport(&nvmet_rdma_ops);
+       if (ret)
+               goto err_ib_client;
+
+       return 0;
+
+err_ib_client:
+       ib_unregister_client(&nvmet_rdma_ib_client);
+       return ret;
  }

  static void __exit nvmet_rdma_exit(void)
@@ -1544,6 +1577,7 @@ static void __exit nvmet_rdma_exit(void)
         mutex_unlock(&nvmet_rdma_queue_mutex);

         flush_scheduled_work();
+       ib_unregister_client(&nvmet_rdma_ib_client);
         ida_destroy(&nvmet_rdma_queue_ida);
  }

--

Unfortunately, the patch failed to produce a clear diff :/ sorry...

Re: Deadlock on device removal event for NVMeF target

[Shiraz Saleem]

On Tue, Jun 27, 2017 at 12:37:51AM -0600, Sagi Grimberg wrote:
> > Hi Sagi/Christoph,
> 
> Hi Shiraz,
> 
> Please CC linux-nvme for nvme-rdma related stuff.
OK.

> > I am seeing a deadlock for a device removal event on NVMeF target.
> > 
> > The sequence of events leading to the deadlock are as follows,
> > 
> > 1. i40iw posts IW_CM_EVENT_CLOSE events for all QPs causing the corresponding
> > NVMet RDMA Queues to disconnect and schedule release of any pending work on WQ
> > 2. i40iw triggers device removal
> > 	ib_unregister_device
> > 	[..]
> > 	cma_remove_id_dev (takes a handler lock before calling the event handler)
> > 	nvmet_rdma_cm_handler
> > 	nvmet_rdma_device_removal (queue->state = NVMET_RDMA_Q_DISCONNECTING due to 1.)
> > 	flush_scheduled_work (blocks till all scheduled work is drained from WQ)
> > 	nvmet_rdma_release_queue_work (queue->state = NVMET_RDMA_Q_DISCONNECTING)
> > 	rdma_destroy_id (waits on the same handler lock as cma_remove_id_dev causing the deadlock)
> >       
> > So this problem can occur when there is a device removal event while the queue is in
> > disconnect state with the some oustanding work that hasnt been drained from the WQ at the
> > time flush_scheduled_work is called.
> 
> This indeed looks like a bug (thanks for reporting!). We indeed don't
> have sufficient information on where the queue release procedure is by
> only looking at the queue state, we can't tell if rdma_destroy_id was
> invoked and we can deadlock with rdma_destroy_id.
> 

 
> How about the (untested) alternative below:
> --
> [PATCH] nvmet-rdma: register ib_client to not deadlock in device
>   removal
> 
> We can deadlock in case we got to a device removal
> event on a queue which is already in the process of
> destroying the cm_id is this is blocking until all
> events on this cm_id will drain. On the other hand
> we cannot guarantee that rdma_destroy_id was invoked
> as we only have indication that the queue disconnect
> flow has been queued (the queue state is updated before
> the realease work has been queued).
> 
> So, we leave all the queue removal to a separate ib_client
> to avoid this deadlock as ib_client device removal is in
> a different context than the cm_id itself.
> 
> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---

Yes. This patch fixes the problem I am seeing.

Shiraz
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deadlock on device removal event for NVMeF target

[Shiraz Saleem]

On Tue, Jun 27, 2017@12:37:51AM -0600, Sagi Grimberg wrote:
> > Hi Sagi/Christoph,
> 
> Hi Shiraz,
> 
> Please CC linux-nvme for nvme-rdma related stuff.
OK.

> > I am seeing a deadlock for a device removal event on NVMeF target.
> > 
> > The sequence of events leading to the deadlock are as follows,
> > 
> > 1. i40iw posts IW_CM_EVENT_CLOSE events for all QPs causing the corresponding
> > NVMet RDMA Queues to disconnect and schedule release of any pending work on WQ
> > 2. i40iw triggers device removal
> > 	ib_unregister_device
> > 	[..]
> > 	cma_remove_id_dev (takes a handler lock before calling the event handler)
> > 	nvmet_rdma_cm_handler
> > 	nvmet_rdma_device_removal (queue->state = NVMET_RDMA_Q_DISCONNECTING due to 1.)
> > 	flush_scheduled_work (blocks till all scheduled work is drained from WQ)
> > 	nvmet_rdma_release_queue_work (queue->state = NVMET_RDMA_Q_DISCONNECTING)
> > 	rdma_destroy_id (waits on the same handler lock as cma_remove_id_dev causing the deadlock)
> >       
> > So this problem can occur when there is a device removal event while the queue is in
> > disconnect state with the some oustanding work that hasnt been drained from the WQ at the
> > time flush_scheduled_work is called.
> 
> This indeed looks like a bug (thanks for reporting!). We indeed don't
> have sufficient information on where the queue release procedure is by
> only looking at the queue state, we can't tell if rdma_destroy_id was
> invoked and we can deadlock with rdma_destroy_id.
> 

 
> How about the (untested) alternative below:
> --
> [PATCH] nvmet-rdma: register ib_client to not deadlock in device
>   removal
> 
> We can deadlock in case we got to a device removal
> event on a queue which is already in the process of
> destroying the cm_id is this is blocking until all
> events on this cm_id will drain. On the other hand
> we cannot guarantee that rdma_destroy_id was invoked
> as we only have indication that the queue disconnect
> flow has been queued (the queue state is updated before
> the realease work has been queued).
> 
> So, we leave all the queue removal to a separate ib_client
> to avoid this deadlock as ib_client device removal is in
> a different context than the cm_id itself.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---

Yes. This patch fixes the problem I am seeing.

Shiraz
 

Re: Deadlock on device removal event for NVMeF target

[Sagi Grimberg]

>> How about the (untested) alternative below:
>> --
>> [PATCH] nvmet-rdma: register ib_client to not deadlock in device
>>    removal
>>
>> We can deadlock in case we got to a device removal
>> event on a queue which is already in the process of
>> destroying the cm_id is this is blocking until all
>> events on this cm_id will drain. On the other hand
>> we cannot guarantee that rdma_destroy_id was invoked
>> as we only have indication that the queue disconnect
>> flow has been queued (the queue state is updated before
>> the realease work has been queued).
>>
>> So, we leave all the queue removal to a separate ib_client
>> to avoid this deadlock as ib_client device removal is in
>> a different context than the cm_id itself.
>>
>> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
>> ---
> 
> Yes. This patch fixes the problem I am seeing.

Awsome,

Adding your Tested-by tag.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deadlock on device removal event for NVMeF target

[Sagi Grimberg]

>> How about the (untested) alternative below:
>> --
>> [PATCH] nvmet-rdma: register ib_client to not deadlock in device
>>    removal
>>
>> We can deadlock in case we got to a device removal
>> event on a queue which is already in the process of
>> destroying the cm_id is this is blocking until all
>> events on this cm_id will drain. On the other hand
>> we cannot guarantee that rdma_destroy_id was invoked
>> as we only have indication that the queue disconnect
>> flow has been queued (the queue state is updated before
>> the realease work has been queued).
>>
>> So, we leave all the queue removal to a separate ib_client
>> to avoid this deadlock as ib_client device removal is in
>> a different context than the cm_id itself.
>>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> ---
> 
> Yes. This patch fixes the problem I am seeing.

Awsome,

Adding your Tested-by tag.

Thanks!

Re: Deadlock on device removal event for NVMeF target

[Robert LeBlanc]

Could something like this be causing the D state problem I was seeing
in iSER almost a year ago? I tried writing a patch for iSER based on
this, but it didn't help. Either the bug is not being triggered in
device removal, or I didn't line up the statuses correctly. But it
seems that things are getting stuck in the work queue and some sort of
deadlock is happening so I was hopeful that something similar may be
in iSER.

Thanks,
Robert
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904  C70E E654 3BB2 FA62 B9F1


On Wed, Jun 28, 2017 at 12:50 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
>
>>> How about the (untested) alternative below:
>>> --
>>> [PATCH] nvmet-rdma: register ib_client to not deadlock in device
>>>    removal
>>>
>>> We can deadlock in case we got to a device removal
>>> event on a queue which is already in the process of
>>> destroying the cm_id is this is blocking until all
>>> events on this cm_id will drain. On the other hand
>>> we cannot guarantee that rdma_destroy_id was invoked
>>> as we only have indication that the queue disconnect
>>> flow has been queued (the queue state is updated before
>>> the realease work has been queued).
>>>
>>> So, we leave all the queue removal to a separate ib_client
>>> to avoid this deadlock as ib_client device removal is in
>>> a different context than the cm_id itself.
>>>
>>> Signed-off-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
>>> ---
>>
>>
>> Yes. This patch fixes the problem I am seeing.
>
>
> Awsome,
>
> Adding your Tested-by tag.
>
> Thanks!
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deadlock on device removal event for NVMeF target

[Robert LeBlanc]

Could something like this be causing the D state problem I was seeing
in iSER almost a year ago? I tried writing a patch for iSER based on
this, but it didn't help. Either the bug is not being triggered in
device removal, or I didn't line up the statuses correctly. But it
seems that things are getting stuck in the work queue and some sort of
deadlock is happening so I was hopeful that something similar may be
in iSER.

Thanks,
Robert
----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904  C70E E654 3BB2 FA62 B9F1


On Wed, Jun 28, 2017@12:50 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
>
>>> How about the (untested) alternative below:
>>> --
>>> [PATCH] nvmet-rdma: register ib_client to not deadlock in device
>>>    removal
>>>
>>> We can deadlock in case we got to a device removal
>>> event on a queue which is already in the process of
>>> destroying the cm_id is this is blocking until all
>>> events on this cm_id will drain. On the other hand
>>> we cannot guarantee that rdma_destroy_id was invoked
>>> as we only have indication that the queue disconnect
>>> flow has been queued (the queue state is updated before
>>> the realease work has been queued).
>>>
>>> So, we leave all the queue removal to a separate ib_client
>>> to avoid this deadlock as ib_client device removal is in
>>> a different context than the cm_id itself.
>>>
>>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>>> ---
>>
>>
>> Yes. This patch fixes the problem I am seeing.
>
>
> Awsome,
>
> Adding your Tested-by tag.
>
> Thanks!
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Deadlock on device removal event for NVMeF target

[Sagi Grimberg]

Hey Robert,

> Could something like this be causing the D state problem I was seeing
> in iSER almost a year ago?

No, that is a bug in the mlx5 device as far as I'm concerned (although I
couldn't prove it). I've tried to track it down but without access to
the FW tools I can't understand what is going on. I've seen this same
phenomenon with nvmet-rdma before as well.

It looks like when we perform QP draining in the presence of rdma
operations it may not complete, meaning that the zero-length rdma write
never generates a completion. Maybe it has something to do with the qp
moving to error state when some rdma operations have not completed.

> I tried writing a patch for iSER based on
> this, but it didn't help. Either the bug is not being triggered in
> device removal,

It's 100% not related to device removal.

> or I didn't line up the statuses correctly. But it
> seems that things are getting stuck in the work queue and some sort of
> deadlock is happening so I was hopeful that something similar may be
> in iSER.

The hang is the ULP code waiting for QP drain.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deadlock on device removal event for NVMeF target

[Sagi Grimberg]

Hey Robert,

> Could something like this be causing the D state problem I was seeing
> in iSER almost a year ago?

No, that is a bug in the mlx5 device as far as I'm concerned (although I
couldn't prove it). I've tried to track it down but without access to
the FW tools I can't understand what is going on. I've seen this same
phenomenon with nvmet-rdma before as well.

It looks like when we perform QP draining in the presence of rdma
operations it may not complete, meaning that the zero-length rdma write
never generates a completion. Maybe it has something to do with the qp
moving to error state when some rdma operations have not completed.

> I tried writing a patch for iSER based on
> this, but it didn't help. Either the bug is not being triggered in
> device removal,

It's 100% not related to device removal.

> or I didn't line up the statuses correctly. But it
> seems that things are getting stuck in the work queue and some sort of
> deadlock is happening so I was hopeful that something similar may be
> in iSER.

The hang is the ULP code waiting for QP drain.

Re: Deadlock on device removal event for NVMeF target

[Robert LeBlanc]

Sagi,

Thanks for the update.

On Thu, Jun 29, 2017 at 8:32 AM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
> Hey Robert,
>
>> Could something like this be causing the D state problem I was seeing
>> in iSER almost a year ago?
>
>
> No, that is a bug in the mlx5 device as far as I'm concerned (although I
> couldn't prove it). I've tried to track it down but without access to
> the FW tools I can't understand what is going on. I've seen this same
> phenomenon with nvmet-rdma before as well.

Do you know who I could contact about it? I can reproduce the problem
pretty easy with two hosts back to back, so it should be easy for
someone with mlx5 Eth devices to replicate.

> It looks like when we perform QP draining in the presence of rdma
> operations it may not complete, meaning that the zero-length rdma write
> never generates a completion. Maybe it has something to do with the qp
> moving to error state when some rdma operations have not completed.
>
>> I tried writing a patch for iSER based on
>> this, but it didn't help. Either the bug is not being triggered in
>> device removal,
>
>
> It's 100% not related to device removal.
>
>> or I didn't line up the statuses correctly. But it
>> seems that things are getting stuck in the work queue and some sort of
>> deadlock is happening so I was hopeful that something similar may be
>> in iSER.
>
>
> The hang is the ULP code waiting for QP drain.

Yeah, the patches I wrote did nothing to help the problem. The only
thing that kind of worked, was forcing the queue to drop (maybe I was
just ignoring the old queue, I can't remember exactly), but it was
leaving some stale iSCSI session info around. Now that I've read more
of the iSCSI code, I wonder if I should revisit that. I think Bart
said that the sledgehammer approach I took should not be necessary.

----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904  C70E E654 3BB2 FA62 B9F1
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Deadlock on device removal event for NVMeF target

[Robert LeBlanc]

Sagi,

Thanks for the update.

On Thu, Jun 29, 2017@8:32 AM, Sagi Grimberg <sagi@grimberg.me> wrote:
> Hey Robert,
>
>> Could something like this be causing the D state problem I was seeing
>> in iSER almost a year ago?
>
>
> No, that is a bug in the mlx5 device as far as I'm concerned (although I
> couldn't prove it). I've tried to track it down but without access to
> the FW tools I can't understand what is going on. I've seen this same
> phenomenon with nvmet-rdma before as well.

Do you know who I could contact about it? I can reproduce the problem
pretty easy with two hosts back to back, so it should be easy for
someone with mlx5 Eth devices to replicate.

> It looks like when we perform QP draining in the presence of rdma
> operations it may not complete, meaning that the zero-length rdma write
> never generates a completion. Maybe it has something to do with the qp
> moving to error state when some rdma operations have not completed.
>
>> I tried writing a patch for iSER based on
>> this, but it didn't help. Either the bug is not being triggered in
>> device removal,
>
>
> It's 100% not related to device removal.
>
>> or I didn't line up the statuses correctly. But it
>> seems that things are getting stuck in the work queue and some sort of
>> deadlock is happening so I was hopeful that something similar may be
>> in iSER.
>
>
> The hang is the ULP code waiting for QP drain.

Yeah, the patches I wrote did nothing to help the problem. The only
thing that kind of worked, was forcing the queue to drop (maybe I was
just ignoring the old queue, I can't remember exactly), but it was
leaving some stale iSCSI session info around. Now that I've read more
of the iSCSI code, I wonder if I should revisit that. I think Bart
said that the sledgehammer approach I took should not be necessary.

----------------
Robert LeBlanc
PGP Fingerprint 79A2 9CA4 6CC4 45DD A904  C70E E654 3BB2 FA62 B9F1