nvme_rdma - leaves provider resources allocated

DISCUSSION Steve Wise 2 participants 6 messages Aug 23, 2016 – Aug 25, 2016 NVMe

Share & tools

Steve Wise

author

2016-08-23 16:58 UTC #0 lore raw

Assume an nvme_rdma host has one attached controller in RECONNECTING state, and that controller has failed to reconnect at least once and thus is in the delay_schedule time before retrying the connection. At that moment, there are no cm_ids allocated for that controller because the admin queue and the io queues have been freed. So nvme_rdma cannot get a DEVICE_REMOVAL from the rdma_cm. This means if the underlying provider module is removed, it will be removed with resources still allocated by nvme_rdma. For iw_cxgb4, this causes a BUG_ON() in gen_pool_destroy() because MRs are still allocated for the controller.

Thoughts on how to fix this?

Thanks,

Steve.

Sagi Grimberg

maintainer

2016-08-24 09:31 UTC #1 lore raw

Quoted 11 lines

Assume an nvme_rdma host has one attached controller in RECONNECTING state, and that controller has failed to reconnect at least once and thus is in the delay_schedule time before retrying the connection. At that moment, there are no cm_ids allocated for that controller because the admin queue and the io queues have been freed. So nvme_rdma cannot get a DEVICE_REMOVAL from the rdma_cm. This means if the underlying provider module is removed, it will be removed with resources still allocated by nvme_rdma. For iw_cxgb4, this causes a BUG_ON() in gen_pool_destroy() because MRs are still allocated for the controller.

Quoted 1 line

Thoughts on how to fix this?

Hey Steve,

I think it's time to go back to your client register proposal.

I can't think of any way to get it right at the moment...

Maybe if we can make it only do something meaningful in remove_one() to handle device removal we can get away with it...

Steve Wise

author

2016-08-24 14:09 UTC #2 lore raw

Quoted 5 lines

Quoted 2 lines
Assume an nvme_rdma host has one attached controller in RECONNECTING state,
and
Quoted 2 lines
that controller has failed to reconnect at least once and thus is in the delay_schedule time before retrying the connection. At that moment, there

are

Quoted 4 lines

Quoted 2 lines
no cm_ids allocated for that controller because the admin queue and the io queues have been freed. So nvme_rdma cannot get a DEVICE_REMOVAL from
the
Quoted 1 line
rdma_cm. This means if the underlying provider module is removed, it will

Quoted 1 line

Quoted 1 line
removed with resources still allocated by nvme_rdma. For iw_cxgb4, this

causes

Quoted 5 lines

Quoted 4 lines
a BUG_ON() in gen_pool_destroy() because MRs are still allocated for the controller.
Thoughts on how to fix this?

Quoted 8 lines

Hey Steve,
I think it's time to go back to your client register proposal.
I can't think of any way to get it right at the moment...
Maybe if we can make it only do something meaningful in remove_one() to handle device removal we can get away with it...

Hey Sagi,

I'm finalizing a WIP series that provides a different approach. (we can certainly reconsider my ib_client patch too). But my WIP adds the concept of an "unplug" cm_id for each nvme_rdma_ctrl controller. When the controller is first created and the admin qp is connected to the target, the unplug_cm_id is created and address resolution is done on it to bind it to the same device that the
admin QP is bound to. This unplug_cm_id remains across any/all kato recovery
and thus will always be available for DEVICE_REMOVAL events. This simplifies the unplug handler because the cm_id isn't associated with any of the IO queues nor the admin queue.

I also found another bug: if the reconnect worker times out waiting for rdma
connection setup on an IO or admin QP, a QP is leaked. I'm looking into this
as well.

Do you have any thoughts on the controller reference around deletion issue I posted?

http://lists.infradead.org/pipermail/linux-nvme/2016-August/005919.html

Thanks!

Steve.

Sagi Grimberg

maintainer

2016-08-25 21:52 UTC #3 lore raw

Quoted 12 lines

Hey Sagi,
I'm finalizing a WIP series that provides a different approach. (we can certainly reconsider my ib_client patch too). But my WIP adds the concept of an "unplug" cm_id for each nvme_rdma_ctrl controller. When the controller is first created and the admin qp is connected to the target, the unplug_cm_id is created and address resolution is done on it to bind it to the same device that the
admin QP is bound to. This unplug_cm_id remains across any/all kato recovery
and thus will always be available for DEVICE_REMOVAL events. This simplifies the unplug handler because the cm_id isn't associated with any of the IO queues nor the admin queue.

OK, let's wait for the patches...

Quoted 3 lines

I also found another bug: if the reconnect worker times out waiting for rdma
connection setup on an IO or admin QP, a QP is leaked. I'm looking into this
as well.

Hmm, I think you're right. If we passed address resolution but failed route (or got a general CONNECT/UNREACHABLE errors) we won't free the queue...

I think this should fix this:
--

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

