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-5-git-send-email-dwysocha@redhat.com> <efc373dd3f321f2f45e749a5edb383f2a11a7b78.camel@hammerspace.com>
In-Reply-To: <efc373dd3f321f2f45e749a5edb383f2a11a7b78.camel@hammerspace.com>
From:   David Wysochanski <dwysocha@redhat.com>
Date:   Mon, 28 Jun 2021 17:12:11 -0400
Message-ID: <CALF+zO=CFMYUGRG2m77XQy=LVVd-Zf_a+oQrJATymu-iqHRNtA@mail.gmail.com>
Subject: Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error
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:09 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Mon, 2021-06-28 at 13:39 -0400, Dave Wysochanski wrote:
> > Earlier commits refactored some NFS read code and removed
> > nfs_readpage_async(), but neglected to properly fixup
> > nfs_readpage_from_fscache_complete().  The code path is
> > only hit when something unusual occurs with the cachefiles
> > backing filesystem, such as an IO error or while a cookie
> > is being invalidated.
> >
> > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > ---
> >  fs/nfs/fscache.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> > index c4c021c6ebbd..d308cb7e1dd4 100644
> > --- a/fs/nfs/fscache.c
> > +++ b/fs/nfs/fscache.c
> > @@ -381,15 +381,25 @@ static void
> > nfs_readpage_from_fscache_complete(struct page *page,
> >                                                void *context,
> >                                                int error)
> >  {
> > +       struct nfs_readdesc desc;
> > +       struct inode *inode = page->mapping->host;
> > +
> >         dfprintk(FSCACHE,
> >                  "NFS: readpage_from_fscache_complete
> > (0x%p/0x%p/%d)\n",
> >                  page, context, error);
> >
> > -       /* if the read completes with an error, we just unlock the
> > page and let
> > -        * the VM reissue the readpage */
> >         if (!error) {
> >                 SetPageUptodate(page);
> >                 unlock_page(page);
> > +       } else {
> > +               desc.ctx = context;
> > +               nfs_pageio_init_read(&desc.pgio, inode, false,
> > +                                    &nfs_async_read_completion_ops);
> > +               error = readpage_async_filler(&desc, page);
> > +               if (error)
> > +                       return;
>
> This code path can clearly fail too. Why can we not fix this code to
> allow it to return that reported error so that we can handle the
> failure case in nfs_readpage() instead of dead-ending here?
>

Maybe the below patch is what you had in mind?  That way if fscache
is enabled, nfs_readpage() should behave the same way as if it's not,
for the case where an IO error occurs in the NFS read completion
path.

If we call into fscache and we get back that the IO has been submitted,
wait until it is completed, so we'll catch any IO errors in the read completion
path.  This does not solve the "catch the internal errors", IOW, the
ones that show up as pg_error, that will probably require copying
pg_error into nfs_open_context.error field.

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 78b9181e94ba..28e3318080e0 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -357,13 +357,13 @@ int nfs_readpage(struct file *file, struct page *page)
        } else
                desc.ctx = get_nfs_open_context(nfs_file_open_context(file));

+       xchg(&desc.ctx->error, 0);
        if (!IS_SYNC(inode)) {
                ret = nfs_readpage_from_fscache(desc.ctx, inode, page);
                if (ret == 0)
-                       goto out;
+                       goto out_wait;
        }

-       xchg(&desc.ctx->error, 0);
        nfs_pageio_init_read(&desc.pgio, inode, false,
                             &nfs_async_read_completion_ops);

@@ -373,6 +373,7 @@ int nfs_readpage(struct file *file, struct page *page)

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


> > +
> > +               nfs_pageio_complete_read(&desc.pgio);
> >         }
> >  }
> >
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>