Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [PATCH v3 1/6] SUNRPC: Implement xdr_reserve_space_vec()
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200803165954.1348263-2-Anna.Schumaker@Netapp.com>
Date:   Mon, 3 Aug 2020 15:19:14 -0400
Cc:     Bruce Fields <bfields@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Anna Schumaker <Anna.Schumaker@Netapp.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <DEB91DA9-0DCE-4C95-8B87-D8167AB57F65@oracle.com>
References: <20200803165954.1348263-1-Anna.Schumaker@Netapp.com>
 <20200803165954.1348263-2-Anna.Schumaker@Netapp.com>
To:     Anna Schumaker <schumaker.anna@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Anna-

> On Aug 3, 2020, at 12:59 PM, schumaker.anna@gmail.com wrote:
>=20
> From: Anna Schumaker <Anna.Schumaker@Netapp.com>
>=20
> 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.
>=20
> 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(+)
>=20
> 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);
>=20
> +
> +/**
> + * 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 =3D 0;
> +	__be32 *p;
> +
> +	/*
> +	 * svcrdma requires every READ payload to start somewhere
> +	 * in xdr->pages.
> +	 */
> +	if (xdr->iov =3D=3D xdr->buf->head) {
> +		xdr->iov =3D NULL;
> +		xdr->end =3D xdr->p;
> +	}
> +
> +	while (nbytes) {
> +		thislen =3D xdr->buf->page_len % PAGE_SIZE;
> +		thislen =3D min_t(size_t, nbytes, PAGE_SIZE - thislen);
> +
> +		p =3D xdr_reserve_space(xdr, thislen);
> +		if (!p)
> +			return -EIO;
> +
> +		vec[v].iov_base =3D p;
> +		vec[v].iov_len =3D thislen;
> +		v++;
> +		nbytes -=3D thislen;
> +	}
> +
> +	return v;
> +}
> +EXPORT_SYMBOL_GPL(xdr_reserve_space_vec);
> +
> /**
>  * xdr_truncate_encode - truncate an encode buffer
>  * @xdr: pointer to xdr_stream
> --=20
> 2.27.0
>=20

--
Chuck Lever