svcrdma: RDMA transport driver close path cleanup

This patchset fixes a number of defects in the close path of the SVCRDMA transport provider. The defects were found by injecting errors on the transport at random intervals while running concurrent iozone tests on IB and iWARP mounts. With this set of patches applied I was able to run the the above tests for several hours without causing a crash or leaking resources. The transport recovers correctly on a client reconnect.

This patchset is based on 2.6 top of tree, however, I think these fixes are good candidates for a 2.6.25 dot release as well. I have a set that is already merged for that release if you would like them. They are available in my git tree on linux-nfs.org in the '2.6.25' branch. Please let me know if you would like me to post them here.

[PATCH 1/17] svcrdma: Simplify receive buffer posting

 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   17 +----------------
 net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   10 ++++++++++
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   19 -------------------
 3 files changed, 11 insertions(+), 35 deletions(-)

[PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

[PATCH 3/17] svcrdma: Fix return value in svc_rdma_send

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH 4/17] svcrdma: Add put of connection ESTABLISHED reference in rdma_cma_handler

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

[PATCH 5/17] svcrdma: Free context on ib_post_recv error

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

[PATCH 6/17] svcrdma: Free context on post_recv error in send_reply

 net/sunrpc/xprtrdma/svc_rdma_sendto.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

[PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

[PATCH 8/17] svcrdma: Return error from rdma_read_xdr so caller knows to free context

 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

[PATCH 9/17] svcrdma: Remove unused READ_DONE context flags bit

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

[PATCH 10/17] svcrdma: Simplify RDMA_READ deferral buffer management

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

[PATCH 11/17] svcrdma: Use standard Linux lists for context cache

 include/linux/sunrpc/svc_rdma.h          |    5 ++-
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   47 ++++++++++++++++-------------
 2 files changed, 29 insertions(+), 23 deletions(-)

[PATCH 12/17] svcrdma: Relaxing locking scope on RQ CQ

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

[PATCH 13/17] svcrdma: Move destroy to kernel thread

 include/linux/sunrpc/svc_rdma.h          |    1 +
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

[PATCH 14/17] svcrdma: Add reference for each SQ/RQ WR

 net/sunrpc/xprtrdma/svc_rdma_transport.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

[PATCH 15/17] svcrdma: Move the QP and cm_id destruction to svc_rdma_free

 net/sunrpc/xprtrdma/svc_rdma_transport.c |   40 +++++++++++++++---------------
 1 files changed, 20 insertions(+), 20 deletions(-)

[PATCH 16/17] svcrdma: Cleanup queued, but unprocessed I/O in svc_rdma_free

 net/sunrpc/xprtrdma/svc_rdma_transport.c |   38 +++++++++++++++++++++++++++--
 1 files changed, 35 insertions(+), 3 deletions(-)

[PATCH 17/17] svcrdma: Use ib verbs version of dma_unmap

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

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

Thanks. I'm a little backlogged--bug me if I haven't gotten back to these by next week.

--b.

Quoted 101 lines · Tom Tucker

This patchset fixes a number of defects in the close path of the SVCRDMA transport provider. The defects were found by injecting errors on the transport at random intervals while running concurrent iozone tests on IB and iWARP mounts. With this set of patches applied I was able to run the the above tests for several hours without causing a crash or leaking resources. The transport recovers correctly on a client reconnect.

This patchset is based on 2.6 top of tree, however, I think these fixes are good candidates for a 2.6.25 dot release as well. I have a set that is already merged for that release if you would like them. They are available in my git tree on linux-nfs.org in the '2.6.25' branch. Please let me know if you would like me to post them here.

[PATCH 1/17] svcrdma: Simplify receive buffer posting

 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c  |   17 +----------------
 net/sunrpc/xprtrdma/svc_rdma_sendto.c    |   10 ++++++++++
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   19 -------------------
 3 files changed, 11 insertions(+), 35 deletions(-)

[PATCH 2/17] svcrdma: Fix race with dto_tasklet in svc_rdma_send

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

[PATCH 3/17] svcrdma: Fix return value in svc_rdma_send

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH 4/17] svcrdma: Add put of connection ESTABLISHED reference in rdma_cma_handler

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

[PATCH 5/17] svcrdma: Free context on ib_post_recv error

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

[PATCH 6/17] svcrdma: Free context on post_recv error in send_reply

 net/sunrpc/xprtrdma/svc_rdma_sendto.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

[PATCH 7/17] svcrdma: Fix error handling during listening endpoint creation

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

[PATCH 8/17] svcrdma: Return error from rdma_read_xdr so caller knows to free context

 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

[PATCH 9/17] svcrdma: Remove unused READ_DONE context flags bit

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

