svcrdma: Fix race between svc_rdma_recvfrom thread and the dto_tasklet

PATCH In review Tom Tucker 2 participants 3 messages Aug 13, 2008

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Tom Tucker

author

2008-08-13 16:05 UTC #0 lore raw

PATCH 1/1 svcrdma: Fix race between svc_rdma_recvfrom thread and the dto_tasklet

RDMA_READ completions are kept on a separate queue from the general I/O request queue. Since a separate lock is used to protect the RDMA_READ completion queue, a race exists between the dto_tasklet and the svc_rdma_recvfrom thread where the dto_tasklet sets the XPT_DATA bit and adds I/O to the read-completion queue. Concurrently, the recvfrom thread checks the generic queue, finds it empty and resets the XPT_DATA bit. A subsequent svc_xprt_enqueue will fail to enqueue the transport for I/O and cause the transport to "stall".

The fix is to protect both lists with the same lock and set the XPT_DATA bit with this lock held.

Signed-off-by Tom Tucker <[email protected]>

---

include/linux/sunrpc/svc_rdma.h          |    1 -
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    8 ++++----
net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 ++---
3 files changed, 6 insertions(+), 8 deletions(-)

include/linux/sunrpc/svc_rdma.h +0 -0

index ef2e3a2..dc05b54 100644
@@ -143,7 +143,6 @@ struct svcxprt_rdma {

	unsigned long	     sc_flags;
	struct list_head     sc_dto_q;		/* DTO tasklet I/O pending Q */
	struct list_head     sc_read_complete_q;
-	spinlock_t           sc_read_complete_lock;
	struct work_struct   sc_work;
};

/* sc_flags */

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +0 -0

index b4b17f4..74de31a 100644
@@ -443,18 +443,18 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)

	dprintk("svcrdma: rqstp=%p\n", rqstp);

- spin_lock_bh(&rdma_xprt->sc_read_complete_lock);

+	spin_lock_bh(&rdma_xprt->sc_rq_dto_lock);
	if (!list_empty(&rdma_xprt->sc_read_complete_q)) {
		ctxt = list_entry(rdma_xprt->sc_read_complete_q.next,
				  struct svc_rdma_op_ctxt,
				  dto_q);
		list_del_init(&ctxt->dto_q);
	}
-	spin_unlock_bh(&rdma_xprt->sc_read_complete_lock);
-	if (ctxt)
+	if (ctxt) {
+		spin_unlock_bh(&rdma_xprt->sc_rq_dto_lock);
		return rdma_read_complete(rqstp, ctxt);

+ }

