Re: [RFC Patch 08/09] NFS/RDMA client - rpcrdma protocol handling

[Chuck Lever <chuck.lever@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 6004 bytes --]

Talpey, Thomas wrote:
> At 01:11 PM 7/13/2007, Chuck Lever wrote:
>> Talpey, Thomas wrote:
>>> The alternative is mentioned, and would involve marking pagelists
>>> built in xdr_inline_pages(), this of course would also require changes to
>>> the NFS-layer callers. I am prepared to do that, pending the outcome
>>> of these comments.
>> I would humbly prefer the clean alternative: I think several other 
>> operations can use this.  Seems like the distinction is the operations 
>> that read data (like readdir, readlink, read) and those that read 
>> metadata (getattr).
>>
>> The ULP should provide a hint on each of these.  Possibly you could hack 
>> the nfs_procedures tables (which is an RPC client data structure) to 
>> provide the hint.
> 
> So, to clean this up, I looked into hacking this flag into the rpc_procinfo
> as you suggested. It works, but I think it's too high up in the layering.
> The issue is that we want to stamp each buffer with its bulk-data disposition,
> not the entire procedure. For example, there might in the future be more than
> one READ in an NFSv4 COMPOUND.
> 
> What do you think of the following? There are some data movers in xdr.c
> that might peek at this flag for hints too, I haven't gone there yet.

I like this much better than what was there.  The NFS client tells the 
transport layer exactly which pages are bulk, instead of having the 
transport guess.

The name "page_is_bulk" however implies that you are marking the pages, 
where really, you are marking the buffer.  Marking the whole buffer is 
probably correct, but then you should probably rename the flag for clarity.