[PATCH 10/17] svcrdma: Simplify RDMA_READ deferral buffer management

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

[PATCH 11/17] svcrdma: Use standard Linux lists for context cache

 include/linux/sunrpc/svc_rdma.h          |    5 ++-
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   47 ++++++++++++++++-------------
 2 files changed, 29 insertions(+), 23 deletions(-)

[PATCH 12/17] svcrdma: Relaxing locking scope on RQ CQ

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

[PATCH 13/17] svcrdma: Move destroy to kernel thread

 include/linux/sunrpc/svc_rdma.h          |    1 +
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

[PATCH 14/17] svcrdma: Add reference for each SQ/RQ WR

 net/sunrpc/xprtrdma/svc_rdma_transport.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

[PATCH 15/17] svcrdma: Move the QP and cm_id destruction to svc_rdma_free

 net/sunrpc/xprtrdma/svc_rdma_transport.c |   40 +++++++++++++++---------------
 1 files changed, 20 insertions(+), 20 deletions(-)

[PATCH 16/17] svcrdma: Cleanup queued, but unprocessed I/O in svc_rdma_free

 net/sunrpc/xprtrdma/svc_rdma_transport.c |   38 +++++++++++++++++++++++++++--
 1 files changed, 35 insertions(+), 3 deletions(-)

Quoted 6 lines

[PATCH 17/17] svcrdma: Use ib verbs version of dma_unmap

 net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

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

--
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

The svcrdma transport provider currently allocates receive buffers to the RQ through the xpo_release_rqst method. This approach is overly complicated since it means that the rqstp rq_xprt_ctxt has to be selectively set based on whether the RPC is going to be processed immediately or deferred. Instead, just post the receive buffer when we are certain that we are replying in the send_reply function.

index af408fc..1e0af2f 100644

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

@@ -910,27 +910,8 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 	return NULL;
 }
 
-/*
- * Post an RQ WQE to the RQ when the rqst is being released. This
- * effectively returns an RQ credit to the client. The rq_xprt_ctxt
- * will be null if the request is deferred due to an RDMA_READ or the
- * transport had no data ready (EAGAIN). Note that an RPC deferred in
- * svc_process will still return the credit, this is because the data
- * is copied and no longer consume a WQE/WC.
- */
 static void svc_rdma_release_rqst(struct svc_rqst *rqstp)
 {
-	int err;
-	struct svcxprt_rdma *rdma =

The svc_rdma_send function will attempt to reap SQ WR to make room for a new request if it finds the SQ full. This function races with the dto_tasklet that also reaps SQ WR. To avoid calling the function unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING flag to serialize access to the sq_cq_reap function.

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

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

index 1e0af2f..14f83a1 100644
@@ -1010,8 +1010,12 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
 		if (xprt->sc_sq_depth == atomic_read(&xprt->sc_sq_count)) {
 			spin_unlock_bh(&xprt->sc_lock);
 			atomic_inc(&rdma_stat_sq_starve);
-			/* See if we can reap some SQ WR */
-			sq_cq_reap(xprt);
+
+			/* See if we can opportunistically reap SQ WR to make room */
+			if (test_and_clear_bit(RDMAXPRT_SQ_PENDING, &xprt->sc_flags)) {
+				ib_req_notify_cq(xprt->sc_sq_cq, IB_CQ_NEXT_COMP);
+				sq_cq_reap(xprt);
+			}
 
 			/* Wait until SQ WR available if SQ still full */
 			wait_event(xprt->sc_send_wait,

Quoted 5 lines · Tom Tucker

The svc_rdma_send function will attempt to reap SQ WR to make room for a new request if it finds the SQ full. This function races with the dto_tasklet that also reaps SQ WR. To avoid calling the function unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING flag to serialize access to the sq_cq_reap function.

OK. I won't pretend to understand much of this, but--would it be worth pulling out the added code here into a helper function, since it now exists in two different places? (Especially if correctness depends on the same thing happening in both the places this bit can be cleared.)

--b.

Quoted 27 lines

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

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

index 1e0af2f..14f83a1 100644
@@ -1010,8 +1010,12 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
 ib_send_wr *wr)
 if (xprt->sc_sq_depth == atomic_read(&xprt->sc_sq_count)) {
 spin_unlock_bh(&xprt->sc_lock);
 atomic_inc(&rdma_stat_sq_starve);
-   /* See if we can reap some SQ WR */
-   sq_cq_reap(xprt);
+
+   /* See if we can opportunistically reap SQ WR to make room */
+   if (test_and_clear_bit(RDMAXPRT_SQ_PENDING, &xprt->sc_flags)) {
+    ib_req_notify_cq(xprt->sc_sq_cq, IB_CQ_NEXT_COMP);
+    sq_cq_reap(xprt);
+   }
 
 /* Wait until SQ WR available if SQ still full */

Quoted 1 line

wait_event(xprt->sc_send_wait,

--
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

Quoted 11 lines · J. Bruce Fields

Quoted 5 lines · Tom Tucker

The svc_rdma_send function will attempt to reap SQ WR to make room for a new request if it finds the SQ full. This function races with the dto_tasklet that also reaps SQ WR. To avoid calling the function unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING flag to serialize access to the sq_cq_reap function.

OK. I won't pretend to understand much of this, but--would it be worth pulling out the added code here into a helper function, since it now exists in two different places? (Especially if correctness depends on the same thing happening in both the places this bit can be cleared.)

Yes. Good suggestions.

BTW, this code is here because the SQ is undersized for big data. Since a single NFS_READ/WRITE can result in an attempt to fetch a large amount of data from the client (2M) and depending on certain HW resources this can result in a lot of WR being posted to the SQ.

That said, there is a change coming in the 2.6.27 time frame that supports what is called Fast NSMR register. This allows the transport to effectively DMA map the entire transfer size (32k -- 2M) all as a single SGE. This will take a lot of pressure off the SQ and effectively make this code unnecessary.

Quoted 17 lines · Tom Tucker

Quoted 11 lines · J. Bruce Fields

Quoted 5 lines · Tom Tucker

The svc_rdma_send function will attempt to reap SQ WR to make room for a new request if it finds the SQ full. This function races with the dto_tasklet that also reaps SQ WR. To avoid calling the function unnecessarily use test_and_clear_bit with the RDMAXPRT_SQ_PENDING flag to serialize access to the sq_cq_reap function.

OK. I won't pretend to understand much of this, but--would it be worth pulling out the added code here into a helper function, since it now exists in two different places? (Especially if correctness depends on the same thing happening in both the places this bit can be cleared.)

Quoted 6 lines

Yes. Good suggestions.

BTW, this code is here because the SQ is undersized for big data. Since a single NFS_READ/WRITE can result in an attempt to fetch a large amount of data from the client (2M) and depending on certain HW resources this can result in a lot of WR being posted to the SQ.

WR is write request, SQ is send queue? (In which case this happens on nfs read operations?) I'm behind....

Quoted 5 lines

That said, there is a change coming in the 2.6.27 time frame that supports what is called Fast NSMR register. This allows the transport to effectively DMA map the entire transfer size (32k -- 2M) all as a single SGE. This will take a lot of pressure off the SQ and effectively make this code unnecessary.

OK!

Ideally we'd have patches for 2.6.27 in linux-next for a little while first, so we should try to have that ready in a month or so, when the -rc's start looking final.

--b.
--
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

Fix the return value on close to -ENOTCONN so caller knows to free context. Also if a thread is waiting for free SQ space, check for close when waking to avoid posting WR to a closing transport.

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

index 14f83a1..8adb2f0 100644
@@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct ib_send_wr *wr)
 	int ret;
 
 	if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
-		return 0;
+		return -ENOTCONN;
 
 	BUG_ON(wr->send_flags != IB_SEND_SIGNALED);
 	BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=

Quoted 3 lines · Tom Tucker

Fix the return value on close to -ENOTCONN so caller knows to free context. Also if a thread is waiting for free SQ space, check for close when waking to avoid posting WR to a closing transport.

A random related question: svc_rdma_send_error() seems to have only one caller, which ignores its return value. Should its return value be made void?

--b.

Quoted 21 lines

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

index 14f83a1..8adb2f0 100644
@@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
 ib_send_wr *wr)
 int ret;
 
 if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
-  return 0;
+  return -ENOTCONN;
 
 BUG_ON(wr->send_flags != IB_SEND_SIGNALED);

Quoted 1 line

BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=

--
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

Quoted 8 lines · J. Bruce Fields

Quoted 3 lines · Tom Tucker

Fix the return value on close to -ENOTCONN so caller knows to free context. Also if a thread is waiting for free SQ space, check for close when waking to avoid posting WR to a closing transport.

A random related question: svc_rdma_send_error() seems to have only one caller, which ignores its return value. Should its return value be made void?

Honestly, I don't think this code path has ever been excercised. This is the "protocol error" path. Given my experience with the "random transport error" tests that resulted in the patchset your currently looking at, I'd be inclined to defer this kind of thing until I've built a "protocol error" set of tests and worked through the "evil client" scenarios.

Thoughts?

Quoted 29 lines

--b.

Quoted 21 lines

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

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

index 14f83a1..8adb2f0 100644
@@ -999,7 +999,7 @@ int svc_rdma_send(struct svcxprt_rdma *xprt, struct
 ib_send_wr *wr)
 int ret;
 
 if (test_bit(XPT_CLOSE, &xprt->sc_xprt.xpt_flags))
-  return 0;
+  return -ENOTCONN;
 
 BUG_ON(wr->send_flags != IB_SEND_SIGNALED);

Quoted 1 line

BUG_ON(((struct svc_rdma_op_ctxt *)(unsigned long)wr->wr_id)->wr_op !=

--
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

A listening endpoint isn't known to the generic transport switch until the svc_create_xprt function returns without error. Calling svc_xprt_put within the xpo_create function causes the module reference count to be erroneously decremented.

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

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

index 83818cf..0d9a828 100644
@@ -669,7 +669,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 
 	listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);
 	if (IS_ERR(listen_id)) {
-		svc_xprt_put(&cma_xprt->sc_xprt);
+		kfree(cma_xprt);
 		dprintk("svcrdma: rdma_create_id failed = %ld\n",
 			PTR_ERR(listen_id));
 		return (void *)listen_id;

@@ -677,7 +677,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 	ret = rdma_bind_addr(listen_id, sa);
 	if (ret) {
 		rdma_destroy_id(listen_id);
-		svc_xprt_put(&cma_xprt->sc_xprt);
+		kfree(cma_xprt);
 		dprintk("svcrdma: rdma_bind_addr failed = %d\n", ret);
 		return ERR_PTR(ret);
 	}

@@ -686,7 +686,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 	ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
 	if (ret) {
 		rdma_destroy_id(listen_id);
-		svc_xprt_put(&cma_xprt->sc_xprt);
+		kfree(cma_xprt);
 		dprintk("svcrdma: rdma_listen failed = %d\n", ret);
 		return ERR_PTR(ret);
 	}

Quoted 4 lines · Tom Tucker

A listening endpoint isn't known to the generic transport switch until the svc_create_xprt function returns without error. Calling svc_xprt_put within the xpo_create function causes the module reference count to be erroneously decremented.

There's some redundant code in these three error paths; would the usual kernel-style "goto cleanup" thing help?

--b.

Quoted 41 lines

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

---
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

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

index 83818cf..0d9a828 100644
@@ -669,7 +669,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
 *serv,
 
 listen_id = rdma_create_id(rdma_listen_handler, cma_xprt, RDMA_PS_TCP);
 if (IS_ERR(listen_id)) {
-  svc_xprt_put(&cma_xprt->sc_xprt);
+  kfree(cma_xprt);
 dprintk("svcrdma: rdma_create_id failed = %ld\n",
 PTR_ERR(listen_id));

return (void *)listen_id;

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

@@ -677,7 +677,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
 *serv,
 ret = rdma_bind_addr(listen_id, sa);
 if (ret) {
 rdma_destroy_id(listen_id);
-  svc_xprt_put(&cma_xprt->sc_xprt);
+  kfree(cma_xprt);
 dprintk("svcrdma: rdma_bind_addr failed = %d\n", ret);
 return ERR_PTR(ret);

}

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

@@ -686,7 +686,7 @@ static struct svc_xprt *svc_rdma_create(struct svc_serv
 *serv,
 ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
 if (ret) {
 rdma_destroy_id(listen_id);
-  svc_xprt_put(&cma_xprt->sc_xprt);
+  kfree(cma_xprt);
 dprintk("svcrdma: rdma_listen failed = %d\n", ret);
 return ERR_PTR(ret);

Quoted 1 line

}

--
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

Quoted 6 lines · J. Bruce Fields

Quoted 4 lines · Tom Tucker

A listening endpoint isn't known to the generic transport switch until the svc_create_xprt function returns without error. Calling svc_xprt_put within the xpo_create function causes the module reference count to be erroneously decremented.

Quoted 2 lines

There's some redundant code in these three error paths; would the usual kernel-style "goto cleanup" thing help?

I think code-size it's a wash, but it might look more familiar.

BTW, I noticed that the error path above returns a positive errno :-\, so that needs to be fixed too.

Quoted 11 lines · Tom Tucker

Quoted 6 lines · J. Bruce Fields

Quoted 4 lines · Tom Tucker

A listening endpoint isn't known to the generic transport switch until the svc_create_xprt function returns without error. Calling svc_xprt_put within the xpo_create function causes the module reference count to be erroneously decremented.

Quoted 4 lines

Quoted 2 lines

There's some redundant code in these three error paths; would the usual kernel-style "goto cleanup" thing help?

I think code-size it's a wash, but it might look more familiar.

Yeah, we try to stick to the same idioms as used elsewhere when it's easy to.

Doesn't have to be done now, though.

Quoted 2 lines

BTW, I noticed that the error path above returns a positive errno :-\, so that needs to be fixed too.

Oops, didn't catch that. I'll expect a patch?

--b.

Quoted 22 lines · J. Bruce Fields

Quoted 11 lines · Tom Tucker

Quoted 6 lines · J. Bruce Fields

Quoted 4 lines · Tom Tucker

A listening endpoint isn't known to the generic transport switch until the svc_create_xprt function returns without error. Calling svc_xprt_put within the xpo_create function causes the module reference count to be erroneously decremented.

Quoted 4 lines

Quoted 2 lines

There's some redundant code in these three error paths; would the usual kernel-style "goto cleanup" thing help?

I think code-size it's a wash, but it might look more familiar.

Yeah, we try to stick to the same idioms as used elsewhere when it's easy to.

Doesn't have to be done now, though.

Quoted 4 lines

Quoted 2 lines

BTW, I noticed that the error path above returns a positive errno :-\, so that needs to be fixed too.

Oops, didn't catch that. I'll expect a patch?

I've got updates to three of the patches. I'm just retesting to make sure I didn't inadvertently break something. I'll get the three updated patches to you by tomorrow.

The rdma_read_xdr function did discriminate between no read-list and

an error posting the read-list. This results a leak of a page in the context when an asyncrhonous error occurs on the transport.

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

---
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +7 -3

index f3a108a..0ac375a 100644
@@ -260,11 +260,16 @@ static int rdma_read_max_sge(struct svcxprt_rdma *xprt, int sge_count)
  * On our side, we need to read into a pagelist. The first page immediately
  * follows the RPC header.
  *
- * This function returns 1 to indicate success. The data is not yet in
+ * This function returns:
+ * 0 - No error and no read-list found.
+ *
+ * 1 - Successful read-list processing. The data is not yet in
  * the pagelist and therefore the RPC request must be deferred. The
  * I/O completion will enqueue the transport again and
  * svc_rdma_recvfrom will complete the request.
  *
+ * <0 - Error processing/posting read-list.
+ *
  * NOTE: The ctxt must not be touched after the last WR has been posted
  * because the I/O completion processing may occur on another
  * processor and free / modify the context. Ne touche pas!

@@ -398,7 +403,7 @@ next_sge:
 			svc_rdma_put_context(head, 1);
 			head = ctxt;
 		}
-		return 0;
+		return err;
 	}
 
 	return 1;

@@ -536,8 +541,12 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 	 * it. Not that in this case, we don't return the RQ credit
 	 * until after the read completes.
 	 */
-	if (rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt)) {

Quoted 1 line · Tom Tucker

The rdma_read_xdr function did discriminate between no read-list and

    ^^^
    did not?

Quoted 47 lines

an error posting the read-list. This results a leak of a page in the context when an asyncrhonous error occurs on the transport.

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

---
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +6 -1

index f3a108a..0ac375a 100644
@@ -260,11 +260,16 @@ static int rdma_read_max_sge(struct svcxprt_rdma *xprt,
 int sge_count)
  * On our side, we need to read into a pagelist. The first page immediately
  * follows the RPC header.
  *
- * This function returns 1 to indicate success. The data is not yet in
+ * This function returns:
+ * 0 - No error and no read-list found.
+ *
+ * 1 - Successful read-list processing. The data is not yet in
  * the pagelist and therefore the RPC request must be deferred. The
  * I/O completion will enqueue the transport again and
  * svc_rdma_recvfrom will complete the request.
  *
+ * <0 - Error processing/posting read-list.
+ *
  * NOTE: The ctxt must not be touched after the last WR has been posted
  * because the I/O completion processing may occur on another

* processor and free / modify the context. Ne touche pas!

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +1 -2

@@ -398,7 +403,7 @@ next_sge:
 svc_rdma_put_context(head, 1);
 head = ctxt;
 }
-  return 0;
+  return err;
 }
 
 return 1;

@@ -536,8 +541,12 @@ int svc_rdma_recvfrom(struct svc_rqst *rqstp)
 * it. Not that in this case, we don't return the RQ credit
 * until after the read completes.
 */
- if (rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt)) {

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c int svc_rdma_recvfrom(struct svc_rqst *rqstp) · line 552

Quoted 10 lines

net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +6 -1

-  svc_xprt_received(xprt);
+ ret = rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt);
+ if (ret) {
+  if (ret > 0)
+   svc_xprt_received(xprt);
+  else
+   svc_rdma_put_context(ctxt, 1);
 return 0;
 }
 

Possibly slightly more straightforward?:

/* Read read-list data. */
ret = rdma_read_xdr(rdma_xprt, rmsgp, rqstp, ctxt); if (ret < 0) {
svc_rdma_put_context(ctxt, 1);
return 0;
}
if (ret > 0) {
/*
* We need to wait; defer the request. Note that in
* this case, we don't return the RQ credit until after
* the read completes.
*/
svc_xprt_received(xprt);
return 0;
}
...

--b.
--
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

Replace the one-off linked list implementation used to implement the context cache with the standard Linux list_head lists. Add a cpmtext

counter to catch resource leaks.

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

---
 include/linux/sunrpc/svc_rdma.h          |    5 ++-
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   47 ++++++++++++++++-------------
 2 files changed, 29 insertions(+), 23 deletions(-)

include/linux/sunrpc/svc_rdma.h +3 -2

index c447c41..7014390 100644
@@ -72,7 +72,7 @@ extern atomic_t rdma_stat_sq_prod;
  */
 struct svc_rdma_op_ctxt {
 	struct svc_rdma_op_ctxt *read_hdr;
-	struct svc_rdma_op_ctxt *next;
+	struct list_head free_list;
 	struct xdr_buf arg;
 	struct list_head dto_q;
 	enum ib_wr_opcode wr_op;

@@ -104,7 +104,8 @@ struct svcxprt_rdma {
 
 	struct ib_pd         *sc_pd;
 
-	struct svc_rdma_op_ctxt  *sc_ctxt_head;
+	atomic_t	     sc_ctxt_used;
+	struct list_head     sc_ctxt_free;
 	int		     sc_ctxt_cnt;
 	int		     sc_ctxt_bump;
 	int		     sc_ctxt_max;

net/sunrpc/xprtrdma/svc_rdma_transport.c +26 -21

index c852fd9..c842cc2 100644
@@ -103,8 +103,8 @@ static int rdma_bump_context_cache(struct svcxprt_rdma *xprt)
 		spin_lock_bh(&xprt->sc_ctxt_lock);
 		if (ctxt) {
 			at_least_one = 1;
-			ctxt->next = xprt->sc_ctxt_head;
-			xprt->sc_ctxt_head = ctxt;
+			INIT_LIST_HEAD(&ctxt->free_list);
+			list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
 		} else {
 			/* kmalloc failed...give up for now */
 			xprt->sc_ctxt_cnt--;

@@ -123,7 +123,7 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct svcxprt_rdma *xprt)
 
 	while (1) {
 		spin_lock_bh(&xprt->sc_ctxt_lock);
-		if (unlikely(xprt->sc_ctxt_head == NULL)) {
+		if (unlikely(list_empty(&xprt->sc_ctxt_free))) {
 			/* Try to bump my cache. */
 			spin_unlock_bh(&xprt->sc_ctxt_lock);
 

@@ -136,12 +136,15 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct svcxprt_rdma *xprt)
 			schedule_timeout_uninterruptible(msecs_to_jiffies(500));
 			continue;
 		}
-		ctxt = xprt->sc_ctxt_head;
-		xprt->sc_ctxt_head = ctxt->next;
+		ctxt = list_entry(xprt->sc_ctxt_free.next,
+				  struct svc_rdma_op_ctxt,
+				  free_list);
+		list_del_init(&ctxt->free_list);
 		spin_unlock_bh(&xprt->sc_ctxt_lock);
 		ctxt->xprt = xprt;
 		INIT_LIST_HEAD(&ctxt->dto_q);
 		ctxt->count = 0;
+		atomic_inc(&xprt->sc_ctxt_used);
 		break;
 	}
 	return ctxt;

@@ -163,10 +166,11 @@ void svc_rdma_put_context(struct svc_rdma_op_ctxt *ctxt, int free_pages)
 				 ctxt->sge[i].addr,
 				 ctxt->sge[i].length,
 				 ctxt->direction);
+
 	spin_lock_bh(&xprt->sc_ctxt_lock);
-	ctxt->next = xprt->sc_ctxt_head;
-	xprt->sc_ctxt_head = ctxt;
+	list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
 	spin_unlock_bh(&xprt->sc_ctxt_lock);
+	atomic_dec(&xprt->sc_ctxt_used);
 }
 
 /* ib_cq event handler */

@@ -409,28 +413,29 @@ static void create_context_cache(struct svcxprt_rdma *xprt,
 	xprt->sc_ctxt_max = ctxt_max;
 	xprt->sc_ctxt_bump = ctxt_bump;
 	xprt->sc_ctxt_cnt = 0;
-	xprt->sc_ctxt_head = NULL;
+	atomic_set(&xprt->sc_ctxt_used, 0);
+
+	INIT_LIST_HEAD(&xprt->sc_ctxt_free);
 	for (i = 0; i < ctxt_count; i++) {
 		ctxt = kmalloc(sizeof(*ctxt), GFP_KERNEL);
 		if (ctxt) {
-			ctxt->next = xprt->sc_ctxt_head;
-			xprt->sc_ctxt_head = ctxt;
+			INIT_LIST_HEAD(&ctxt->free_list);
+			list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
 			xprt->sc_ctxt_cnt++;
 		}
 	}
 }
 
-static void destroy_context_cache(struct svc_rdma_op_ctxt *ctxt)
+static void destroy_context_cache(struct svcxprt_rdma *xprt)
 {
-	struct svc_rdma_op_ctxt *next;
-	if (!ctxt)
-		return;
-
-	do {
-		next = ctxt->next;
+	while (!list_empty(&xprt->sc_ctxt_free)) {
+		struct svc_rdma_op_ctxt *ctxt;
+		ctxt = list_entry(xprt->sc_ctxt_free.next,
+				  struct svc_rdma_op_ctxt,
+				  free_list);
+		list_del_init(&ctxt->free_list);
 		kfree(ctxt);
-		ctxt = next;
-	} while (next);
+	}
 }
 
 static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,

