Re: [PATCH v3 1/6] SUNRPC: Implement xdr_reserve_space_vec()

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

Hi Anna-

> On Aug 3, 2020, at 12:59 PM, schumaker.anna@gmail.com wrote:
> 
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
> 
> Reserving space for a large READ payload requires special handling when
> reserving space in the xdr buffer pages. One problem we can have is use
> of the scratch buffer, which is used to get a pointer to a contiguous
> region of data up to PAGE_SIZE. When using the scratch buffer, calls to
> xdr_commit_encode() shift the data to it's proper alignment in the xdr
> buffer. If we've reserved several pages in a vector, then this could
> potentially invalidate earlier pointers and result in incorrect READ
> data being sent to the client.
> 
> I get around this by looking at the amount of space left in the current
> page, and never reserve more than that for each entry in the read
> vector. This lets us place data directly where it needs to go in the
> buffer pages.

Nit: This appears to be a refactoring change that should be squashed
together with 2/6.


> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> include/linux/sunrpc/xdr.h |  2 ++
> net/sunrpc/xdr.c           | 45 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
> 
> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
> index 22c207b2425f..bac459584dd0 100644
> --- a/include/linux/sunrpc/xdr.h
> +++ b/include/linux/sunrpc/xdr.h
> @@ -234,6 +234,8 @@ typedef int	(*kxdrdproc_t)(struct rpc_rqst *rqstp, struct xdr_stream *xdr,
> extern void xdr_init_encode(struct xdr_stream *xdr, struct xdr_buf *buf,
> 			    __be32 *p, struct rpc_rqst *rqst);
> extern __be32 *xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes);
> +extern int xdr_reserve_space_vec(struct xdr_stream *xdr, struct kvec *vec,
> +		size_t nbytes);
> extern void xdr_commit_encode(struct xdr_stream *xdr);
> extern void xdr_truncate_encode(struct xdr_stream *xdr, size_t len);
> extern int xdr_restrict_buflen(struct xdr_stream *xdr, int newbuflen);
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index be11d672b5b9..6dfe5dc8b35f 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -648,6 +648,51 @@ __be32 * xdr_reserve_space(struct xdr_stream *xdr, size_t nbytes)
> }
> EXPORT_SYMBOL_GPL(xdr_reserve_space);
> 
> +
> +/**
> + * xdr_reserve_space_vec - Reserves a large amount of buffer space for sending
> + * @xdr: pointer to xdr_stream
> + * @vec: pointer to a kvec array
> + * @nbytes: number of bytes to reserve
> + *
> + * Reserves enough buffer space to encode 'nbytes' of data and stores the
> + * pointers in 'vec'. The size argument passed to xdr_reserve_space() is
> + * determined based on the number of bytes remaining in the current page to
> + * avoid invalidating iov_base pointers when xdr_commit_encode() is called.
> + */
> +int xdr_reserve_space_vec(struct xdr_stream *xdr, struct kvec *vec, size_t nbytes)
> +{
> +	int thislen;
> +	int v = 0;
> +	__be32 *p;
> +
> +	/*
> +	 * svcrdma requires every READ payload to start somewhere
> +	 * in xdr->pages.
> +	 */
> +	if (xdr->iov == xdr->buf->head) {
> +		xdr->iov = NULL;
> +		xdr->end = xdr->p;
> +	}
> +
> +	while (nbytes) {
> +		thislen = xdr->buf->page_len % PAGE_SIZE;
> +		thislen = min_t(size_t, nbytes, PAGE_SIZE - thislen);
> +
> +		p = xdr_reserve_space(xdr, thislen);
> +		if (!p)
> +			return -EIO;
> +
> +		vec[v].iov_base = p;
> +		vec[v].iov_len = thislen;
> +		v++;
> +		nbytes -= thislen;
> +	}
> +
> +	return v;
> +}
> +EXPORT_SYMBOL_GPL(xdr_reserve_space_vec);
> +
> /**
>  * xdr_truncate_encode - truncate an encode buffer
>  * @xdr: pointer to xdr_stream
> -- 
> 2.27.0
> 

--
Chuck Lever