[PATCH 0/4] Fix a few error paths in nfs_readpage and fscache

[PATCH 0/4] Fix a few error paths in nfs_readpage and fscache

[Dave Wysochanski]

A prior patchset refactored some nfs read code paths but introduced a
few problems in some error paths.  This patcheset cleans up these
error paths specifically the following commit:
1e83b173b266 ("NFS: Add nfs_pageio_complete_read() and remove nfs_readpage_async()")

The last patch fixes a bug introduced in the above commit, and when
fscache is enabled and an unlikely error occurs.  The first 3 patches
are refactorings / cleanups and while they do not fix any actual bug
to my knowledge, they are required for the 4th patch.

NOTE: The first patch was submitted prior as a solo patch[1], it is
included here unchanged from that submission.
[1] https://marc.info/?l=linux-nfs&m=162454885107812&w=2

Testing
=======
I applied this series on top of Trond's testing tree based on 5.13-rc6
and at:
159dd33001be Merge branch 'sysfs-devel'

scripts/checkpatch.pl was run on all patches and no warnings or errors.

For unit tests of these specific code paths I created custom error injection
to induce failures.

For regression, I ran iterations of xfstests with different servers and all NFS
versions (4.2, 4.1, 4.0, 3), including pNFS (flexfiles,filelayout), and
didn't see any new failures.


Dave Wysochanski (4):
  NFS: Remove unnecessary inode parameter from
    nfs_pageio_complete_read()
  NFS: Ensure nfs_readpage does not wait an internal error occurs
  NFS: Allow internal use of read structs and functions
  NFS: Fix fscache read from NFS after cache error

 fs/nfs/fscache.c  | 14 ++++++++++++--
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/read.c     | 39 ++++++++++++++++++---------------------
 3 files changed, 37 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] NFS: Remove unnecessary inode parameter from nfs_pageio_complete_read()

[Dave Wysochanski]

