Re: [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs

[Trond Myklebust <trondmy@hammerspace.com>]

On Mon, 2021-06-28 at 16:00 -0400, David Wysochanski wrote:
> 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);
> 

Yep. This what what I had in mind. Thanks!

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com