Hi Sagi-
Thanks for the review.
On Jan 11, 2015, at 12:37 PM, Sagi Grimberg <sagig@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>
>> 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