> Index: linux-2.6.22/fs/nfs/nfs2xdr.c
> ===================================================================
> --- linux-2.6.22.orig/fs/nfs/nfs2xdr.c
> +++ linux-2.6.22/fs/nfs/nfs2xdr.c
> @@ -251,6 +251,7 @@ nfs_xdr_readargs(struct rpc_rqst *req, _
>  	replen = (RPC_REPHDRSIZE + auth->au_rslack + NFS_readres_sz) << 2;
>  	xdr_inline_pages(&req->rq_rcv_buf, replen,
>  			 args->pages, args->pgbase, count);
> +	req->rq_rcv_buf.page_is_bulk = 1;
>  	return 0;
>  }
>  
> Index: linux-2.6.22/fs/nfs/nfs3xdr.c
> ===================================================================
> --- linux-2.6.22.orig/fs/nfs/nfs3xdr.c
> +++ linux-2.6.22/fs/nfs/nfs3xdr.c
> @@ -346,6 +346,7 @@ nfs3_xdr_readargs(struct rpc_rqst *req, 
>  	replen = (RPC_REPHDRSIZE + auth->au_rslack + NFS3_readres_sz) << 2;
>  	xdr_inline_pages(&req->rq_rcv_buf, replen,
>  			 args->pages, args->pgbase, count);
> +	req->rq_rcv_buf.page_is_bulk = 1;
>  	return 0;
>  }
>  
> Index: linux-2.6.22/fs/nfs/nfs4xdr.c
> ===================================================================
> --- linux-2.6.22.orig/fs/nfs/nfs4xdr.c
> +++ linux-2.6.22/fs/nfs/nfs4xdr.c
> @@ -1857,6 +1857,7 @@ static int nfs4_xdr_enc_read(struct rpc_
>  	replen = (RPC_REPHDRSIZE + auth->au_rslack + NFS4_dec_read_sz) << 2;
>  	xdr_inline_pages(&req->rq_rcv_buf, replen,
>  			 args->pages, args->pgbase, args->count);
> +	req->rq_rcv_buf.page_is_bulk = 1;
>  out:
>  	return status;
>  }
> Index: linux-2.6.22/include/linux/sunrpc/xdr.h
> ===================================================================
> --- linux-2.6.22.orig/include/linux/sunrpc/xdr.h
> +++ linux-2.6.22/include/linux/sunrpc/xdr.h
> @@ -70,7 +70,8 @@ struct xdr_buf {
>  
>  	struct page **	pages;		/* Array of contiguous pages */
>  	unsigned int	page_base,	/* Start of page data */
> -			page_len;	/* Length of page data */
> +			page_len,	/* Length of page data */
> +			page_is_bulk;	/* Page(s) hold bulk data only */
>  
>  	unsigned int	buflen,		/* Total length of storage buffer */
>  			len;		/* Length of XDR encoded message */
> Index: linux-2.6.22/net/sunrpc/clnt.c
> ===================================================================
> --- linux-2.6.22.orig/net/sunrpc/clnt.c
> +++ linux-2.6.22/net/sunrpc/clnt.c
> @@ -871,6 +871,7 @@ rpc_xdr_buf_init(struct xdr_buf *buf, vo
>  	buf->head[0].iov_len = len;
>  	buf->tail[0].iov_len = 0;
>  	buf->page_len = 0;
> +	buf->page_is_bulk = 0;
>  	buf->len = 0;
>  	buf->buflen = len;
>  }
> Index: linux-2.6.22/net/sunrpc/xprtrdma/rpc_rdma.c
> ===================================================================
> --- linux-2.6.22.orig/net/sunrpc/xprtrdma/rpc_rdma.c
> +++ linux-2.6.22/net/sunrpc/xprtrdma/rpc_rdma.c
> @@ -47,10 +47,6 @@
>  
>  #include "xprt_rdma.h"
>  
> -#include <linux/nfs2.h>
> -#include <linux/nfs3.h>
> -#include <linux/nfs4.h>
> -
>  #include <linux/highmem.h>
>  
>  #ifdef RPC_DEBUG
> @@ -351,37 +347,6 @@ rpcrdma_inline_pullup(struct rpc_rqst *r
>  }
>  
>  /*
> - * Totally imperfect, temporary attempt to detect nfs reads...
> - * e.g. establish a hint via xdr_inline_pages, etc.
> - */
> -static int
> -is_nfs_read(struct rpc_rqst *rqst)
> -{
> -	u32 *p;
> -
> -	if (rqst->rq_task->tk_client->cl_prog != NFS_PROGRAM)
> -		return 0;
> -	switch (rqst->rq_task->tk_client->cl_vers) {
> -	case 4:
> -		/* Must dig into the COMPOUND. */
> -		/* Back up from the end of what a read request would be */
> -		/* PUTFH, fh, OP_READ, stateid(16), offset(8), count(4) */
> -		p = (u32 *)(rqst->rq_snd_buf.head[0].iov_base +
> -			    rqst->rq_snd_buf.head[0].iov_len);
> -		/* test read and count */
> -		return (rqst->rq_snd_buf.head[0].iov_len > 40 &&
> -			p[-8] == __constant_htonl(OP_READ) &&
> -
> -			p[-1] == htonl(rqst->rq_rcv_buf.page_len));
> -	case 3:
> -		return rqst->rq_task->tk_msg.rpc_proc->p_proc == NFS3PROC_READ;
> -	case 2:
> -		return rqst->rq_task->tk_msg.rpc_proc->p_proc == NFSPROC_READ;
> -	}
> -	return 0;
> -}
> -
> -/*
>   * Marshal a request: the primary job of this routine is to choose
>   * the transfer modes. See comments below.
>   *
> @@ -443,7 +408,7 @@ rpcrdma_marshal_req(struct rpc_rqst *rqs
>  		wtype = rpcrdma_noch;
>  	else if (rqst->rq_rcv_buf.page_len == 0)
>  		wtype = rpcrdma_replych;
> -	else if (is_nfs_read(rqst))
> +	else if (rqst->rq_rcv_buf.page_is_bulk)
>  		wtype = rpcrdma_writech;
>  	else
>  		wtype = rpcrdma_replych;

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 290 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 315 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >>  http://get.splunk.com/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs