| Message ID | 1624901943-20027-3-git-send-email-dwysocha@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix a few error paths in nfs_readpage and fscache | expand |
On Mon, 2021-06-28 at 13:39 -0400, Dave Wysochanski wrote: > 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;
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);
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!
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;
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(-)