-	spin_lock_bh(&rdma_xprt->sc_rq_dto_lock);
	if (!list_empty(&rdma_xprt->sc_rq_dto_q)) {
		ctxt = list_entry(rdma_xprt->sc_rq_dto_q.next,
				  struct svc_rdma_op_ctxt,

net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -0

index 19ddc38..900cb69 100644
@@ -359,11 +359,11 @@ static void sq_cq_reap(struct svcxprt_rdma *xprt)

			if (test_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags)) {
				struct svc_rdma_op_ctxt *read_hdr = ctxt->read_hdr;
				BUG_ON(!read_hdr);
+				spin_lock_bh(&xprt->sc_rq_dto_lock);
				set_bit(XPT_DATA, &xprt->sc_xprt.xpt_flags);
-				spin_lock_bh(&xprt->sc_read_complete_lock);
				list_add_tail(&read_hdr->dto_q,
					      &xprt->sc_read_complete_q);
-				spin_unlock_bh(&xprt->sc_read_complete_lock);
+				spin_unlock_bh(&xprt->sc_rq_dto_lock);
				svc_xprt_enqueue(&xprt->sc_xprt);
			}
			svc_rdma_put_context(ctxt, 0);
@@ -428,7 +428,6 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
	init_waitqueue_head(&cma_xprt->sc_send_wait);

	spin_lock_init(&cma_xprt->sc_lock);
-	spin_lock_init(&cma_xprt->sc_read_complete_lock);
	spin_lock_init(&cma_xprt->sc_rq_dto_lock);

	cma_xprt->sc_ord = svcrdma_ord;

J. Bruce Fields

2008-08-13 21:10 UTC #1 lore raw

Quoted 10 lines · Tom Tucker

RDMA_READ completions are kept on a separate queue from the general I/O request queue. Since a separate lock is used to protect the RDMA_READ completion queue, a race exists between the dto_tasklet and the svc_rdma_recvfrom thread where the dto_tasklet sets the XPT_DATA bit and adds I/O to the read-completion queue. Concurrently, the recvfrom thread checks the generic queue, finds it empty and resets the XPT_DATA bit. A subsequent svc_xprt_enqueue will fail to enqueue the transport for I/O and cause the transport to "stall".

Quoted 4 lines

The fix is to protect both lists with the same lock and set the XPT_DATA bit with this lock held.
Signed-off-by Tom Tucker <[email protected]>

OK. As a bugfix, would you like this sent in now for 2.6.27?

--b.

Quoted 73 lines

---

include/linux/sunrpc/svc_rdma.h          |    1 -
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    8 ++++----
net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 ++---
3 files changed, 6 insertions(+), 8 deletions(-)

include/linux/sunrpc/svc_rdma.h +0 -1

index ef2e3a2..dc05b54 100644
@@ -143,7 +143,6 @@ struct svcxprt_rdma {
 	unsigned long	     sc_flags;
 	struct list_head     sc_dto_q;		/* DTO tasklet I/O pending Q */
 	struct list_head     sc_read_complete_q;
-	spinlock_t           sc_read_complete_lock;
 	struct work_struct   sc_work;
 };
 /* sc_flags */

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4 -4

index b4b17f4..74de31a 100644
@@ -443,18 +443,18 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 
 	dprintk("svcrdma: rqstp=%p\n", rqstp);
 
-	spin_lock_bh(&rdma_xprt->sc_read_complete_lock);
+	spin_lock_bh(&rdma_xprt->sc_rq_dto_lock);
 	if (!list_empty(&rdma_xprt->sc_read_complete_q)) {
 		ctxt = list_entry(rdma_xprt->sc_read_complete_q.next,
 				  struct svc_rdma_op_ctxt,
 				  dto_q);
 		list_del_init(&ctxt->dto_q);
 	}
-	spin_unlock_bh(&rdma_xprt->sc_read_complete_lock);
-	if (ctxt)
+	if (ctxt) {
+		spin_unlock_bh(&rdma_xprt->sc_rq_dto_lock);
 		return rdma_read_complete(rqstp, ctxt);
+	}
 
-	spin_lock_bh(&rdma_xprt->sc_rq_dto_lock);
 	if (!list_empty(&rdma_xprt->sc_rq_dto_q)) {
 		ctxt = list_entry(rdma_xprt->sc_rq_dto_q.next,
 				  struct svc_rdma_op_ctxt,

net/sunrpc/xprtrdma/svc_rdma_transport.c +2 -3

index 19ddc38..900cb69 100644
@@ -359,11 +359,11 @@ static void sq_cq_reap(struct svcxprt_rdma *xprt)
 			if (test_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags)) {
 				struct svc_rdma_op_ctxt *read_hdr = ctxt->read_hdr;
 				BUG_ON(!read_hdr);
+				spin_lock_bh(&xprt->sc_rq_dto_lock);
 				set_bit(XPT_DATA, &xprt->sc_xprt.xpt_flags);
-				spin_lock_bh(&xprt->sc_read_complete_lock);
 				list_add_tail(&read_hdr->dto_q,
 					      &xprt->sc_read_complete_q);
-				spin_unlock_bh(&xprt->sc_read_complete_lock);
+				spin_unlock_bh(&xprt->sc_rq_dto_lock);
 				svc_xprt_enqueue(&xprt->sc_xprt);
 			}
 			svc_rdma_put_context(ctxt, 0);

@@ -428,7 +428,6 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
 	init_waitqueue_head(&cma_xprt->sc_send_wait);
 
 	spin_lock_init(&cma_xprt->sc_lock);
-	spin_lock_init(&cma_xprt->sc_read_complete_lock);
 	spin_lock_init(&cma_xprt->sc_rq_dto_lock);
 
 	cma_xprt->sc_ord = svcrdma_ord;

Tom Tucker

author

2008-08-13 21:31 UTC #2 lore raw

Quoted 10 lines · J. Bruce Fields

Quoted 9 lines · Tom Tucker
RDMA_READ completions are kept on a separate queue from the general I/O request queue. Since a separate lock is used to protect the RDMA_READ completion queue, a race exists between the dto_tasklet and the svc_rdma_recvfrom thread where the dto_tasklet sets the XPT_DATA bit and adds I/O to the read-completion queue. Concurrently, the recvfrom thread checks the generic queue, finds it empty and resets the XPT_DATA bit. A subsequent svc_xprt_enqueue will fail to enqueue the transport for I/O and cause the transport to "stall".

Quoted 7 lines

Quoted 4 lines
The fix is to protect both lists with the same lock and set the XPT_DATA bit with this lock held.
Signed-off-by Tom Tucker <[email protected]>
OK. As a bugfix, would you like this sent in now for 2.6.27?

Yes I would if that's possible. I put it before all the other patches so it would be independent.

Thanks,
Tom

Quoted 78 lines

--b.

Quoted 65 lines

---

include/linux/sunrpc/svc_rdma.h          |    1 -
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |    8 ++++----
net/sunrpc/xprtrdma/svc_rdma_transport.c |    5 ++---
3 files changed, 6 insertions(+), 8 deletions(-)

include/linux/sunrpc/svc_rdma.h +0 -1

index ef2e3a2..dc05b54 100644
@@ -143,7 +143,6 @@ struct svcxprt_rdma {
 	unsigned long	     sc_flags;
 	struct list_head     sc_dto_q;		/* DTO tasklet I/O pending Q */
 	struct list_head     sc_read_complete_q;
-	spinlock_t           sc_read_complete_lock;
 	struct work_struct   sc_work;
 };
 /* sc_flags */

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4 -4

index b4b17f4..74de31a 100644
@@ -443,18 +443,18 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 
 	dprintk("svcrdma: rqstp=%p\n", rqstp);
 
-	spin_lock_bh(&rdma_xprt->sc_read_complete_lock);
+	spin_lock_bh(&rdma_xprt->sc_rq_dto_lock);
 	if (!list_empty(&rdma_xprt->sc_read_complete_q)) {
 		ctxt = list_entry(rdma_xprt->sc_read_complete_q.next,
 				  struct svc_rdma_op_ctxt,
 				  dto_q);
 		list_del_init(&ctxt->dto_q);
 	}
-	spin_unlock_bh(&rdma_xprt->sc_read_complete_lock);
-	if (ctxt)
+	if (ctxt) {
+		spin_unlock_bh(&rdma_xprt->sc_rq_dto_lock);
 		return rdma_read_complete(rqstp, ctxt);
+	}
 
-	spin_lock_bh(&rdma_xprt->sc_rq_dto_lock);
 	if (!list_empty(&rdma_xprt->sc_rq_dto_q)) {
 		ctxt = list_entry(rdma_xprt->sc_rq_dto_q.next,
 				  struct svc_rdma_op_ctxt,

net/sunrpc/xprtrdma/svc_rdma_transport.c +2 -2

index 19ddc38..900cb69 100644
@@ -359,11 +359,11 @@ static void sq_cq_reap(struct svcxprt_rdma *xprt)
 			if (test_bit(RDMACTXT_F_LAST_CTXT, &ctxt->flags)) {
 				struct svc_rdma_op_ctxt *read_hdr = ctxt->read_hdr;
 				BUG_ON(!read_hdr);
+				spin_lock_bh(&xprt->sc_rq_dto_lock);
 				set_bit(XPT_DATA, &xprt->sc_xprt.xpt_flags);
-				spin_lock_bh(&xprt->sc_read_complete_lock);
 				list_add_tail(&read_hdr->dto_q,
 					      &xprt->sc_read_complete_q);
-				spin_unlock_bh(&xprt->sc_read_complete_lock);
+				spin_unlock_bh(&xprt->sc_rq_dto_lock);
 				svc_xprt_enqueue(&xprt->sc_xprt);
 			}
 			svc_rdma_put_context(ctxt, 0);

net/sunrpc/xprtrdma/svc_rdma_transport.c static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv, · line 433

Quoted 7 lines

net/sunrpc/xprtrdma/svc_rdma_transport.c +0 -1

@@ -428,7 +428,6 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
 	init_waitqueue_head(&cma_xprt->sc_send_wait);
 
 	spin_lock_init(&cma_xprt->sc_lock);
-	spin_lock_init(&cma_xprt->sc_read_complete_lock);
 	spin_lock_init(&cma_xprt->sc_rq_dto_lock);
 
 	cma_xprt->sc_ord = svcrdma_ord;

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html