Fix regression in NFSRDMA server
Subsystems (3)
PATCH 1/1 Fix regression in NFSRDMA server
From: Tom Tucker <[email protected]>
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.
---
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 12 ++++-------- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 1 + 2 files changed, 5 insertions(+), 8 deletions(-)
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4 -8
@@ -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
@@ -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 5 lines · Steve Wise
From: Tom Tucker <[email protected]>
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 therq_respagesarray.
Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
Also: I don't get why all these rq_next_page initializations are required. Why isn't the initialization at the top of svc_process() enough? Is rdma using it before we get to that point? The only use of it I see off hand is in the while loop that you're deleting.
--b.
Quoted 73 lines
Signed-off-by Tom Tucker <[email protected]>Tested-by Steve Wise <[email protected]>---
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 12 ++++-------- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 1 + 2 files changed, 5 insertions(+), 8 deletions(-)net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4
-8@@ -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@@ -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 10 lines
Quoted 4 lines
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 therq_respagesarray.Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
Chelsio is adding NFSRDMA testing into their QA flow over their iWARP devices.
Hi Bruce,
Quoted 10 lines · J. Bruce Fields
Quoted 5 lines · Steve Wise
From: Tom Tucker <[email protected]>
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 therq_respagesarray.Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
I think Chelsio is going to be adding some NFSRDMA regression testing to their system test.
Quoted 4 lines
Also: I don't get why all these
rq_next_pageinitializations are required. Why isn't the initialization at the top ofsvc_process()enough? Is rdma using it before we get to that point? The only use of it I see off hand is in the while loop that you're deleting.
I didn't apply tremendous deductive powers here, I just added updates to rq_next_page wherever the transport messed with rq_respages. That said, NFS WRITE is likely the culprit since the write is completed as a deferral and therefore the request doesn't go through svc_process, so if rq_next_page is bogus, the cleanup will free/re-use pages that are actually in use by the transport.
Tom
Quoted 78 lines
--b.
Quoted 65 lines
Signed-off-by Tom Tucker <[email protected]>Tested-by Steve Wise <[email protected]>---
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 12 ++++-------- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 1 + 2 files changed, 5 insertions(+), 8 deletions(-)net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4
-8@@ -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 +0
-0net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdma, · line 631
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-0@@ -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 ;--
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 9 lines · Tom Tucker
Hi Bruce,
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Steve Wise
From: Tom Tucker <[email protected]>
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 therq_respagesarray.
Quoted 7 lines
Quoted 4 lines
Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
I think Chelsio is going to be adding some NFSRDMA regression testing to their system test.
OK. Do you know who there is setting that up? I'd be curious exactly what kernels they intend to test and how they plan to report results.
Quoted 11 lines
Quoted 4 lines
Also: I don't get why all these
rq_next_pageinitializations are required. Why isn't the initialization at the top ofsvc_process()enough? Is rdma using it before we get to that point? The only use of it I see off hand is in the while loop that you're deleting.I didn't apply tremendous deductive powers here, I just added updates to
rq_next_pagewherever the transport messed withrq_respages. That said, NFS WRITE is likely the culprit since the write is completed as a deferral and therefore the request doesn't go throughsvc_process, so ifrq_next_pageis bogus, the cleanup will free/re-use pages that are actually in use by the transport.
Ugh, OK, without tracing through the code I guess I can see how that would happen. Remind me why it's using deferrals?
Applying the patch.
--b.
Quoted 73 lines
Tom
Quoted 70 lines
--b.
Quoted 57 lines
Signed-off-by Tom Tucker <[email protected]>Tested-by Steve Wise <[email protected]>---
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 12 ++++-------- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 1 + 2 files changed, 5 insertions(+), 8 deletions(-)net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4
-8@@ -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 +0
-0net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdma, · line 631
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-0@@ -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 ;--
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
+Indranil
Indranil Choudhury is the QA contact.
Steve
Quoted 122 lines
-----Original Message-----
From: J. Bruce Fields [mailto:[email protected]]
Sent: Friday, March 28, 2014 4:27 PM
To: Tom Tucker
Cc: Steve Wise; [email protected]; [email protected]
Subject: Re: [PATCH] Fix regression in NFSRDMA serverQuoted 9 lines · Tom Tucker
Hi Bruce,
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Steve Wise
From: Tom Tucker <[email protected]>
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 therq_respagesarray.Quoted 7 lines
Quoted 4 lines
Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
I think Chelsio is going to be adding some NFSRDMA regression testing to their system test.
OK. Do you know who there is setting that up? I'd be curious exactly what kernels they intend to test and how they plan to report results.
Quoted 11 lines
Quoted 4 lines
Also: I don't get why all these
rq_next_pageinitializations are required. Why isn't the initialization at the top ofsvc_process()enough? Is rdma using it before we get to that point? The only use of it I see off hand is in the while loop that you're deleting.I didn't apply tremendous deductive powers here, I just added updates to
rq_next_pagewherever the transport messed withrq_respages. That said, NFS WRITE is likely the culprit since the write is completed as a deferral and therefore the request doesn't go throughsvc_process, so ifrq_next_pageis bogus, the cleanup will free/re-use pages that are actually in use by the transport.Ugh, OK, without tracing through the code I guess I can see how that would happen. Remind me why it's using deferrals?
Applying the patch.
--b.
Quoted 8 lines
Tom
Quoted 6 lines
--b.
Quoted 4 lines
Signed-off-by Tom Tucker <[email protected]>Tested-by Steve Wise <[email protected]>---
Quoted 5 lines
Quoted 5 lines
Review
Quoted 5 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 12 ++++-------- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 1 + 2 files changed, 5 insertions(+), 8 deletions(-)(quoted diff) +0
-0
b/net/sunrpc/xprtrdma/svc_rdma_recvfrom.cQuoted 35 lines
Quoted 35 lines
Quoted 35 lines
index 0ce7552..8d904e4 100644
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +3
-7@@ -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; }Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +1
-1@@ -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 ];(quoted diff) +0
-0
b/net/sunrpc/xprtrdma/svc_rdma_sendto.cQuoted 17 lines
Quoted 16 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdma, · line 631
Quoted 12 lines
index c1d124d..11e90f8 100644
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-0@@ -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 ;--
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
Hi Bruce,
Quoted 10 lines · J. Bruce Fields
Quoted 9 lines · Tom Tucker
Hi Bruce,
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Steve Wise
From: Tom Tucker <[email protected]>
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 therq_respagesarray.
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
I think Chelsio is going to be adding some NFSRDMA regression testing to their system test.
OK. Do you know who there is setting that up? I'd be curious exactly what kernels they intend to test and how they plan to report results.
I don't know, Steve can weigh in on this...
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Also: I don't get why all these
rq_next_pageinitializations are required. Why isn't the initialization at the top ofsvc_process()enough? Is rdma using it before we get to that point? The only use of it I see off hand is in the while loop that you're deleting.
Quoted 8 lines
Quoted 6 lines
I didn't apply tremendous deductive powers here, I just added updates to
rq_next_pagewherever the transport messed withrq_respages. That said, NFS WRITE is likely the culprit since the write is completed as a deferral and therefore the request doesn't go throughsvc_process, so ifrq_next_pageis bogus, the cleanup will free/re-use pages that are actually in use by the transport.Ugh, OK, without tracing through the code I guess I can see how that would happen. Remind me why it's using deferrals?
The server fetches the write data from the client using RDMA READ. So the request says ... "here's where the data is in my memory", and then the server issues an RDMA READ to fetch it. When the read completes, the deferred request is completed.
Tom
Quoted 75 lines
Applying the patch.
--b.
Quoted 71 lines
Tom
Quoted 70 lines
--b.
Quoted 57 lines
Signed-off-by Tom Tucker <[email protected]>Tested-by Steve Wise <[email protected]>---
net/sunrpc/xprtrdma/svc_rdma_recvfrom.c | 12 ++++-------- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 1 + 2 files changed, 5 insertions(+), 8 deletions(-)net/sunrpc/xprtrdma/svc_rdma_recvfrom.c +4
-8@@ -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 +0
-0net/sunrpc/xprtrdma/svc_rdma_sendto.c static int send_reply(struct svcxprt_rdma *rdma, · line 631
Quoted 7 lines
net/sunrpc/xprtrdma/svc_rdma_sendto.c +1
-0@@ -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 ;--
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 35 lines · Tom Tucker
Hi Bruce,
Quoted 10 lines · J. Bruce Fields
Quoted 9 lines · Tom Tucker
Hi Bruce,
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Steve Wise
From: Tom Tucker <[email protected]>
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 therq_respagesarray.Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Apologies. (But, it could happen again--could we set up some regular testing? It doesn't have to be anything fancy, just cthon over rdma--really, just read and write over rdma--would probably catch a lot.)
I think Chelsio is going to be adding some NFSRDMA regression testing to their system test.
OK. Do you know who there is setting that up? I'd be curious exactly what kernels they intend to test and how they plan to report results.
I don't know, Steve can weigh in on this...
Quoted 10 lines
Quoted 10 lines
Quoted 4 lines
Also: I don't get why all these
rq_next_pageinitializations are required. Why isn't the initialization at the top ofsvc_process()enough? Is rdma using it before we get to that point? The only use of it I see off hand is in the while loop that you're deleting.I didn't apply tremendous deductive powers here, I just added updates to
rq_next_pagewherever the transport messed withrq_respages. That said, NFS WRITE is likely the culprit since the write is completed as a deferral and therefore the request doesn't go throughsvc_process, so ifrq_next_pageis bogus, the cleanup will free/re-use pages that are actually in use by the transport.
Quoted 7 lines
Quoted 2 lines
Ugh, OK, without tracing through the code I guess I can see how that would happen. Remind me why it's using deferrals?
The server fetches the write data from the client using RDMA READ. So the request says ... "here's where the data is in my memory", and then the server issues an RDMA READ to fetch it. When the read completes, the deferred request is completed.
That makes sense, but maybe I'm not sure what you mean by deferring.
The tcp code can also receive a request over multiple recvfroms. See Trond's hack in 31d68ef65c7d4 "SUNRPC: Don't wait for full record to receive tcp data".
--b.