> On Jan 31, 2018, at 9:04 AM, Kalderon, Michal <Michal.Kalderon@xxxxxxxxxx> wrote: > >> From: Chuck Lever [mailto:chucklever@xxxxxxxxx] On Behalf Of Chuck Lever >> Sent: Tuesday, January 30, 2018 10:54 PM >> To: Kalderon, Michal <Michal.Kalderon@xxxxxxxxxx> >> Cc: linux-rdma@xxxxxxxxxxxxxxx; linux-nfs@xxxxxxxxxxxxxxx >> Subject: [PATCH v1] xprtrdma: Fix calculation of ri_max_send_sges >> >> Commit 16f906d66cd7 ("xprtrdma: Reduce required number of send >> SGEs") introduced the rpcrdma_ia::ri_max_send_sges field. This fixes a >> problem where xprtrdma would not work if the device's max_sge capability >> was small (low single digits). >> >> At least RPCRDMA_MIN_SEND_SGES are needed for the inline parts of each >> RPC. ri_max_send_sges is set to this value: >> >> ia->ri_max_send_sges = max_sge - RPCRDMA_MIN_SEND_SGES; >> >> Then when marshaling each RPC, rpcrdma_args_inline uses that value to >> determine whether the device has enough Send SGEs to convey an NFS >> WRITE payload inline, or whether instead a Read chunk is required. >> >> More recently, commit ae72950abf99 ("xprtrdma: Add data structure to >> manage RDMA Send arguments") used the ri_max_send_sges value to >> calculate the size of an array, but that commit erroneously assumed >> ri_max_send_sges contains a value similar to the device's max_sge, and not >> one that was reduced by the minimum SGE count. >> >> This assumption results in the calculated size of the sendctx's Send SGE array >> to be too small. When the array is used to marshal an RPC, the code can write >> Send SGEs into the following sendctx element in that array, corrupting it. >> When the device's max_sge is large, this issue is entirely harmless; but it >> results in an oops in the provider's post_send method, if dev.attrs.max_sge >> is small. >> >> So let's straighten this out: ri_max_send_sges will now contain a value with >> the same meaning as dev.attrs.max_sge, which makes the code easier to >> understand, and enables rpcrdma_sendctx_create to calculate the size of >> the SGE array correctly. >> >> Reported-by: Michal Kalderon <Michal.Kalderon@xxxxxxxxxx> >> Fixes: 16f906d66cd7 ("xprtrdma: Reduce required number of send SGEs") >> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx> >> --- >> net/sunrpc/xprtrdma/rpc_rdma.c | 2 +- >> net/sunrpc/xprtrdma/verbs.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c >> b/net/sunrpc/xprtrdma/rpc_rdma.c index 162e5dd..f0855a9 100644 >> --- a/net/sunrpc/xprtrdma/rpc_rdma.c >> +++ b/net/sunrpc/xprtrdma/rpc_rdma.c >> @@ -143,7 +143,7 @@ static bool rpcrdma_args_inline(struct rpcrdma_xprt >> *r_xprt, >> if (xdr->page_len) { >> remaining = xdr->page_len; >> offset = offset_in_page(xdr->page_base); >> - count = 0; >> + count = RPCRDMA_MIN_SEND_SGES; >> while (remaining) { >> remaining -= min_t(unsigned int, >> PAGE_SIZE - offset, remaining); diff >> --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c index >> f4eb63e..bb56b9d 100644 >> --- a/net/sunrpc/xprtrdma/verbs.c >> +++ b/net/sunrpc/xprtrdma/verbs.c >> @@ -505,7 +505,7 @@ >> pr_warn("rpcrdma: HCA provides only %d send SGEs\n", >> max_sge); >> return -ENOMEM; >> } >> - ia->ri_max_send_sges = max_sge - RPCRDMA_MIN_SEND_SGES; >> + ia->ri_max_send_sges = max_sge; >> >> if (ia->ri_device->attrs.max_qp_wr <= RPCRDMA_BACKWARD_WRS) >> { >> dprintk("RPC: %s: insufficient wqe's available\n", > > thanks, this fixed our first issue of mount failing. May I add "Tested-by: Michal Kalderon <Michal.Kalderon@xxxxxxxxxx>" ? -- Chuck Lever -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
