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;
MIME-Version: 1.0
References: <20200803165954.1348263-1-Anna.Schumaker@Netapp.com>
 <20200803165954.1348263-2-Anna.Schumaker@Netapp.com> <DEB91DA9-0DCE-4C95-8B87-D8167AB57F65@oracle.com>
In-Reply-To: <DEB91DA9-0DCE-4C95-8B87-D8167AB57F65@oracle.com>
From:   Anna Schumaker <schumaker.anna@gmail.com>
Date:   Mon, 3 Aug 2020 15:37:57 -0400
Message-ID: <CAFX2JfnObW2z1wPgvQK1_WceaKS-gcS5b1YqUgZGg1uxnLdU3w@mail.gmail.com>
Subject: Re: [PATCH v3 1/6] SUNRPC: Implement xdr_reserve_space_vec()
To:     Chuck Lever <chuck.lever@oracle.com>
Cc:     Bruce Fields <bfields@redhat.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Chuck,

On Mon, Aug 3, 2020 at 3:21 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>
> 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.

My default was to leave sunrpc and nfs changes as separate patches,
but I can squash them together if you want me to!

Anna
>
>
> > 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
>
>
>