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: <1624901943-20027-1-git-send-email-dwysocha@redhat.com>
 <1624901943-20027-3-git-send-email-dwysocha@redhat.com> <2561b4f973e14eba413f648b657a7945830af202.camel@hammerspace.com>
In-Reply-To: <2561b4f973e14eba413f648b657a7945830af202.camel@hammerspace.com>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Mon, 28 Jun 2021 16:00:48 -0400
Message-ID: <CALF+zOn7jTduKYRX_fpNjV+qRat+4qqocVLa=dMfQeUU+RmVaw@mail.gmail.com>
Subject: Re: [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when
 internal error occurs
To:     Trond Myklebust <trondmy@hammerspace.com>
Cc:     "anna.schumaker@netapp.com" <anna.schumaker@netapp.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jun 28, 2021 at 3:17 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Mon, 2021-06-28 at 13:39 -0400, Dave Wysochanski wrote:
> > A previous refactoring of nfs_readpage() might end up calling
> > wait_on_page_locked_killable() even if readpage_async_filler() failed
> > with an internal error and pg_error was non-zero (for example, if
> > nfs_create_request() failed).  In the case of an internal error,
> > skip over wait_on_page_locked_killable() as this is only needed
> > when the read is sent and an error occurs during completion handling.
> >
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > ---
> >  fs/nfs/read.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> > index 684a730f6670..b0680351df23 100644
> > --- a/fs/nfs/read.c
> > +++ b/fs/nfs/read.c
> > @@ -74,7 +74,7 @@ void nfs_pageio_init_read(struct
> > nfs_pageio_descriptor *pgio,
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_pageio_init_read);
> >
> > -static void nfs_pageio_complete_read(struct nfs_pageio_descriptor
> > *pgio)
> > +static int nfs_pageio_complete_read(struct nfs_pageio_descriptor
> > *pgio)
> >  {
> >         struct nfs_pgio_mirror *pgm;
> >         unsigned long npages;
> > @@ -88,6 +88,8 @@ static void nfs_pageio_complete_read(struct
> > nfs_pageio_descriptor *pgio)
> >         NFS_I(pgio->pg_inode)->read_io += pgm->pg_bytes_written;
> >         npages = (pgm->pg_bytes_written + PAGE_SIZE - 1) >>
> > PAGE_SHIFT;
> >         nfs_add_stats(pgio->pg_inode, NFSIOS_READPAGES, npages);
> > +
> > +       return pgio->pg_error < 0 ? pgio->pg_error : 0;
> >  }
> >
> >
> > @@ -373,16 +375,17 @@ int nfs_readpage(struct file *file, struct page
> > *page)
> >                              &nfs_async_read_completion_ops);
> >
> >         ret = readpage_async_filler(&desc, page);
> > +       if (ret)
> > +               goto out;
> >
> > -       if (!ret)
>
> Can't this patch basically be reduced to the above 2 changes? The rest
> appears just to be shifting code around. I'm seeing nothing in the
> remaining patches that actually depends on nfs_pageio_complete_read()
> returning a value.
>

Originally I was thinking there was a benefit to having
nfs_pageio_complete_read return a success/failure
similar to readpage_async_filler() which is why I moved
it.

You mean just this right?  If so, yes I agree this would be a minimal patch.
Want this as a v2?

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 684a730f6670..eb390eb618b3 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -373,10 +373,10 @@ int nfs_readpage(struct file *file, struct page *page)
                             &nfs_async_read_completion_ops);

        ret = readpage_async_filler(&desc, page);
+       if (ret)
+               goto out;

-       if (!ret)
-               nfs_pageio_complete_read(&desc.pgio);
-
+       nfs_pageio_complete_read(&desc.pgio);
        ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0;
        if (!ret) {
                ret = wait_on_page_locked_killable(page);