Re: [PATCH v4 01/11] svcrdma: Do not send XDR roundup bytes for a write chunk

["J. Bruce Fields" <bfields@fieldses.org>]

On Mon, Dec 21, 2015 at 05:11:56PM -0500, Chuck Lever wrote:
> 
> > On Dec 21, 2015, at 4:29 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > 
> > On Mon, Dec 21, 2015 at 04:15:23PM -0500, Chuck Lever wrote:
> >> 
> >>> On Dec 21, 2015, at 4:07 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >>> 
> >>> On Mon, Dec 14, 2015 at 04:30:09PM -0500, Chuck Lever wrote:
> >>>> Minor optimization: when dealing with write chunk XDR roundup, do
> >>>> not post a Write WR for the zero bytes in the pad. Simply update
> >>>> the write segment in the RPC-over-RDMA header to reflect the extra
> >>>> pad bytes.
> >>>> 
> >>>> The Reply chunk is also a write chunk, but the server does not use
> >>>> send_write_chunks() to send the Reply chunk. That's OK in this case:
> >>>> the server Upper Layer typically marshals the Reply chunk contents
> >>>> in a single contiguous buffer, without a separate tail for the XDR
> >>>> pad.
> >>>> 
> >>>> The comments and the variable naming refer to "chunks" but what is
> >>>> really meant is "segments." The existing code sends only one
> >>>> xdr_write_chunk per RPC reply.
> >>>> 
> >>>> The fix assumes this as well. When the XDR pad in the first write
> >>>> chunk is reached, the assumption is the Write list is complete and
> >>>> send_write_chunks() returns.
> >>>> 
> >>>> That will remain a valid assumption until the server Upper Layer can
> >>>> support multiple bulk payload results per RPC.
> >>>> 
> >>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >>>> ---
> >>>> net/sunrpc/xprtrdma/svc_rdma_sendto.c |    7 +++++++
> >>>> 1 file changed, 7 insertions(+)
> >>>> 
> >>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_sendto.c b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> >>>> index 969a1ab..bad5eaa 100644
> >>>> --- a/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> >>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_sendto.c
> >>>> @@ -342,6 +342,13 @@ static int send_write_chunks(struct svcxprt_rdma *xprt,
> >>>> 						arg_ch->rs_handle,
> >>>> 						arg_ch->rs_offset,
> >>>> 						write_len);
> >>>> +
> >>>> +		/* Do not send XDR pad bytes */
> >>>> +		if (chunk_no && write_len < 4) {
> >>>> +			chunk_no++;
> >>>> +			break;
> >>> 
> >>> I'm pretty lost in this code.  Why does (chunk_no && write_len < 4) mean
> >>> this is xdr padding?
> >> 
> >> Chunk zero is always data. Padding is always going to be
> >> after the first chunk. Any chunk after chunk zero that is
> >> shorter than XDR quad alignment is going to be a pad.
> > 
> > I don't really know what a chunk is....  Looking at the code:
> > 
> > 	write_len = min(xfer_len, be32_to_cpu(arg_ch->rs_length));
> > 
> > so I guess the assumption is just that those rs_length's are always a
> > multiple of four?
> 
> The example you recently gave was a two-byte NFS READ
> that crosses a page boundary.
> 
> In that case, the NFSD would pass down an xdr_buf that
> has one byte in a page, one byte in another page, and
> a two-byte XDR pad. The logic introduced by this
> optimization would be fooled, and neither the second
> byte nor the XDR pad would be written to the client.
> 
> Unless you can think of a way to recognize an XDR pad
> in the xdr_buf 100% of the time, you should drop this
> patch.

It might be best to make this explicit and mark bytes as padding somehow
while encoding.

> As far as I know, none of the other patches in this
> series depend on this optimization, so please merge
> them if you can.

OK, I'll take a look at the rest, thanks.

--b.

> >> Probably too clever. Is there a better way to detect
> >> the XDR pad?
> >> 
> >> 
> >>>> +		}
> >>>> +
> >>>> 		chunk_off = 0;
> >>>> 		while (write_len) {
> >>>> 			ret = send_write(xprt, rqstp,
> >> 
> >> --
> >> Chuck Lever
> >> 
> >> 
> >> 
> > --
> > 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
> 
> --
> Chuck Lever
> 
> 
>