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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
