Skip to contentSkip to conversation

Fix regression errors in NFSRDMA

PATCH In review Steve Wise 2 participants 3 messages Mar 25, 2014 – Apr 2, 2014

Subsystems (3)
lore
Share & tools
lore

Steve Wise

author
#0 lore raw

PATCH 1/1 Fix regression errors in NFSRDMA

From: Tom Tucker <[email protected]>

A few changes regressed the client and server transports for NFSRDMA.

The server regression was caused by the addition of rq_next_page (afc59400d6c65bad66d4ad0b2daf879cbff8e23e). There were a few places that were missed with the update of the rq_respages array.

The client regression was due to additional XDR sanity checking (64bd577ea0021f5903505de061b3b7d8a785ee94) that exposed a latent bug in the NFSRDMA client. The bug is that if there were inline data, then the rpcrdma_fixup_inline function would would incorrectly set the XDR page_len to zero. Since the decode_read3resok logic previously computed this value from the reported len - header len, the bug was not symptomatic.

---

 net/sunrpc/xprtrdma/rpc_rdma.c          |    3 +--
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   12 ++++--------
 net/sunrpc/xprtrdma/svc_rdma_sendto.c   |    1 +
 3 files changed, 6 insertions(+), 10 deletions(-)
net/sunrpc/xprtrdma/rpc_rdma.c +1 -2
index e03725b..e811c40 100644
@@ -650,8 +650,7 @@ rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int copy_len, int pad)
 			page_base = 0;
 		}
 		rqst->rq_rcv_buf.page_len = olen - copy_len;
