Re: [PATCH RFC2 2/3] SUNRPC: Track current encode position in struct xdr_stream

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2020-01-31 at 15:41 -0500, Chuck Lever wrote:
> Maintain a byte-offset cursor in struct xdr_stream. This will be
> used in a subsequent patch.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4proc.c         |    1 +
>  include/linux/sunrpc/xdr.h |    1 +
>  net/sunrpc/xdr.c           |    2 ++
>  3 files changed, 4 insertions(+)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 4798667af647..9cd610485f7d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1904,6 +1904,7 @@ static void svcxdr_init_encode(struct svc_rqst
> *rqstp,
>  	xdr->iov = head;
>  	xdr->p   = head->iov_base + head->iov_len;
>  	xdr->end = head->iov_base + PAGE_SIZE - rqstp->rq_auth_slack;
> +	xdr->pos = head->iov_len;
>  	/* Tail and page_len should be zero at this point: */
>  	buf->len = buf->head[0].iov_len;
>  	xdr->scratch.iov_len = 0;
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index b41f34977995..2ca479b0708e 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -232,6 +232,7 @@ struct xdr_stream {
>  	struct kvec *iov;	/* pointer to the current kvec */
>  	struct kvec scratch;	/* Scratch buffer */
>  	struct page **page_ptr;	/* pointer to the current page */
> +	unsigned int pos;	/* current encode position */
>  	unsigned int nwords;	/* Remaining decode buffer length */

Is there any reason not to make this an anonymous union? 'pos' is used
only when encoding, and 'nwords' is only when decoding. Never the twain
shall meet...

>  
>  	struct rpc_rqst *rqst;	/* For debugging */
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index f3104be8ff5d..b8b78876a9bd 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -542,6 +542,7 @@ void xdr_init_encode(struct xdr_stream *xdr,
> struct xdr_buf *buf, __be32 *p,
>  		buf->len += len;
>  		iov->iov_len += len;
>  	}
> +	xdr->pos = iov->iov_len;
>  	xdr->rqst = rqst;
>  }
>  EXPORT_SYMBOL_GPL(xdr_init_encode);
> @@ -644,6 +645,7 @@ __be32 * xdr_reserve_space(struct xdr_stream
> *xdr, size_t nbytes)
>  	else
>  		xdr->buf->page_len += nbytes;
>  	xdr->buf->len += nbytes;
> +	xdr->pos += nbytes;
>  	return p;
>  }
>  EXPORT_SYMBOL_GPL(xdr_reserve_space);
> 

You should fix up xdr_encode_write() as well for consistency.


Now with all that said, why do we need both xdr->pos and xdr->buf->len? 
They should always hold the same value.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com