| Message ID | 20150109192237.4901.92644.stgit@klimt.1015granger.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 1/9/2015 9:22 PM, Chuck Lever wrote: > xdr_start() can return the wrong rmsgp address if an assumption > about how the xdr_buf was constructed changes. When it gets it > wrong, the client receives a reply that has gibberish in the > RPC/RDMA header, preventing it from matching a waiting RPC request. > > Instead, make (and document) just one assumption: that the RDMA > header for the client's RPC call is at the start of the first page > in rq_pages. Would it make more sense to add another pointer assigned at req initialization (maybe in the RDMA request context) instead of hard coding this assumption? I may be completely wrong here though... > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > > net/sunrpc/xprtrdma/svc_rdma_sendto.c | 18 ++++-------------- > 1 files changed, 4 insertions(+), 14 deletions(-) > > diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c > index 7d79897..7de33d1 100644 > --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c > +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c > @@ -483,18 +483,6 @@ void svc_rdma_prep_reply_hdr(struct svc_rqst *rqstp) > { > } > > -/* > - * Return the start of an xdr buffer. > - */ > -static void *xdr_start(struct xdr_buf *xdr) > -{ > - return xdr->head[0].iov_base - > - (xdr->len - > - xdr->page_len - > - xdr->tail[0].iov_len - > - xdr->head[0].iov_len); > -} > - > int svc_rdma_sendto(struct svc_rqst *rqstp) > { > struct svc_xprt *xprt = rqstp->rq_xprt; > @@ -512,8 +500,10 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) > > dprintk("svcrdma: sending response for rqstp=%p\n", rqstp); > > - /* Get the RDMA request header. */ > - rdma_argp = xdr_start(&rqstp->rq_arg); > + /* Get the RDMA request header. The receive logic always > + * places this at the start of page 0. > + */ > + rdma_argp = page_address(rqstp->rq_pages[0]); > > /* Build an req vec for the XDR */ > ctxt = svc_rdma_get_context(rdma); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sagi- Thanks for the review. On Jan 11, 2015, at 12:37 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > On 1/9/2015 9:22 PM, Chuck Lever wrote: >> xdr_start() can return the wrong rmsgp address if an assumption >> about how the xdr_buf was constructed changes. When it gets it >> wrong, the client receives a reply that has gibberish in the >> RPC/RDMA header, preventing it from matching a waiting RPC request. >> >> Instead, make (and document) just one assumption: that the RDMA >> header for the client's RPC call is at the start of the first page >> in rq_pages. > > Would it make more sense to add another pointer assigned at req > initialization (maybe in the RDMA request context) instead of hard > coding this assumption? I may be completely wrong here though... I considered this. I couldn’t find an appropriate place to add such a pointer. I think that’s why xdr_start() was there in the first place: there is no convenient place to save a pointer to the request’s RDMA header. Bruce might have other thoughts about this. >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> >> net/sunrpc/xprtrdma/svc_rdma_sendto.c | 18 ++++-------------- >> 1 files changed, 4 insertions(+), 14 deletions(-) >> >> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c >> index 7d79897..7de33d1 100644 >> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c >> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c >> @@ -483,18 +483,6 @@ void svc_rdma_prep_reply_hdr(struct svc_rqst *rqstp) >> { >> } >> >> -/* >> - * Return the start of an xdr buffer. >> - */ >> -static void *xdr_start(struct xdr_buf *xdr) >> -{ >> - return xdr->head[0].iov_base - >> - (xdr->len - >> - xdr->page_len - >> - xdr->tail[0].iov_len - >> - xdr->head[0].iov_len); >> -} >> - >> int svc_rdma_sendto(struct svc_rqst *rqstp) >> { >> struct svc_xprt *xprt = rqstp->rq_xprt; >> @@ -512,8 +500,10 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) >> >> dprintk("svcrdma: sending response for rqstp=%p\n", rqstp); >> >> - /* Get the RDMA request header. */ >> - rdma_argp = xdr_start(&rqstp->rq_arg); >> + /* Get the RDMA request header. The receive logic always >> + * places this at the start of page 0. >> + */ >> + rdma_argp = page_address(rqstp->rq_pages[0]); >> >> /* Build an req vec for the XDR */ >> ctxt = svc_rdma_get_context(rdma); >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/12/2015 2:30 AM, Chuck Lever wrote: > Hi Sagi- > > Thanks for the review. > > On Jan 11, 2015, at 12:37 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote: > >> On 1/9/2015 9:22 PM, Chuck Lever wrote: >>> xdr_start() can return the wrong rmsgp address if an assumption >>> about how the xdr_buf was constructed changes. When it gets it >>> wrong, the client receives a reply that has gibberish in the >>> RPC/RDMA header, preventing it from matching a waiting RPC request. >>> >>> Instead, make (and document) just one assumption: that the RDMA >>> header for the client's RPC call is at the start of the first page >>> in rq_pages. >> >> Would it make more sense to add another pointer assigned at req >> initialization (maybe in the RDMA request context) instead of hard >> coding this assumption? I may be completely wrong here though... > > I considered this. I couldn’t find an appropriate place to add > such a pointer. > > I think that’s why xdr_start() was there in the first place: there > is no convenient place to save a pointer to the request’s RDMA > header. > > Bruce might have other thoughts about this. Yep, I didn't find any nice place to put that also, thought you might have an idea... Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c index 7d79897..7de33d1 100644 --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c @@ -483,18 +483,6 @@ void svc_rdma_prep_reply_hdr(struct svc_rqst *rqstp) { } -/* - * Return the start of an xdr buffer. - */ -static void *xdr_start(struct xdr_buf *xdr) -{ - return xdr->head[0].iov_base - - (xdr->len - - xdr->page_len - - xdr->tail[0].iov_len - - xdr->head[0].iov_len); -} - int svc_rdma_sendto(struct svc_rqst *rqstp) { struct svc_xprt *xprt = rqstp->rq_xprt; @@ -512,8 +500,10 @@ int svc_rdma_sendto(struct svc_rqst *rqstp) dprintk("svcrdma: sending response for rqstp=%p\n", rqstp); - /* Get the RDMA request header. */ - rdma_argp = xdr_start(&rqstp->rq_arg); + /* Get the RDMA request header. The receive logic always + * places this at the start of page 0. + */ + rdma_argp = page_address(rqstp->rq_pages[0]); /* Build an req vec for the XDR */ ctxt = svc_rdma_get_context(rdma);
xdr_start() can return the wrong rmsgp address if an assumption about how the xdr_buf was constructed changes. When it gets it wrong, the client receives a reply that has gibberish in the RPC/RDMA header, preventing it from matching a waiting RPC request. Instead, make (and document) just one assumption: that the RDMA header for the client's RPC call is at the start of the first page in rq_pages. Signed-off-by: Chuck Lever <chuck.lever@oracle.com> --- net/sunrpc/xprtrdma/svc_rdma_sendto.c | 18 ++++-------------- 1 files changed, 4 insertions(+), 14 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html