-	} else
-		rqst->rq_rcv_buf.page_len = 0;
+	}
 
 	if (copy_len && rqst->rq_rcv_buf.tail[0].iov_len) {
 		curlen = copy_len;
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4 -8
index 0ce7552..8d904e4 100644
@@ -90,6 +90,7 @@ static void rdma_build_arg_xdr(struct svc_rqst *rqstp,
 		sge_no++;
 	}
 	rqstp->rq_respages = &rqstp->rq_pages[sge_no];
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 	/* We should never run out of SGE because the limit is defined to
 	 * support the max allowed RPC data length
@@ -169,6 +170,7 @@ static int map_read_chunks(struct svcxprt_rdma *xprt,
 		 */
 		head->arg.pages[page_no] = rqstp->rq_arg.pages[page_no];
 		rqstp->rq_respages = &rqstp->rq_arg.pages[page_no+1];
+		rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 		byte_count -= sge_bytes;
 		ch_bytes -= sge_bytes;
@@ -276,6 +278,7 @@ static int fast_reg_read_chunks(struct svcxprt_rdma *xprt,
 
 	/* rq_respages points one past arg pages */
 	rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 	/* Create the reply and chunk maps */
 	offset = 0;
@@ -520,13 +523,6 @@ next_sge:
 	for (ch_no = 0; &rqstp->rq_pages[ch_no] < rqstp->rq_respages; ch_no++)
 		rqstp->rq_pages[ch_no] = NULL;
 
-	/*
-	 * Detach res pages. If svc_release sees any it will attempt to
-	 * put them.
-	 */
-	while (rqstp->rq_next_page != rqstp->rq_respages)
-		*(--rqstp->rq_next_page) = NULL;
-
 	return err;
 }
 
@@ -550,7 +546,7 @@ static int rdma_read_complete(struct svc_rqst *rqstp,
 
 	/* rq_respages starts after the last arg page */
 	rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
-	rqstp->rq_next_page = &rqstp->rq_arg.pages[page_no];
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 	/* Rebuild rq_arg head and tail. */
 	rqstp->rq_arg.head[0] = head->arg.head[0];
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -0
index c1d124d..11e90f8 100644
@@ -625,6 +625,7 @@ static int send_reply(struct svcxprt_rdma *rdma,
 		if (page_no+1 >= sge_no)
 			ctxt->sge[page_no+1].length = 0;
 	}
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 	BUG_ON(sge_no > rdma->sc_max_sge);
 	memset(&send_wr, 0, sizeof send_wr);
 	ctxt->wr_op = IB_WR_SEND;
author
#1 lore raw
Quoted 6 lines

The client regression was due to additional XDR sanity checking (64bd577ea0021f5903505de061b3b7d8a785ee94) that exposed a latent bug in the NFSRDMA client. The bug is that if there were inline data, then the rpcrdma_fixup_inline function would would incorrectly set the XDR page_len to zero. Since the decode_read3resok logic previously computed this value from the reported len - header len, the bug was not symptomatic.

Oops: The client bug has already been fixed by Chuck's patch which is already 

in Trond's nfs-for-next branch:

----
commit 2b7bbc963da8d076f263574af4138b5df2e1581f

Author: Chuck Lever <[email protected]>
Date:   Wed Mar 12 12:51:30 2014 -0400

SUNRPC: Fix large reads on NFS/RDMA
----

So I'll repost Tom's server-side fix, after applying it on nfs-for-next and verifying things still work.

Steve.

Tested-by on v1 1/1
#2 lore raw
In reply to Steve Wise
Quoted 103 lines · On Tue, 25 Mar 2014 09:50:45 -0500 Steve Wise <[email protected]>…

From: Tom Tucker <[email protected]>

A few changes regressed the client and server transports for NFSRDMA.

The server regression was caused by the addition of rq_next_page (afc59400d6c65bad66d4ad0b2daf879cbff8e23e). There were a few places that were missed with the update of the rq_respages array.

The client regression was due to additional XDR sanity checking (64bd577ea0021f5903505de061b3b7d8a785ee94) that exposed a latent bug in the NFSRDMA client. The bug is that if there were inline data, then the rpcrdma_fixup_inline function would would incorrectly set the XDR page_len to zero. Since the decode_read3resok logic previously computed this value from the reported len - header len, the bug was not symptomatic.

---

 net/sunrpc/xprtrdma/rpc_rdma.c          |    3 +--
 net/sunrpc/xprtrdma/svc_rdma_recvfrom.c |   12 ++++--------
 net/sunrpc/xprtrdma/svc_rdma_sendto.c   |    1 +
 3 files changed, 6 insertions(+), 10 deletions(-)
net/sunrpc/xprtrdma/rpc_rdma.c +1 -2
index e03725b..e811c40 100644
@@ -650,8 +650,7 @@ rpcrdma_inline_fixup(struct rpc_rqst *rqst, char *srcp, int copy_len, int pad)
 			page_base = 0;
 		}
 		rqst->rq_rcv_buf.page_len = olen - copy_len;
-	} else
-		rqst->rq_rcv_buf.page_len = 0;
+	}
 
 	if (copy_len && rqst->rq_rcv_buf.tail[0].iov_len) {
 		curlen = copy_len;
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4 -8
index 0ce7552..8d904e4 100644
@@ -90,6 +90,7 @@ static void rdma_build_arg_xdr(struct svc_rqst *rqstp,
 		sge_no++;
 	}
 	rqstp->rq_respages = &rqstp->rq_pages[sge_no];
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 	/* We should never run out of SGE because the limit is defined to
 	 * support the max allowed RPC data length
@@ -169,6 +170,7 @@ static int map_read_chunks(struct svcxprt_rdma *xprt,
 		 */
 		head->arg.pages[page_no] = rqstp->rq_arg.pages[page_no];
 		rqstp->rq_respages = &rqstp->rq_arg.pages[page_no+1];
+		rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 		byte_count -= sge_bytes;
 		ch_bytes -= sge_bytes;
@@ -276,6 +278,7 @@ static int fast_reg_read_chunks(struct svcxprt_rdma *xprt,
 
 	/* rq_respages points one past arg pages */
 	rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 	/* Create the reply and chunk maps */
 	offset = 0;
@@ -520,13 +523,6 @@ next_sge:
 	for (ch_no = 0; &rqstp->rq_pages[ch_no] < rqstp->rq_respages; ch_no++)
 		rqstp->rq_pages[ch_no] = NULL;
 
-	/*
-	 * Detach res pages. If svc_release sees any it will attempt to
-	 * put them.
-	 */
-	while (rqstp->rq_next_page != rqstp->rq_respages)
-		*(--rqstp->rq_next_page) = NULL;
-
 	return err;
 }
 
@@ -550,7 +546,7 @@ static int rdma_read_complete(struct svc_rqst *rqstp,
 
 	/* rq_respages starts after the last arg page */
 	rqstp->rq_respages = &rqstp->rq_arg.pages[page_no];
-	rqstp->rq_next_page = &rqstp->rq_arg.pages[page_no];
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 
 	/* Rebuild rq_arg head and tail. */
 	rqstp->rq_arg.head[0] = head->arg.head[0];
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1 -1
index c1d124d..11e90f8 100644
@@ -625,6 +625,7 @@ static int send_reply(struct svcxprt_rdma *rdma,
 		if (page_no+1 >= sge_no)
 			ctxt->sge[page_no+1].length = 0;
 	}
+	rqstp->rq_next_page = rqstp->rq_respages + 1;
 	BUG_ON(sge_no > rdma->sc_max_sge);
 	memset(&send_wr, 0, sizeof send_wr);
 	ctxt->wr_op = IB_WR_SEND;
 
--
Quoted 3 lines

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

This seems to fix the oopses I was seeing when issuing NFS WRITEs to the server: