Re: [PATCH 0/3] NFS: Disable READ_PLUS by default

From: Anna Schumaker <schumaker.anna@gmail.com>
To: Olga Kornievskaia <aglo@umich.edu>
Cc: Trond Myklebust <trondmy@hammerspace.com>,
	"tigran.mkrtchyan@desy.de" <tigran.mkrtchyan@desy.de>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 0/3] NFS: Disable READ_PLUS by default
Date: Wed, 9 Dec 2020 12:28:59 -0500	[thread overview]
Message-ID: <CAFX2JfnMpEfqZFUYWQD9rV0VCvyYAejNWbNMUvW44fvhBeGhBw@mail.gmail.com> (raw)
In-Reply-To: <CAN-5tyFUmQQeQQjHtmetvTN2rcJpf3KwPbhm+6TB_N33auirag@mail.gmail.com>

On Wed, Dec 9, 2020 at 12:22 PM Olga Kornievskaia <aglo@umich.edu> wrote:
>
> On Wed, Dec 9, 2020 at 12:12 PM Trond Myklebust <trondmy@hammerspace.com> 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@hammerspace.com> 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:

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@hammerspace.com
> >
> >
