[PATCH] nvmet-rdma: recheck queue state is LIVE in state lock in recv done

[PATCH] nvmet-rdma: recheck queue state is LIVE in state lock in recv done

[Ruozhu Li]

The queue state checking in nvmet_rdma_recv_done is not in queue state
lock.Queue state can transfer to LIVE in cm establish handler between
state checking and state lock here, cause a silent drop of nvme connect
cmd.
Recheck queue state whether in LIVE state in state lock to prevent this
issue.

Signed-off-by: Ruozhu Li <david.li@jaguarmicro.com>
---
 drivers/nvme/target/rdma.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 1afd93026f9b..9f9e51fbfe49 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -1042,12 +1042,16 @@ static void nvmet_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
 		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
-			nvmet_rdma_put_rsp(rsp);
+		if (queue->state != NVMET_RDMA_Q_LIVE) {
+			if (queue->state == NVMET_RDMA_Q_CONNECTING)
+				list_add_tail(&rsp->wait_list, &queue->rsp_wait_list);
+			else
+				nvmet_rdma_put_rsp(rsp);
+
+			spin_unlock_irqrestore(&queue->state_lock, flags);
+			return;
+		}
 		spin_unlock_irqrestore(&queue->state_lock, flags);
-		return;
 	}
 
 	nvmet_rdma_handle_command(queue, rsp);
-- 
2.43.0

Re: [PATCH] nvmet-rdma: recheck queue state is LIVE in state lock in recv done

[Christoph Hellwig]

> +++ b/drivers/nvme/target/rdma.c
> @@ -1042,12 +1042,16 @@ static void nvmet_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
>  		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
> -			nvmet_rdma_put_rsp(rsp);
> +		if (queue->state != NVMET_RDMA_Q_LIVE) {

You'll want a comment based on the commit message that the first live
check is racy. 

> +			if (queue->state == NVMET_RDMA_Q_CONNECTING)
> +				list_add_tail(&rsp->wait_list, &queue->rsp_wait_list);
> +			else
> +				nvmet_rdma_put_rsp(rsp);
> +
> +			spin_unlock_irqrestore(&queue->state_lock, flags);
> +			return;

This gets a bit messy now.  What about a struture like this instead?

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 1afd93026f9b..ae6a123ac63f 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -996,6 +996,23 @@ static void nvmet_rdma_handle_command(struct nvmet_rdma_queue *queue,
 	nvmet_req_complete(&cmd->req, status);
 }
 
+static bool nvmet_rdma_recv_not_live(struct nvmet_rdma_queue *queue,
+		struct nvmet_rdma_rsp *rsp)
+{
+	unsigned long flags;
+	bool ret = true;
+
+	spin_lock_irqsave(&queue->state_lock, flags);
+	if (queue->state == NVMET_RDMA_Q_LIVE)
+		ret = false;
+	else if (queue->state == NVMET_RDMA_Q_CONNECTING)
+		list_add_tail(&rsp->wait_list, &queue->rsp_wait_list);
+	else
+		nvmet_rdma_put_rsp(rsp);
+	spin_unlock_irqrestore(&queue->state_lock, flags);
+	return ret;
+}
+
 static void nvmet_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
 {
 	struct nvmet_rdma_cmd *cmd =
@@ -1038,18 +1055,9 @@ static void nvmet_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
 	rsp->n_rdma = 0;
 	rsp->invalidate_rkey = 0;
 
-	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
-			nvmet_rdma_put_rsp(rsp);
-		spin_unlock_irqrestore(&queue->state_lock, flags);
+	if (unlikely(queue->state != NVMET_RDMA_Q_LIVE) &&
+	    nvmet_rdma_recv_not_live(queue, rsp))
 		return;
-	}
-
 	nvmet_rdma_handle_command(queue, rsp);
 }
 

RE: [PATCH] nvmet-rdma: recheck queue state is LIVE in state lock in recv done

[Ruozhu Li]

On 2025/02/13 6:20 UTC, Christoph Hellwig wrote:
> > +++ b/drivers/nvme/target/rdma.c
> > @@ -1042,12 +1042,16 @@ static void nvmet_rdma_recv_done(struct ib_cq *cq, struct ib_wc *wc)
> >               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
> > -                     nvmet_rdma_put_rsp(rsp);
> > +             if (queue->state != NVMET_RDMA_Q_LIVE) {
>
> You'll want a comment based on the commit message that the first live check is racy.
Okay, I will add a comment in next version.
>
> > +                     if (queue->state == NVMET_RDMA_Q_CONNECTING)
> > +                             list_add_tail(&rsp->wait_list, &queue->rsp_wait_list);
> > +                     else
> > +                             nvmet_rdma_put_rsp(rsp);
> > +
> > +                     spin_unlock_irqrestore(&queue->state_lock, flags);
> > +                     return;
>
> This gets a bit messy now.  What about a struture like this instead?
Looks good to me, I will retest it and send another version soon.

Thanks.