On Wed, Dec 9, 2020 at 12:29 PM Anna Schumaker <schumaker.anna@xxxxxxxxx> wrote:
>
> On Wed, Dec 9, 2020 at 12:22 PM Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
> >
> > On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 2020-12-09 at 12:07 -0500, Olga Kornievskaia wrote:
> > > > On Wed, Dec 9, 2020 at 11:59 AM Trond Myklebust
> > > > <trondmy@xxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 2020-12-04 at 15:00 -0500, Olga Kornievskaia wrote:
> > > > > > I object to putting the disable patch in, I think we need to fix
> > > > > > the
> > > > > > problem.
> > > > >
> > > > > I can't see the problem is fixable in 5.10. There are way too many
> > > > > changes required, and we're in the middle of the week of the last -
> > > > > rc
> > > > > for 5.10. Furthermore, there are no regressions introduced by just
> > > > > disabling the functionality, because READ_PLUS has only just been
> > > > > merged in this release cycle.
> > > > >
> > > > > I therefore strongly suggest we just send [PATCH 1/3] NFS: Disable
> > > > > READ_PLUS by default and then fix the rest in 5.11.
> > > >
> > > > Sure, but shouldn't there be more ifdefs inside of the xdr code to
> > > > turn it off completely?
> > >
> > > AFAICT, those functions are not called by anything else, so as long as
> > > the READ_PLUS client functionality is disabled, they should be
> > > harmless.
> >
> > Is it benign that in the normal read path sunrpc will be calling a new
> > function of xdr_realign_pages()? Non readplus code didn't have it.
>
> It should be. All I did was pull out some code from xdr_align_pages()
> and put it into a new function. `git show --diff-algorithm=histogram`
> says this is what I did:
Ok sounds good then. I just wanted to double check.
> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> index 909920fab93b..d93bcad5ba9f 100644
> --- a/net/sunrpc/xdr.c
> +++ b/net/sunrpc/xdr.c
> @@ -997,10 +997,25 @@ __be32 * xdr_inline_decode(struct xdr_stream
> *xdr, size_t nbytes)
> }
> EXPORT_SYMBOL_GPL(xdr_inline_decode);
>
> +static void xdr_realign_pages(struct xdr_stream *xdr)
> +{
> + struct xdr_buf *buf = xdr->buf;
> + struct kvec *iov = buf->head;
> + unsigned int cur = xdr_stream_pos(xdr);
> + unsigned int copied, offset;
> +
> + /* Realign pages to current pointer position */
> + if (iov->iov_len > cur) {
> + offset = iov->iov_len - cur;
> + copied = xdr_shrink_bufhead(buf, offset);
> + trace_rpc_xdr_alignment(xdr, offset, copied);
> + xdr->nwords = XDR_QUADLEN(buf->len - cur);
> + }
> +}
> +
> static unsigned int xdr_align_pages(struct xdr_stream *xdr, unsigned int len)
> {
> struct xdr_buf *buf = xdr->buf;
> - struct kvec *iov;
> unsigned int nwords = XDR_QUADLEN(len);
> unsigned int cur = xdr_stream_pos(xdr);
> unsigned int copied, offset;
> @@ -1008,15 +1023,7 @@ static unsigned int xdr_align_pages(struct
> xdr_stream *xdr, unsigned int len)
> if (xdr->nwords == 0)
> return 0;
>
> - /* Realign pages to current pointer position */
> - iov = buf->head;
> - if (iov->iov_len > cur) {
> - offset = iov->iov_len - cur;
> - copied = xdr_shrink_bufhead(buf, offset);
> - trace_rpc_xdr_alignment(xdr, offset, copied);
> - xdr->nwords = XDR_QUADLEN(buf->len - cur);
> - }
> -
> + xdr_realign_pages(xdr);
> if (nwords > xdr->nwords) {
> nwords = xdr->nwords;
> len = nwords << 2;
>
>
> >
> > >
> > > --
> > > Trond Myklebust
> > > Linux NFS client maintainer, Hammerspace
> > > trond.myklebust@xxxxxxxxxxxxxxx
> > >
> > >
