| Message ID | 1624901943-20027-5-git-send-email-dwysocha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a few error paths in nfs_readpage and fscache | expand |
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); > } > } >
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 > >
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 > >
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.
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 > >
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?
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?
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!
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); + } +} +
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?
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,
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.
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...
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); } }
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(-)