Skip to contentSkip to conversation

Use rsps_lock in nvmet_rdma_free_rsp

PATCH New [email protected] 2 participants 4 messages Nov 22, 2016 – Nov 25, 2016

Subsystems (2)
NVMe NVMe target
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 Use rsps_lock in nvmet_rdma_free_rsp

Hi Christoph and Sagi,

Without using rsps_lock in nvmet_rdma_free_rsp, free_list is being operated. The following patch, this issue fixed. Is this patch right?

rdma.c | 3 +++

 1 file changed, 3 insertions(+)
drivers/nvme/target/rdma.c 2016-11-22 14:22:27.482438775 +0900 renamed from drivers/nvme/target/rdma.c.org 2016-11-21 06:52:19.000000000 +0900 +3 -0
@@ -423,11 +423,14 @@ static void nvmet_rdma_free_rsps(struct
 {
        struct nvmet_rdma_device *ndev = queue->dev;
        int i, nr_rsps = queue->recv_queue_size * 2;
+       unsigned long flags;
 
        for (i = 0; i < nr_rsps; i++) {
                struct nvmet_rdma_rsp *rsp = &queue->rsps[i];
 
+               spin_lock_irqsave(&queue->rsps_lock, flags);
                list_del(&rsp->free_list);
+               spin_unlock_irqrestore(&queue->rsps_lock, flags);
                nvmet_rdma_free_rsp(ndev, rsp);
        }
        kfree(queue->rsps);

thanks
--
Haruo

#1 lore raw

PATCH 1/1 Use rsps_lock in nvmet_rdma_free_rsp

Hi Tomita,

what are you trying to protect against during device teardown?

author
#2 lore raw

PATCH 1/1 Use rsps_lock in nvmet_rdma_free_rsp

Hi Christoph,

Thank you for your reply.

Quoted 1 line

what are you trying to protect against during device teardown?

I'm trying to protect nvmet_rdma_get_rsp and nvmet_rdma_put_rsp. In these functions, queue->free_rsps is protected by rsps_lock. rsp->free_list is also protected at the same time.

I am investigating whether there is a race issue on the target of nvme. For another function, I also think of the following patch.

drivers/nvme/target/rdma.c 2016-11-25 06:55:53.000000000 +0900 renamed from drivers/nvme/target/rdma.c.orig 2016-11-25 06:51:06.000000000 +0900 +3 -2
@@ -724,6 +724,7 @@ static void nvmet_rdma_recv_done(struct
                container_of(wc->wr_cqe, struct nvmet_rdma_cmd, cqe);
        struct nvmet_rdma_queue *queue = cq->cq_context;
        struct nvmet_rdma_rsp *rsp;
+       unsigned long flags;
 
        if (unlikely(wc->status != IB_WC_SUCCESS)) {
                if (wc->status != IB_WC_WR_FLUSH_ERR) {

@@ -747,10 +748,9 @@ static void nvmet_rdma_recv_done(struct
        rsp->flags = 0;
        rsp->req.cmd = cmd->nvme_cmd;
 
+       spin_lock_irqsave(&queue->state_lock, flags);
        if (unlikely(queue->state != NVMET_RDMA_Q_LIVE)) {
-               unsigned long flags;
 
-               spin_lock_irqsave(&queue->state_lock, flags);
                if (queue->state == NVMET_RDMA_Q_CONNECTING)
                        list_add_tail(&rsp->wait_list, &queue->rsp_wait_list);
                else

@@ -758,6 +758,7 @@ static void nvmet_rdma_recv_done(struct
                spin_unlock_irqrestore(&queue->state_lock, flags);
                return;
        }
+       spin_unlock_irqrestore(&queue->state_lock, flags);
 
        nvmet_rdma_handle_command(queue, rsp);
 }

--
Haruo

#3 lore raw

PATCH 1/1 Use rsps_lock in nvmet_rdma_free_rsp

Quoted 9 lines · On Fri, Nov 25, 2016@05:00:26AM +0000, [email protected] wrote:

Hi Christoph,

Thank you for your reply.

Quoted 1 line

what are you trying to protect against during device teardown?

I'm trying to protect nvmet_rdma_get_rsp and nvmet_rdma_put_rsp. In these functions, queue->free_rsps is protected by rsps_lock. rsp->free_list is also protected at the same time.

If we can invoke these functions concurrently with nvmet_rdma_free_rsp we have sever life time rule error. Under what circumstances do you see such concurrency?

Quoted 2 lines

I am investigating whether there is a race issue on the target of nvme. For another function, I also think of the following patch.

What do you try to protect against with this patch?