index ab545fb347a0..452727c5ea13 100644
@@ -1382,10 +1382,12 @@ static int nvme_rdma_cm_handler(struct

rdma_cm_id *cm_id,

         case RDMA_CM_EVENT_REJECTED:
                 cm_error = nvme_rdma_conn_rejected(queue, ev);
                 break;

- case RDMA_CM_EVENT_ADDR_ERROR:
case RDMA_CM_EVENT_ROUTE_ERROR:
case RDMA_CM_EVENT_CONNECT_ERROR:
case RDMA_CM_EVENT_UNREACHABLE:
+ nvme_rdma_destroy_queue_ib(queue);
+ case RDMA_CM_EVENT_ADDR_ERROR:
+ /* FALLTHRU */

                 dev_dbg(queue->ctrl->ctrl.device,
                         "CM error event %d\n", ev->event);
                 cm_error = -ECONNRESET;

Quoted 5 lines

Do you have any thoughts on the controller reference around deletion issue I posted?
http://lists.infradead.org/pipermail/linux-nvme/2016-August/005919.html

Looks fine, but you need to use kref_get_unless_zero.

Steve Wise

author

2016-08-25 22:03 UTC #4 lore raw

Quoted 5 lines

Quoted 5 lines
Hey Sagi,
I'm finalizing a WIP series that provides a different approach. (we can certainly reconsider my ib_client patch too). But my WIP adds the concept

of an

Quoted 1 line

Quoted 1 line
"unplug" cm_id for each nvme_rdma_ctrl controller. When the controller is

first

Quoted 1 line

Quoted 1 line
created and the admin qp is connected to the target, the unplug_cm_id is

created

Quoted 2 lines

Quoted 2 lines
and address resolution is done on it to bind it to the same device that the
admin QP is bound to. This unplug_cm_id remains across any/all kato

recovery

Quoted 1 line

Quoted 1 line
and thus will always be available for DEVICE_REMOVAL events. This

simplifies

Quoted 1 line

Quoted 1 line
the unplug handler because the cm_id isn't associated with any of the IO

queues

Quoted 3 lines

Quoted 1 line
nor the admin queue.
OK, let's wait for the patches...

I'll send them tomorrow. They're still Work In Progress (WIP) though. There is at least one other bug I'm chasing.

Quoted 2 lines

Quoted 2 lines
I also found another bug: if the reconnect worker times out waiting for

rdma

Quoted 1 line

Quoted 1 line
connection setup on an IO or admin QP, a QP is leaked. I'm looking into

this

Quoted 27 lines

Quoted 1 line

as well.

Hmm, I think you're right. If we passed address resolution but failed route (or got a general CONNECT/UNREACHABLE errors) we won't free the queue...

I think this should fix this:
--

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

index ab545fb347a0..452727c5ea13 100644
@@ -1382,10 +1382,12 @@ static int nvme_rdma_cm_handler(struct
 rdma_cm_id *cm_id,
         case RDMA_CM_EVENT_REJECTED:
                 cm_error = nvme_rdma_conn_rejected(queue, ev);
                 break;
-       case RDMA_CM_EVENT_ADDR_ERROR:
         case RDMA_CM_EVENT_ROUTE_ERROR:
         case RDMA_CM_EVENT_CONNECT_ERROR:
         case RDMA_CM_EVENT_UNREACHABLE:
+               nvme_rdma_destroy_queue_ib(queue);
+       case RDMA_CM_EVENT_ADDR_ERROR:
+               /* FALLTHRU */
                 dev_dbg(queue->ctrl->ctrl.device,
                         "CM error event %d\n", ev->event);

                 cm_error = -ECONNRESET;

Quoted 1 line

--

This won't quite do it. The reconnect thread can timeout and give up before any event arrives, and that path also needs to destroy the queue resources if it was allocated. This is my current patch. I appreciate any comments. (but I'll resend this formally tomorrow with other changes)...

---

nvme-rdma: destroy nvme queue rdma resources on connect failure

From: Steve Wise <[email protected]>

After address resolution, the nvme_rdma_queue rdma resources are allocated. If rdma route resolution or the connect fails, or the controller reconnect times out and gives up, then the rdma resources need to be freed. Otherwise, rdma resources are leaked.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

---

 drivers/nvme/host/rdma.c |    5 +++++
 1 file changed, 5 insertions(+)

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

index 6198eaa..ea271df 100644
@@ -87,6 +87,7 @@ struct nvme_rdma_request {
 
 enum nvme_rdma_queue_flags {
        NVME_RDMA_Q_CONNECTED = (1 << 0),
+       NVME_RDMA_IB_QUEUE_ALLOCATED = (1 << 1),
 };
 
 struct nvme_rdma_queue {

@@ -488,6 +489,7 @@ static void nvme_rdma_destroy_queue_ib(struct

nvme_rdma_queue *queue)

        struct nvme_rdma_device *dev = queue->device;
        struct ib_device *ibdev = dev->dev;

+       clear_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);
        rdma_destroy_qp(queue->cm_id);
        ib_free_cq(queue->ib_cq);

@@ -538,6 +540,7 @@ static int nvme_rdma_create_queue_ib(struct nvme_rdma_queue *queue,

                ret = -ENOMEM;
                goto out_destroy_qp;
        }

+ set_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &queue->flags);

        return 0;

@@ -595,6 +598,8 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,

        return 0;

out_destroy_cm_id:
+ if (test_bit(NVME_RDMA_IB_QUEUE_ALLOCATED, &ctrl->queues[0].flags))

+               nvme_rdma_destroy_queue_ib(queue);
        rdma_destroy_id(queue->cm_id);
        return ret;

}

Sagi Grimberg

maintainer

2016-08-25 22:06 UTC #5 lore raw

Quoted 5 lines

This won't quite do it. The reconnect thread can timeout and give up before any event arrives, and that path also needs to destroy the queue resources if it was allocated. This is my current patch. I appreciate any comments. (but I'll resend this formally tomorrow with other changes)...

Makes sense...

With 2 patches now adding queue flags I think it's time to ramp a proper state machine? Looks like we starting to need it for all the corner cases...