* [PATCH 0/4] Fix a few error paths in nfs_readpage and fscache @ 2021-06-28 17:38 Dave Wysochanski 2021-06-28 17:39 ` [PATCH 1/4] NFS: Remove unnecessary inode parameter from nfs_pageio_complete_read() Dave Wysochanski ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Dave Wysochanski @ 2021-06-28 17:38 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/4] NFS: Remove unnecessary inode parameter from nfs_pageio_complete_read() 2021-06-28 17:38 [PATCH 0/4] Fix a few error paths in nfs_readpage and fscache Dave Wysochanski @ 2021-06-28 17:39 ` Dave Wysochanski 2021-06-28 17:39 ` [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs Dave Wysochanski ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Dave Wysochanski @ 2021-06-28 17:39 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs 2021-06-28 17:38 [PATCH 0/4] Fix a few error paths in nfs_readpage and fscache Dave Wysochanski 2021-06-28 17:39 ` [PATCH 1/4] NFS: Remove unnecessary inode parameter from nfs_pageio_complete_read() Dave Wysochanski @ 2021-06-28 17:39 ` Dave Wysochanski 2021-06-28 19:17 ` Trond Myklebust 2021-06-28 17:39 ` [PATCH 3/4] NFS: Allow internal use of read structs and functions Dave Wysochanski 2021-06-28 17:39 ` [PATCH 4/4] NFS: Fix fscache read from NFS after cache error Dave Wysochanski 3 siblings, 1 reply; 21+ messages in thread From: Dave Wysochanski @ 2021-06-28 17:39 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs 2021-06-28 17:39 ` [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs Dave Wysochanski @ 2021-06-28 19:17 ` Trond Myklebust 2021-06-28 20:00 ` David Wysochanski 0 siblings, 1 reply; 21+ messages in thread From: Trond Myklebust @ 2021-06-28 19:17 UTC (permalink / raw) To: anna.schumaker, dwysocha; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs 2021-06-28 19:17 ` Trond Myklebust @ 2021-06-28 20:00 ` David Wysochanski 2021-06-28 22:00 ` Trond Myklebust 0 siblings, 1 reply; 21+ messages in thread From: David Wysochanski @ 2021-06-28 20:00 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna.schumaker, linux-nfs 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); ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs 2021-06-28 20:00 ` David Wysochanski @ 2021-06-28 22:00 ` Trond Myklebust 0 siblings, 0 replies; 21+ messages in thread From: Trond Myklebust @ 2021-06-28 22:00 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/4] NFS: Allow internal use of read structs and functions 2021-06-28 17:38 [PATCH 0/4] Fix a few error paths in nfs_readpage and fscache Dave Wysochanski 2021-06-28 17:39 ` [PATCH 1/4] NFS: Remove unnecessary inode parameter from nfs_pageio_complete_read() Dave Wysochanski 2021-06-28 17:39 ` [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs Dave Wysochanski @ 2021-06-28 17:39 ` Dave Wysochanski 2021-06-28 17:39 ` [PATCH 4/4] NFS: Fix fscache read from NFS after cache error Dave Wysochanski 3 siblings, 0 replies; 21+ messages in thread From: Dave Wysochanski @ 2021-06-28 17:39 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 17:38 [PATCH 0/4] Fix a few error paths in nfs_readpage and fscache Dave Wysochanski ` (2 preceding siblings ...) 2021-06-28 17:39 ` [PATCH 3/4] NFS: Allow internal use of read structs and functions Dave Wysochanski @ 2021-06-28 17:39 ` Dave Wysochanski 2021-06-28 19:09 ` Trond Myklebust 3 siblings, 1 reply; 21+ messages in thread From: Dave Wysochanski @ 2021-06-28 17:39 UTC (permalink / raw) To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 17:39 ` [PATCH 4/4] NFS: Fix fscache read from NFS after cache error Dave Wysochanski @ 2021-06-28 19:09 ` Trond Myklebust 2021-06-28 20:15 ` David Wysochanski 2021-06-28 21:12 ` David Wysochanski 0 siblings, 2 replies; 21+ messages in thread From: Trond Myklebust @ 2021-06-28 19:09 UTC (permalink / raw) To: anna.schumaker, dwysocha; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 19:09 ` Trond Myklebust @ 2021-06-28 20:15 ` David Wysochanski 2021-06-28 21:12 ` David Wysochanski 1 sibling, 0 replies; 21+ messages in thread From: David Wysochanski @ 2021-06-28 20:15 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna.schumaker, linux-nfs 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 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 19:09 ` Trond Myklebust 2021-06-28 20:15 ` David Wysochanski @ 2021-06-28 21:12 ` David Wysochanski 2021-06-28 21:59 ` Trond Myklebust 1 sibling, 1 reply; 21+ messages in thread From: David Wysochanski @ 2021-06-28 21:12 UTC (permalink / raw) To: Trond Myklebust; +Cc: anna.schumaker, linux-nfs 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 > > ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 21:12 ` David Wysochanski @ 2021-06-28 21:59 ` Trond Myklebust 2021-06-28 23:46 ` David Wysochanski 0 siblings, 1 reply; 21+ messages in thread From: Trond Myklebust @ 2021-06-28 21:59 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 21:59 ` Trond Myklebust @ 2021-06-28 23:46 ` David Wysochanski 2021-06-29 0:39 ` Trond Myklebust 0 siblings, 1 reply; 21+ messages in thread From: David Wysochanski @ 2021-06-28 23:46 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, anna.schumaker 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 > > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-28 23:46 ` David Wysochanski @ 2021-06-29 0:39 ` Trond Myklebust 2021-06-29 9:17 ` David Wysochanski 0 siblings, 1 reply; 21+ messages in thread From: Trond Myklebust @ 2021-06-29 0:39 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 0:39 ` Trond Myklebust @ 2021-06-29 9:17 ` David Wysochanski 2021-06-29 12:45 ` Trond Myklebust 0 siblings, 1 reply; 21+ messages in thread From: David Wysochanski @ 2021-06-29 9:17 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, anna.schumaker 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 9:17 ` David Wysochanski @ 2021-06-29 12:45 ` Trond Myklebust 2021-06-29 13:20 ` David Wysochanski 0 siblings, 1 reply; 21+ messages in thread From: Trond Myklebust @ 2021-06-29 12:45 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 12:45 ` Trond Myklebust @ 2021-06-29 13:20 ` David Wysochanski 2021-06-29 14:54 ` Trond Myklebust 0 siblings, 1 reply; 21+ messages in thread From: David Wysochanski @ 2021-06-29 13:20 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, anna.schumaker 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); + } +} + ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 13:20 ` David Wysochanski @ 2021-06-29 14:54 ` Trond Myklebust 2021-06-29 15:29 ` David Wysochanski 0 siblings, 1 reply; 21+ messages in thread From: Trond Myklebust @ 2021-06-29 14:54 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 14:54 ` Trond Myklebust @ 2021-06-29 15:29 ` David Wysochanski 2021-06-29 15:50 ` Trond Myklebust 0 siblings, 1 reply; 21+ messages in thread From: David Wysochanski @ 2021-06-29 15:29 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs, anna.schumaker 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, ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 15:29 ` David Wysochanski @ 2021-06-29 15:50 ` Trond Myklebust 2021-06-29 15:54 ` Trond Myklebust 0 siblings, 1 reply; 21+ messages in thread From: Trond Myklebust @ 2021-06-29 15:50 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] NFS: Fix fscache read from NFS after cache error 2021-06-29 15:50 ` Trond Myklebust @ 2021-06-29 15:54 ` Trond Myklebust 0 siblings, 0 replies; 21+ messages in thread From: Trond Myklebust @ 2021-06-29 15:54 UTC (permalink / raw) To: dwysocha; +Cc: linux-nfs, anna.schumaker 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-06-29 15:54 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-28 17:38 [PATCH 0/4] Fix a few error paths in nfs_readpage and fscache Dave Wysochanski 2021-06-28 17:39 ` [PATCH 1/4] NFS: Remove unnecessary inode parameter from nfs_pageio_complete_read() Dave Wysochanski 2021-06-28 17:39 ` [PATCH 2/4] NFS: Ensure nfs_readpage returns promptly when internal error occurs Dave Wysochanski 2021-06-28 19:17 ` Trond Myklebust 2021-06-28 20:00 ` David Wysochanski 2021-06-28 22:00 ` Trond Myklebust 2021-06-28 17:39 ` [PATCH 3/4] NFS: Allow internal use of read structs and functions Dave Wysochanski 2021-06-28 17:39 ` [PATCH 4/4] NFS: Fix fscache read from NFS after cache error Dave Wysochanski 2021-06-28 19:09 ` Trond Myklebust 2021-06-28 20:15 ` David Wysochanski 2021-06-28 21:12 ` David Wysochanski 2021-06-28 21:59 ` Trond Myklebust 2021-06-28 23:46 ` David Wysochanski 2021-06-29 0:39 ` Trond Myklebust 2021-06-29 9:17 ` David Wysochanski 2021-06-29 12:45 ` Trond Myklebust 2021-06-29 13:20 ` David Wysochanski 2021-06-29 14:54 ` Trond Myklebust 2021-06-29 15:29 ` David Wysochanski 2021-06-29 15:50 ` Trond Myklebust 2021-06-29 15:54 ` Trond Myklebust
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.