@@ -467,7 +472,7 @@ static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,
 				     reqs +
 				     cma_xprt->sc_sq_depth +
 				     RPCRDMA_MAX_THREADS + 1); /* max */
-		if (!cma_xprt->sc_ctxt_head) {
+		if (list_empty(&cma_xprt->sc_ctxt_free)) {
 			kfree(cma_xprt);
 			return NULL;
 		}

@@ -971,7 +976,7 @@ static void svc_rdma_free(struct svc_xprt *xprt)
 	if (rdma->sc_pd && !IS_ERR(rdma->sc_pd))
 		ib_dealloc_pd(rdma->sc_pd);
 
-	destroy_context_cache(rdma->sc_ctxt_head);
+	destroy_context_cache(rdma);
 	kfree(rdma);
 }
 

Quoted 2 lines · Tom Tucker

Replace the one-off linked list implementation used to implement the context cache with the standard Linux list_head lists. Add a cpmtext

cmptext == context?

Some spell checking might help those of us who are already a little lost in the alphabet soup here.

Also, I assume that's referring to sc_ctxt_used. It's initialized, incremented, and decremented, but doesn't actually seem to be used anywhere yet?

--b.

Quoted 149 lines

counter to catch resource leaks.

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

---
 include/linux/sunrpc/svc_rdma.h          |    5 ++-
 net/sunrpc/xprtrdma/svc_rdma_transport.c |   47
++++++++++++++++-------------
 2 files changed, 29 insertions(+), 23 deletions(-)

include/linux/sunrpc/svc_rdma.h +3 -2

index c447c41..7014390 100644
@@ -72,7 +72,7 @@ extern atomic_t rdma_stat_sq_prod;
  */
 struct svc_rdma_op_ctxt {
 struct svc_rdma_op_ctxt *read_hdr;
- struct svc_rdma_op_ctxt *next;
+ struct list_head free_list;
 struct xdr_buf arg;
 struct list_head dto_q;
 enum ib_wr_opcode wr_op;

@@ -104,7 +104,8 @@ struct svcxprt_rdma {
 
 struct ib_pd         *sc_pd;
 
- struct svc_rdma_op_ctxt  *sc_ctxt_head;
+ atomic_t      sc_ctxt_used;
+ struct list_head     sc_ctxt_free;
 int       sc_ctxt_cnt;
 int       sc_ctxt_bump;
 int       sc_ctxt_max;

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

index c852fd9..c842cc2 100644
@@ -103,8 +103,8 @@ static int rdma_bump_context_cache(struct svcxprt_rdma
 *xprt)
 spin_lock_bh(&xprt->sc_ctxt_lock);
 if (ctxt) {
 at_least_one = 1;
-   ctxt->next = xprt->sc_ctxt_head;
-   xprt->sc_ctxt_head = ctxt;
+   INIT_LIST_HEAD(&ctxt->free_list);
+   list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
 } else {
 /* kmalloc failed...give up for now */

xprt->sc_ctxt_cnt--;

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

@@ -123,7 +123,7 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct
 svcxprt_rdma *xprt)
 
 while (1) {
 spin_lock_bh(&xprt->sc_ctxt_lock);
-  if (unlikely(xprt->sc_ctxt_head == NULL)) {
+  if (unlikely(list_empty(&xprt->sc_ctxt_free))) {
 /* Try to bump my cache. */
 spin_unlock_bh(&xprt->sc_ctxt_lock);

@@ -136,12 +136,15 @@ struct svc_rdma_op_ctxt *svc_rdma_get_context(struct
 svcxprt_rdma *xprt)
 schedule_timeout_uninterruptible(msecs_to_jiffies(500));
 continue;
 }
-  ctxt = xprt->sc_ctxt_head;
-  xprt->sc_ctxt_head = ctxt->next;
+  ctxt = list_entry(xprt->sc_ctxt_free.next,
+      struct svc_rdma_op_ctxt,
+      free_list);
+  list_del_init(&ctxt->free_list);
 spin_unlock_bh(&xprt->sc_ctxt_lock);
 ctxt->xprt = xprt;
 INIT_LIST_HEAD(&ctxt->dto_q);
 ctxt->count = 0;
+  atomic_inc(&xprt->sc_ctxt_used);
 break;
 }

return ctxt;

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

@@ -163,10 +166,11 @@ void svc_rdma_put_context(struct svc_rdma_op_ctxt
 *ctxt, int free_pages)
 ctxt->sge[i].addr,
 ctxt->sge[i].length,
 ctxt->direction);
+
 spin_lock_bh(&xprt->sc_ctxt_lock);
- ctxt->next = xprt->sc_ctxt_head;
- xprt->sc_ctxt_head = ctxt;
+ list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
 spin_unlock_bh(&xprt->sc_ctxt_lock);
+ atomic_dec(&xprt->sc_ctxt_used);
 }
 

/* ib_cq event handler */

net/sunrpc/xprtrdma/svc_rdma_transport.c +13 -12

@@ -409,28 +413,29 @@ static void create_context_cache(struct svcxprt_rdma
 *xprt,
 xprt->sc_ctxt_max = ctxt_max;
 xprt->sc_ctxt_bump = ctxt_bump;
 xprt->sc_ctxt_cnt = 0;
- xprt->sc_ctxt_head = NULL;
+ atomic_set(&xprt->sc_ctxt_used, 0);
+
+ INIT_LIST_HEAD(&xprt->sc_ctxt_free);
 for (i = 0; i < ctxt_count; i++) {
 ctxt = kmalloc(sizeof(*ctxt), GFP_KERNEL);
 if (ctxt) {
-   ctxt->next = xprt->sc_ctxt_head;
-   xprt->sc_ctxt_head = ctxt;
+   INIT_LIST_HEAD(&ctxt->free_list);
+   list_add(&ctxt->free_list, &xprt->sc_ctxt_free);
 xprt->sc_ctxt_cnt++;
 }
 }
 }
 
-static void destroy_context_cache(struct svc_rdma_op_ctxt *ctxt)
+static void destroy_context_cache(struct svcxprt_rdma *xprt)
 {
- struct svc_rdma_op_ctxt *next;
- if (!ctxt)
-  return;
-
- do {
-  next = ctxt->next;
+ while (!list_empty(&xprt->sc_ctxt_free)) {
+  struct svc_rdma_op_ctxt *ctxt;
+  ctxt = list_entry(xprt->sc_ctxt_free.next,
+      struct svc_rdma_op_ctxt,
+      free_list);
+  list_del_init(&ctxt->free_list);
 kfree(ctxt);
-  ctxt = next;
- } while (next);
+ }
 }
 

static struct svcxprt_rdma *rdma_create_xprt(struct svc_serv *serv,

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

@@ -467,7 +472,7 @@ static struct svcxprt_rdma *rdma_create_xprt(struct
 svc_serv *serv,
    reqs +
    cma_xprt->sc_sq_depth +
    RPCRDMA_MAX_THREADS + 1); /* max */
-  if (!cma_xprt->sc_ctxt_head) {
+  if (list_empty(&cma_xprt->sc_ctxt_free)) {
 kfree(cma_xprt);
 return NULL;

}

net/sunrpc/xprtrdma/svc_rdma_transport.c static void svc_rdma_free(struct svc_xprt *xprt) · line 982

Quoted 8 lines

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

@@ -971,7 +976,7 @@ static void svc_rdma_free(struct svc_xprt *xprt)
 if (rdma->sc_pd && !IS_ERR(rdma->sc_pd))
 ib_dealloc_pd(rdma->sc_pd);
 
- destroy_context_cache(rdma->sc_ctxt_head);
+ destroy_context_cache(rdma);
 kfree(rdma);
 }
 

--
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

Bruce:

This patchset fixes a number of defects in the close path of the SVCRDMA transport provider. The defects were found by injecting errors on the transport at random intervals while running concurrent iozone tests on IB and iWARP mounts. With this set of patches applied I was able to run the the above tests for several hours without causing a crash or leaking resources. The transport recovers correctly on a client reconnect.

This patchset is based on 2.6 top of tree, however, I think these fixes are good candidates for a 2.6.25 dot release as well.