> On Aug 3, 2020, at 3:37 PM, Anna Schumaker <schumaker.anna@xxxxxxxxx> wrote:
>
> Hi Chuck,
>
> On Mon, Aug 3, 2020 at 3:21 PM Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>>
>> Hi Anna-
>>
>>> On Aug 3, 2020, at 12:59 PM, schumaker.anna@xxxxxxxxx wrote:
>>>
>>> From: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
>>>
>>> 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.
>
> My default was to leave sunrpc and nfs changes as separate patches,
> but I can squash them together if you want me to!
IMO, in this case the rule about introducing and using a new helper
in the same patch takes precedence.
> Anna
>>
>>
>>> Signed-off-by: Anna Schumaker <Anna.Schumaker@xxxxxxxxxx>
>>> ---
>>> 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
--
Chuck Lever