Simplify nfs_pageio_complete_read() by using the inode pointer saved
inside nfs_pageio_descriptor.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/read.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index d2b6dce1f99f..684a730f6670 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -74,8 +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,
-				     struct inode *inode)
+static void nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio)
 {
 	struct nfs_pgio_mirror *pgm;
 	unsigned long npages;
@@ -86,9 +85,9 @@ static void nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio,
 	WARN_ON_ONCE(pgio->pg_mirror_count != 1);
 
 	pgm = &pgio->pg_mirrors[0];
-	NFS_I(inode)->read_io += pgm->pg_bytes_written;
+	NFS_I(pgio->pg_inode)->read_io += pgm->pg_bytes_written;
 	npages = (pgm->pg_bytes_written + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	nfs_add_stats(inode, NFSIOS_READPAGES, npages);
+	nfs_add_stats(pgio->pg_inode, NFSIOS_READPAGES, npages);
 }
 
 
@@ -376,7 +375,7 @@ int nfs_readpage(struct file *file, struct page *page)
 	ret = readpage_async_filler(&desc, page);
 
 	if (!ret)
-		nfs_pageio_complete_read(&desc.pgio, inode);
+		nfs_pageio_complete_read(&desc.pgio);
 
 	ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0;
 	if (!ret) {
@@ -430,7 +429,7 @@ int nfs_readpages(struct file *file, struct address_space *mapping,
 
 	ret = read_cache_pages(mapping, pages, readpage_async_filler, &desc);
 
-	nfs_pageio_complete_read(&desc.pgio, inode);
+	nfs_pageio_complete_read(&desc.pgio);
 
 read_complete:
 	put_nfs_open_context(desc.ctx);
-- 
1.8.3.1

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

[Dave Wysochanski]

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)
-		nfs_pageio_complete_read(&desc.pgio);
+	ret = nfs_pageio_complete_read(&desc.pgio);
+	if (ret)
+		goto out;
+
+	ret = wait_on_page_locked_killable(page);
+	if (!PageUptodate(page) && !ret)
+		ret = xchg(&desc.ctx->error, 0);
 
-	ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0;
-	if (!ret) {
-		ret = wait_on_page_locked_killable(page);
-		if (!PageUptodate(page) && !ret)
-			ret = xchg(&desc.ctx->error, 0);
-	}
 out:
 	put_nfs_open_context(desc.ctx);
 	return ret;
-- 
1.8.3.1

[PATCH 3/4] NFS: Allow internal use of read structs and functions

[Dave Wysochanski]

The conversion of the NFS read paths to the new fscache API
will require use of a few read structs and functions,
so move these declarations as required.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
---
 fs/nfs/internal.h |  7 +++++++
 fs/nfs/read.c     | 13 ++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
index a36af04188c2..f9f6c6a6370f 100644
--- a/fs/nfs/internal.h
+++ b/fs/nfs/internal.h
@@ -463,9 +463,16 @@ extern char *nfs_path(char **p, struct dentry *dentry,
 
 struct nfs_pgio_completion_ops;
 /* read.c */
+extern const struct nfs_pgio_completion_ops nfs_async_read_completion_ops;
 extern void nfs_pageio_init_read(struct nfs_pageio_descriptor *pgio,
 			struct inode *inode, bool force_mds,
 			const struct nfs_pgio_completion_ops *compl_ops);
+struct nfs_readdesc {
+	struct nfs_pageio_descriptor pgio;
+	struct nfs_open_context *ctx;
+};
+extern int readpage_async_filler(void *data, struct page *page);
+extern int nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio);
 extern void nfs_read_prepare(struct rpc_task *task, void *calldata);
 extern void nfs_pageio_reset_read_mds(struct nfs_pageio_descriptor *pgio);
 
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index b0680351df23..a0b4ce6893a8 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -30,7 +30,7 @@
 
 #define NFSDBG_FACILITY		NFSDBG_PAGECACHE
 
-static const struct nfs_pgio_completion_ops nfs_async_read_completion_ops;
+const struct nfs_pgio_completion_ops nfs_async_read_completion_ops;
 static const struct nfs_rw_ops nfs_rw_read_ops;
 
 static struct kmem_cache *nfs_rdata_cachep;
@@ -74,7 +74,7 @@ void nfs_pageio_init_read(struct nfs_pageio_descriptor *pgio,
 }
 EXPORT_SYMBOL_GPL(nfs_pageio_init_read);
 
-static int nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio)
+int nfs_pageio_complete_read(struct nfs_pageio_descriptor *pgio)
 {
 	struct nfs_pgio_mirror *pgm;
 	unsigned long npages;
@@ -133,11 +133,6 @@ static void nfs_readpage_release(struct nfs_page *req, int error)
 	nfs_release_request(req);
 }
 
-struct nfs_readdesc {
-	struct nfs_pageio_descriptor pgio;
-	struct nfs_open_context *ctx;
-};
-
 static void nfs_page_group_set_uptodate(struct nfs_page *req)
 {
 	if (nfs_page_group_sync_on_bit(req, PG_UPTODATE))
@@ -216,7 +211,7 @@ static void nfs_initiate_read(struct nfs_pgio_header *hdr,
 	}
 }
 
-static const struct nfs_pgio_completion_ops nfs_async_read_completion_ops = {
+const struct nfs_pgio_completion_ops nfs_async_read_completion_ops = {
 	.error_cleanup = nfs_async_read_error,
 	.completion = nfs_read_completion,
 };
@@ -291,7 +286,7 @@ static void nfs_readpage_result(struct rpc_task *task,
 		nfs_readpage_retry(task, hdr);
 }
 
-static int
+int
 readpage_async_filler(void *data, struct page *page)
 {
 	struct nfs_readdesc *desc = data;
-- 
1.8.3.1

[PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Dave Wysochanski]

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;
+
+		nfs_pageio_complete_read(&desc.pgio);
 	}
 }
 
-- 
1.8.3.1

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

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?

> +
> +               nfs_pageio_complete_read(&desc.pgio);
>         }
>  }
>  

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

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

[Trond Myklebust]

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.

> -               nfs_pageio_complete_read(&desc.pgio);
> +       ret = nfs_pageio_complete_read(&desc.pgio);
> +       if (ret)
> +               goto out;
> +
> +       ret = wait_on_page_locked_killable(page);
> +       if (!PageUptodate(page) && !ret)
> +               ret = xchg(&desc.ctx->error, 0);
>  
> -       ret = desc.pgio.pg_error < 0 ? desc.pgio.pg_error : 0;
> -       if (!ret) {
> -               ret = wait_on_page_locked_killable(page);
> -               if (!PageUptodate(page) && !ret)
> -                       ret = xchg(&desc.ctx->error, 0);
> -       }
>  out:
>         put_nfs_open_context(desc.ctx);
>         return ret;

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

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

[David Wysochanski]

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);

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[David Wysochanski]

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?
>

Because the context of the process calling nfs_readpage() is already
gone - this is called from keventd context coming up from fscache.
So if fscache fails, and then we retry here, and that fails, what is left
to be done?  This _is_ the retry code.  I guess I'm not following.  Do you
want the process to wait inside nfs_readpage() until fscache calls
nfs_readpage_from_fscache_complete?  If so, that would be new as
we have not done this before - this patch was just putting back the hunk I
mistakenly removed in commit 1e83b173b266


nfs_readpage() -> nfs_readpage_from_fscache -> fscache_read_or_alloc_page
* nfs_readpage_from_fscache_complete() will be called by fscache when
IO completes or there's an error
* fscache_read_or_alloc_page() returns 0 saying we've submitted the BIO

nfs_readpage()
...
    if (!IS_SYNC(inode)) {
        ret = nfs_readpage_from_fscache(desc.ctx, inode, page);
        if (ret == 0)
            goto out;
    }


int __nfs_readpage_from_fscache(struct nfs_open_context *ctx,
                struct inode *inode, struct page *page)
{
...
    ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
                     page,
                     nfs_readpage_from_fscache_complete,
                     ctx,
                     GFP_KERNEL);

   switch (ret) {
    case 0: /* read BIO submitted (page in fscache) */
        dfprintk(FSCACHE,
             "NFS:    readpage_from_fscache: BIO submitted\n");
        nfs_inc_fscache_stats(inode, NFSIOS_FSCACHE_PAGES_READ_OK);
        return ret;




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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[David Wysochanski]

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
>
>

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> 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
> > 
> > 
> 

Yes, please. This avoids that duplication of NFS read code in the
fscache layer.

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

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

[Trond Myklebust]

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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[David Wysochanski]

On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > 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
> > >
> > >
> >
>
> Yes, please. This avoids that duplication of NFS read code in the
> fscache layer.
>

If you mean patch 4 we still need that - I don't see anyway to
avoid it.  The above just will make the fscache enabled
path waits for the IO to complete, same as the non-fscache case.

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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > > 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
> > > > 
> > > > 
> > > 
> > 
> > Yes, please. This avoids that duplication of NFS read code in the
> > fscache layer.
> > 
> 
> If you mean patch 4 we still need that - I don't see anyway to
> avoid it.  The above just will make the fscache enabled
> path waits for the IO to complete, same as the non-fscache case.
> 

With the above, you can simplify patch 4/4 to just make the page unlock
unconditional on the error, no?

i.e.
	if (!error)
		SetPageUptodate(page);
	unlock_page(page);

End result: the client just does the same check as before and let's the
vfs/mm decide based on the status of the PG_uptodate flag what to do
next. I'm assuming that a retry won't cause fscache to do another bio
attempt?

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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[David Wysochanski]

On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > >
> > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > > > 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
> > > > >
> > > > >
> > > >
> > >
> > > Yes, please. This avoids that duplication of NFS read code in the
> > > fscache layer.
> > >
> >
> > If you mean patch 4 we still need that - I don't see anyway to
> > avoid it.  The above just will make the fscache enabled
> > path waits for the IO to complete, same as the non-fscache case.
> >
>
> With the above, you can simplify patch 4/4 to just make the page unlock
> unconditional on the error, no?
>
> i.e.
>         if (!error)
>                 SetPageUptodate(page);
>         unlock_page(page);
>
> End result: the client just does the same check as before and let's the
> vfs/mm decide based on the status of the PG_uptodate flag what to do
> next. I'm assuming that a retry won't cause fscache to do another bio
> attempt?
>

Yes I think you're right and I'm following - let me test it and I'll send a v2.
Then we can drop patch #3 right?

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

On Tue, 2021-06-29 at 05:17 -0400, David Wysochanski wrote:
> On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> > > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > > > > 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
> > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > > Yes, please. This avoids that duplication of NFS read code in
> > > > the
> > > > fscache layer.
> > > > 
> > > 
> > > If you mean patch 4 we still need that - I don't see anyway to
> > > avoid it.  The above just will make the fscache enabled
> > > path waits for the IO to complete, same as the non-fscache case.
> > > 
> > 
> > With the above, you can simplify patch 4/4 to just make the page
> > unlock
> > unconditional on the error, no?
> > 
> > i.e.
> >         if (!error)
> >                 SetPageUptodate(page);
> >         unlock_page(page);
> > 
> > End result: the client just does the same check as before and let's
> > the
> > vfs/mm decide based on the status of the PG_uptodate flag what to
> > do
> > next. I'm assuming that a retry won't cause fscache to do another
> > bio
> > attempt?
> > 
> 
> Yes I think you're right and I'm following - let me test it and I'll
> send a v2.
> Then we can drop patch #3 right?
> 
Sounds good. Thanks Dave!

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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[David Wysochanski]

On Tue, Jun 29, 2021 at 8:46 AM Trond Myklebust <trondmy@hammerspace.com> wrote:
>
> On Tue, 2021-06-29 at 05:17 -0400, David Wysochanski wrote:
> > On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > >
> > > On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> > > > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > >
> > > > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > > > > > 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
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > > Yes, please. This avoids that duplication of NFS read code in
> > > > > the
> > > > > fscache layer.
> > > > >
> > > >
> > > > If you mean patch 4 we still need that - I don't see anyway to
> > > > avoid it.  The above just will make the fscache enabled
> > > > path waits for the IO to complete, same as the non-fscache case.
> > > >
> > >
> > > With the above, you can simplify patch 4/4 to just make the page
> > > unlock
> > > unconditional on the error, no?
> > >
> > > i.e.
> > >         if (!error)
> > >                 SetPageUptodate(page);
> > >         unlock_page(page);
> > >
> > > End result: the client just does the same check as before and let's
> > > the
> > > vfs/mm decide based on the status of the PG_uptodate flag what to
> > > do
> > > next. I'm assuming that a retry won't cause fscache to do another
> > > bio
> > > attempt?
> > >
> >
> > Yes I think you're right and I'm following - let me test it and I'll
> > send a v2.
> > Then we can drop patch #3 right?
> >
> Sounds good. Thanks Dave!
>

This approach works but it differs from the original when an fscache
error occurs.
The original (see below) would call back into NFS to read from the server, but
now we just let the VM handle it.  The VM will re-issue the read, but
will go back into
fscache again (because it's enabled), which may fail again.

If you're ok with that limitation, I'll send the patch.  I tried an
alternative patch on
top to disable fscache on the inode if an error occurs - that way the
reissue from
the VM should go to the NFS server like the original.  But so far I
got a deadlock
in fscache when I try that so it didn't work.  If you want I can try
to work on disable
for another patch but it may take longer.

Original (before commit 1e83b173b266)
static void nfs_readpage_from_fscache_complete(struct page *page,
...
        if (!error) {
                SetPageUptodate(page);
                unlock_page(page);
        } else {
                error = nfs_readpage_async(context, page->mapping->host, page);
                if (error)
                        unlock_page(page);
        }


Patched with above idea

diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index c4c021c6ebbd..dca7ce676b1d 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -385,12 +385,11 @@ static void
nfs_readpage_from_fscache_complete(struct page *page,
                 "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
+       /* if the read completes with an error, unlock the page and let
         * the VM reissue the readpage */
-       if (!error) {
+       if (!error)
                SetPageUptodate(page);
-               unlock_page(page);
-       }
+       unlock_page(page);
 }

 /*
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index eb390eb618b3..9f39e0a1a38b 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -362,13 +362,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);

@@ -378,6 +378,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)



Patched with above plus an attempt at disabling fscache on error
(hangs in fscache disabling)

commit cf88e94e941e718156e68e11dbff10ca0a90bbad
Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Tue Jun 29 08:39:44 2021 -0400

    NFS: Disable fscache on an inode if we get an error when reading a
page from fscache

    If fscache is enabled, and a read from fscache completes with an error,
    disable fscache and unlock the page.  The VM will then re-issue the
    readpage and it should then be sent to the NFS server as a fallback.

    Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index cfa6e8c1b5a4..ef02bdd2b775 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -415,10 +415,12 @@ static void
nfs_readpage_from_fscache_complete(struct page *page,
                 "NFS: readpage_from_fscache_complete (0x%p/0x%p/%d)\n",
                 page, context, error);

-       /* if the read completes with an error, unlock the page and let
-        * the VM reissue the readpage */
+       /* if the read completes with an error, disable caching on the inode,
+        * unlock the page, and let the VM reissue the readpage to the server */
        if (!error)
                SetPageUptodate(page);
+       else
+               nfs_fscache_enable_inode(page->mapping->host, 0);
        unlock_page(page);
 }


@@ -286,6 +286,29 @@ static bool nfs_fscache_can_enable(void *data)
        return !inode_is_open_for_write(inode);
 }

+
+void nfs_fscache_enable_inode(struct inode *inode, int enable)
+{
+       struct nfs_fscache_inode_auxdata auxdata;
+       struct nfs_inode *nfsi = NFS_I(inode);
+       struct fscache_cookie *cookie = nfs_i_fscache(inode);
+
+       dfprintk(FSCACHE, "NFS: nfsi 0x%p %s cache\n", nfsi,
+                enable ? "enabling" : "disabling");
+
+       nfs_fscache_update_auxdata(&auxdata, nfsi);
+       if (enable) {
+               fscache_enable_cookie(cookie, &auxdata, nfsi->vfs_inode.i_size,
+                                     nfs_fscache_can_enable, inode);
+               if (fscache_cookie_enabled(cookie))
+                       set_bit(NFS_INO_FSCACHE, &NFS_I(inode)->flags);
+       } else {
+               clear_bit(NFS_INO_FSCACHE, &nfsi->flags);
+               fscache_disable_cookie(cookie, &auxdata, true);
+               fscache_uncache_all_inode_pages(cookie, inode);
+       }
+}
+

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

On Tue, 2021-06-29 at 09:20 -0400, David Wysochanski wrote:
> On Tue, Jun 29, 2021 at 8:46 AM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Tue, 2021-06-29 at 05:17 -0400, David Wysochanski wrote:
> > > On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> > > > > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > > > > <trondmy@hammerspace.com> wrote:
> > > > > > 
> > > > > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > > > > > > 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
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > Yes, please. This avoids that duplication of NFS read code
> > > > > > in
> > > > > > the
> > > > > > fscache layer.
> > > > > > 
> > > > > 
> > > > > If you mean patch 4 we still need that - I don't see anyway
> > > > > to
> > > > > avoid it.  The above just will make the fscache enabled
> > > > > path waits for the IO to complete, same as the non-fscache
> > > > > case.
> > > > > 
> > > > 
> > > > With the above, you can simplify patch 4/4 to just make the
> > > > page
> > > > unlock
> > > > unconditional on the error, no?
> > > > 
> > > > i.e.
> > > >         if (!error)
> > > >                 SetPageUptodate(page);
> > > >         unlock_page(page);
> > > > 
> > > > End result: the client just does the same check as before and
> > > > let's
> > > > the
> > > > vfs/mm decide based on the status of the PG_uptodate flag what
> > > > to
> > > > do
> > > > next. I'm assuming that a retry won't cause fscache to do
> > > > another
> > > > bio
> > > > attempt?
> > > > 
> > > 
> > > Yes I think you're right and I'm following - let me test it and
> > > I'll
> > > send a v2.
> > > Then we can drop patch #3 right?
> > > 
> > Sounds good. Thanks Dave!
> > 
> 
> This approach works but it differs from the original when an fscache
> error occurs.
> The original (see below) would call back into NFS to read from the
> server, but
> now we just let the VM handle it.  The VM will re-issue the read, but
> will go back into
> fscache again (because it's enabled), which may fail again.

How about marking the page on failure, then? I don't believe we
currently use PG_owner_priv_1 (a.k.a. PageOwnerPriv1, PageChecked,
PagePinned, PageForeign, PageSwapCache, PageXenRemapped) for anything
and according to legend it is supposed to be usable by the fs for page
cache pages.

So what say we use SetPageChecked() to mark the page as having failed
retrieval from fscache?


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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[David Wysochanski]

On Tue, Jun 29, 2021 at 10:54 AM Trond Myklebust
<trondmy@hammerspace.com> wrote:
>
> On Tue, 2021-06-29 at 09:20 -0400, David Wysochanski wrote:
> > On Tue, Jun 29, 2021 at 8:46 AM Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > >
> > > On Tue, 2021-06-29 at 05:17 -0400, David Wysochanski wrote:
> > > > On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > >
> > > > > On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> > > > > > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > >
> > > > > > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski wrote:
> > > > > > > > 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
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > Yes, please. This avoids that duplication of NFS read code
> > > > > > > in
> > > > > > > the
> > > > > > > fscache layer.
> > > > > > >
> > > > > >
> > > > > > If you mean patch 4 we still need that - I don't see anyway
> > > > > > to
> > > > > > avoid it.  The above just will make the fscache enabled
> > > > > > path waits for the IO to complete, same as the non-fscache
> > > > > > case.
> > > > > >
> > > > >
> > > > > With the above, you can simplify patch 4/4 to just make the
> > > > > page
> > > > > unlock
> > > > > unconditional on the error, no?
> > > > >
> > > > > i.e.
> > > > >         if (!error)
> > > > >                 SetPageUptodate(page);
> > > > >         unlock_page(page);
> > > > >
> > > > > End result: the client just does the same check as before and
> > > > > let's
> > > > > the
> > > > > vfs/mm decide based on the status of the PG_uptodate flag what
> > > > > to
> > > > > do
> > > > > next. I'm assuming that a retry won't cause fscache to do
> > > > > another
> > > > > bio
> > > > > attempt?
> > > > >
> > > >
> > > > Yes I think you're right and I'm following - let me test it and
> > > > I'll
> > > > send a v2.
> > > > Then we can drop patch #3 right?
> > > >
> > > Sounds good. Thanks Dave!
> > >
> >
> > This approach works but it differs from the original when an fscache
> > error occurs.
> > The original (see below) would call back into NFS to read from the
> > server, but
> > now we just let the VM handle it.  The VM will re-issue the read, but
> > will go back into
> > fscache again (because it's enabled), which may fail again.
>
> How about marking the page on failure, then? I don't believe we
> currently use PG_owner_priv_1 (a.k.a. PageOwnerPriv1, PageChecked,
> PagePinned, PageForeign, PageSwapCache, PageXenRemapped) for anything
> and according to legend it is supposed to be usable by the fs for page
> cache pages.
>
> So what say we use SetPageChecked() to mark the page as having failed
> retrieval from fscache?
>

So this?  I confirm this patch on top of the one I just sent works.
Want me to merge them together and send a v3?

Author: Dave Wysochanski <dwysocha@redhat.com>
Date:   Tue Jun 29 11:10:15 2021 -0400

    NFS: Mark page with PG_checked if fscache IO completes in error

    If fscache is enabled and we try to read from fscache, but the
    IO fails, mark the page with PG_checked.  Then when the VM
    re-issues the IO, skip over fscache and just read from the server.

    Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
index 0966e147e973..687e98b08994 100644
--- a/fs/nfs/fscache.c
+++ b/fs/nfs/fscache.c
@@ -404,10 +404,12 @@ static void
nfs_readpage_from_fscache_complete(struct page *page,
                 "NFS: readpage_from_fscache_complete (0x%p/0x%p/%d)\n",
                 page, context, error);

-       /* if the read completes with an error, unlock the page and let
-        * the VM reissue the readpage */
+       /* if the read completes with an error, mark the page with PG_checked,
+        * unlock the page, and let the VM reissue the readpage */
        if (!error)
                SetPageUptodate(page);
+       else
+               SetPageChecked(page);
        unlock_page(page);
 }

@@ -423,6 +425,11 @@ int __nfs_readpage_from_fscache(struct
nfs_open_context *ctx,
                 "NFS: readpage_from_fscache(fsc:%p/p:%p(i:%lx f:%lx)/0x%p)\n",
                 nfs_i_fscache(inode), page, page->index, page->flags, inode);

+       if (PageChecked(page)) {
+               ClearPageChecked(page);
+               return 1;
+       }
+

        ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
                                         page,
                                         nfs_readpage_from_fscache_complete,

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

On Tue, 2021-06-29 at 11:29 -0400, David Wysochanski wrote:
> On Tue, Jun 29, 2021 at 10:54 AM Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > 
> > On Tue, 2021-06-29 at 09:20 -0400, David Wysochanski wrote:
> > > On Tue, Jun 29, 2021 at 8:46 AM Trond Myklebust
> > > <trondmy@hammerspace.com> wrote:
> > > > 
> > > > On Tue, 2021-06-29 at 05:17 -0400, David Wysochanski wrote:
> > > > > On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust
> > > > > <trondmy@hammerspace.com> wrote:
> > > > > > 
> > > > > > On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski wrote:
> > > > > > > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > 
> > > > > > > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski
> > > > > > > > wrote:
> > > > > > > > > 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
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Yes, please. This avoids that duplication of NFS read
> > > > > > > > code
> > > > > > > > in
> > > > > > > > the
> > > > > > > > fscache layer.
> > > > > > > > 
> > > > > > > 
> > > > > > > If you mean patch 4 we still need that - I don't see
> > > > > > > anyway
> > > > > > > to
> > > > > > > avoid it.  The above just will make the fscache enabled
> > > > > > > path waits for the IO to complete, same as the non-
> > > > > > > fscache
> > > > > > > case.
> > > > > > > 
> > > > > > 
> > > > > > With the above, you can simplify patch 4/4 to just make the
> > > > > > page
> > > > > > unlock
> > > > > > unconditional on the error, no?
> > > > > > 
> > > > > > i.e.
> > > > > >         if (!error)
> > > > > >                 SetPageUptodate(page);
> > > > > >         unlock_page(page);
> > > > > > 
> > > > > > End result: the client just does the same check as before
> > > > > > and
> > > > > > let's
> > > > > > the
> > > > > > vfs/mm decide based on the status of the PG_uptodate flag
> > > > > > what
> > > > > > to
> > > > > > do
> > > > > > next. I'm assuming that a retry won't cause fscache to do
> > > > > > another
> > > > > > bio
> > > > > > attempt?
> > > > > > 
> > > > > 
> > > > > Yes I think you're right and I'm following - let me test it
> > > > > and
> > > > > I'll
> > > > > send a v2.
> > > > > Then we can drop patch #3 right?
> > > > > 
> > > > Sounds good. Thanks Dave!
> > > > 
> > > 
> > > This approach works but it differs from the original when an
> > > fscache
> > > error occurs.
> > > The original (see below) would call back into NFS to read from
> > > the
> > > server, but
> > > now we just let the VM handle it.  The VM will re-issue the read,
> > > but
> > > will go back into
> > > fscache again (because it's enabled), which may fail again.
> > 
> > How about marking the page on failure, then? I don't believe we
> > currently use PG_owner_priv_1 (a.k.a. PageOwnerPriv1, PageChecked,
> > PagePinned, PageForeign, PageSwapCache, PageXenRemapped) for
> > anything
> > and according to legend it is supposed to be usable by the fs for
> > page
> > cache pages.
> > 
> > So what say we use SetPageChecked() to mark the page as having
> > failed
> > retrieval from fscache?
> > 
> 
> So this?  I confirm this patch on top of the one I just sent works.
> Want me to merge them together and send a v3?
> 
> Author: Dave Wysochanski <dwysocha@redhat.com>
> Date:   Tue Jun 29 11:10:15 2021 -0400
> 
>     NFS: Mark page with PG_checked if fscache IO completes in error
> 
>     If fscache is enabled and we try to read from fscache, but the
>     IO fails, mark the page with PG_checked.  Then when the VM
>     re-issues the IO, skip over fscache and just read from the
> server.
> 
>     Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> 
> diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> index 0966e147e973..687e98b08994 100644
> --- a/fs/nfs/fscache.c
> +++ b/fs/nfs/fscache.c
> @@ -404,10 +404,12 @@ static void
> nfs_readpage_from_fscache_complete(struct page *page,
>                  "NFS: readpage_from_fscache_complete
> (0x%p/0x%p/%d)\n",
>                  page, context, error);
> 
> -       /* if the read completes with an error, unlock the page and
> let
> -        * the VM reissue the readpage */
> +       /* if the read completes with an error, mark the page with
> PG_checked,
> +        * unlock the page, and let the VM reissue the readpage */
>         if (!error)
>                 SetPageUptodate(page);
> +       else
> +               SetPageChecked(page);
>         unlock_page(page);
>  }
> 
> @@ -423,6 +425,11 @@ int __nfs_readpage_from_fscache(struct
> nfs_open_context *ctx,
>                  "NFS: readpage_from_fscache(fsc:%p/p:%p(i:%lx
> f:%lx)/0x%p)\n",
>                  nfs_i_fscache(inode), page, page->index, page-
> >flags, inode);
> 
> +       if (PageChecked(page)) {
> +               ClearPageChecked(page)
> +               return 1;
> +       }
> +
> 
>         ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
>                                          page,
>                                         
> nfs_readpage_from_fscache_complete,
> 

Yes, but how about just changing the above to:

	if (PageChecked(page))
		return 1;
	SetPageChecked(page);

Then you can short-circuit all further checks in
__nfs_readpage_from_fscache() if they've already failed once.

Note that I don't think it is useful to clear PageChecked() once it has
been set. Once a call to nfs_readpage() succeeds, the page will need to
be evicted from the page cache before we can call nfs_readpage() on it
again.


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

Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error

[Trond Myklebust]

On Tue, 2021-06-29 at 11:50 -0400, Trond Myklebust wrote:
> On Tue, 2021-06-29 at 11:29 -0400, David Wysochanski wrote:
> > On Tue, Jun 29, 2021 at 10:54 AM Trond Myklebust
> > <trondmy@hammerspace.com> wrote:
> > > 
> > > On Tue, 2021-06-29 at 09:20 -0400, David Wysochanski wrote:
> > > > On Tue, Jun 29, 2021 at 8:46 AM Trond Myklebust
> > > > <trondmy@hammerspace.com> wrote:
> > > > > 
> > > > > On Tue, 2021-06-29 at 05:17 -0400, David Wysochanski wrote:
> > > > > > On Mon, Jun 28, 2021 at 8:39 PM Trond Myklebust
> > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > 
> > > > > > > On Mon, 2021-06-28 at 19:46 -0400, David Wysochanski
> > > > > > > wrote:
> > > > > > > > On Mon, Jun 28, 2021 at 5:59 PM Trond Myklebust
> > > > > > > > <trondmy@hammerspace.com> wrote:
> > > > > > > > > 
> > > > > > > > > On Mon, 2021-06-28 at 17:12 -0400, David Wysochanski
> > > > > > > > > wrote:
> > > > > > > > > > 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
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Yes, please. This avoids that duplication of NFS read
> > > > > > > > > code
> > > > > > > > > in
> > > > > > > > > the
> > > > > > > > > fscache layer.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > If you mean patch 4 we still need that - I don't see
> > > > > > > > anyway
> > > > > > > > to
> > > > > > > > avoid it.  The above just will make the fscache enabled
> > > > > > > > path waits for the IO to complete, same as the non-
> > > > > > > > fscache
> > > > > > > > case.
> > > > > > > > 
> > > > > > > 
> > > > > > > With the above, you can simplify patch 4/4 to just make
> > > > > > > the
> > > > > > > page
> > > > > > > unlock
> > > > > > > unconditional on the error, no?
> > > > > > > 
> > > > > > > i.e.
> > > > > > >         if (!error)
> > > > > > >                 SetPageUptodate(page);
> > > > > > >         unlock_page(page);
> > > > > > > 
> > > > > > > End result: the client just does the same check as before
> > > > > > > and
> > > > > > > let's
> > > > > > > the
> > > > > > > vfs/mm decide based on the status of the PG_uptodate flag
> > > > > > > what
> > > > > > > to
> > > > > > > do
> > > > > > > next. I'm assuming that a retry won't cause fscache to do
> > > > > > > another
> > > > > > > bio
> > > > > > > attempt?
> > > > > > > 
> > > > > > 
> > > > > > Yes I think you're right and I'm following - let me test it
> > > > > > and
> > > > > > I'll
> > > > > > send a v2.
> > > > > > Then we can drop patch #3 right?
> > > > > > 
> > > > > Sounds good. Thanks Dave!
> > > > > 
> > > > 
> > > > This approach works but it differs from the original when an
> > > > fscache
> > > > error occurs.
> > > > The original (see below) would call back into NFS to read from
> > > > the
> > > > server, but
> > > > now we just let the VM handle it.  The VM will re-issue the
> > > > read,
> > > > but
> > > > will go back into
> > > > fscache again (because it's enabled), which may fail again.
> > > 
> > > How about marking the page on failure, then? I don't believe we
> > > currently use PG_owner_priv_1 (a.k.a. PageOwnerPriv1,
> > > PageChecked,
> > > PagePinned, PageForeign, PageSwapCache, PageXenRemapped) for
> > > anything
> > > and according to legend it is supposed to be usable by the fs for
> > > page
> > > cache pages.
> > > 
> > > So what say we use SetPageChecked() to mark the page as having
> > > failed
> > > retrieval from fscache?
> > > 
> > 
> > So this?  I confirm this patch on top of the one I just sent works.
> > Want me to merge them together and send a v3?
> > 
> > Author: Dave Wysochanski <dwysocha@redhat.com>
> > Date:   Tue Jun 29 11:10:15 2021 -0400
> > 
> >     NFS: Mark page with PG_checked if fscache IO completes in error
> > 
> >     If fscache is enabled and we try to read from fscache, but the
> >     IO fails, mark the page with PG_checked.  Then when the VM
> >     re-issues the IO, skip over fscache and just read from the
> > server.
> > 
> >     Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
> > 
> > diff --git a/fs/nfs/fscache.c b/fs/nfs/fscache.c
> > index 0966e147e973..687e98b08994 100644
> > --- a/fs/nfs/fscache.c
> > +++ b/fs/nfs/fscache.c
> > @@ -404,10 +404,12 @@ static void
> > nfs_readpage_from_fscache_complete(struct page *page,
> >                  "NFS: readpage_from_fscache_complete
> > (0x%p/0x%p/%d)\n",
> >                  page, context, error);
> > 
> > -       /* if the read completes with an error, unlock the page and
> > let
> > -        * the VM reissue the readpage */
> > +       /* if the read completes with an error, mark the page with
> > PG_checked,
> > +        * unlock the page, and let the VM reissue the readpage */
> >         if (!error)
> >                 SetPageUptodate(page);
> > +       else
> > +               SetPageChecked(page);
> >         unlock_page(page);
> >  }
> > 
> > @@ -423,6 +425,11 @@ int __nfs_readpage_from_fscache(struct
> > nfs_open_context *ctx,
> >                  "NFS: readpage_from_fscache(fsc:%p/p:%p(i:%lx
> > f:%lx)/0x%p)\n",
> >                  nfs_i_fscache(inode), page, page->index, page-
> > > flags, inode);
> > 
> > +       if (PageChecked(page)) {
> > +               ClearPageChecked(page)
> > +               return 1;
> > +       }
> > +
> > 
> >         ret = fscache_read_or_alloc_page(nfs_i_fscache(inode),
> >                                          page,
> >                                         
> > nfs_readpage_from_fscache_complete,
> > 
> 
> Yes, but how about just changing the above to:
> 
>         if (PageChecked(page))
>                 return 1;
>         SetPageChecked(page);
> 
> Then you can short-circuit all further checks in
> __nfs_readpage_from_fscache() if they've already failed once.
> 
> Note that I don't think it is useful to clear PageChecked() once it
> has
> been set. Once a call to nfs_readpage() succeeds, the page will need
> to
> be evicted from the page cache before we can call nfs_readpage() on
> it
> again.
> 
> 

Oops. Never mind. The advantage of doing it as you do above is that
nfs_readpage_from_fscache_complete() is also called from
__nfs_readpages_from_fscache(). So let's stick with this patch...

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