On Wed, 2018-10-17 at 12:05 -0400, Benjamin Coddington wrote:
> We must check pg_error and call error_cleanup after any call to
> pg_doio.
> Currently, we are skipping the unlock of a page if we encounter an
> error in
> nfs_pageio_complete() before handing off the work to the RPC layer.
>
> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> ---
> fs/nfs/pagelist.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
> index cd3bc41ab68d..54c2bfc45a57 100644
> --- a/fs/nfs/pagelist.c
> +++ b/fs/nfs/pagelist.c
> @@ -1110,6 +1110,20 @@ static int
> nfs_pageio_add_request_mirror(struct nfs_pageio_descriptor *desc,
> return ret;
> }
>
> +static void nfs_pageio_error_cleanup(struct nfs_pageio_descriptor
> *desc)
> +{
> + u32 midx;
> + struct nfs_pgio_mirror *mirror;
> +
> + if (!desc->pg_error)
> + return;
> +
> + for (midx = 0; midx < desc->pg_mirror_count; midx++) {
> + mirror = &desc->pg_mirrors[midx];
> + desc->pg_completion_ops->error_cleanup(&mirror-
> >pg_list);
> + }
> +}
> +
> int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
> struct nfs_page *req)
> {
> @@ -1160,20 +1174,7 @@ int nfs_pageio_add_request(struct
> nfs_pageio_descriptor *desc,
> return 1;
>
> out_failed:
> - /*
> - * We might have failed before sending any reqs over wire.
> - * Clean up rest of the reqs in mirror pg_list.
> - */
> - if (desc->pg_error) {
> - struct nfs_pgio_mirror *mirror;
> - void (*func)(struct list_head *);
> -
> - func = desc->pg_completion_ops->error_cleanup;
> - for (midx = 0; midx < desc->pg_mirror_count; midx++) {
> - mirror = &desc->pg_mirrors[midx];
> - func(&mirror->pg_list);
> - }
> - }
> + nfs_pageio_error_cleanup(desc);
> return 0;
> }
>
> @@ -1245,7 +1246,9 @@ void nfs_pageio_complete(struct
> nfs_pageio_descriptor *desc)
> for (midx = 0; midx < desc->pg_mirror_count; midx++)
> nfs_pageio_complete_mirror(desc, midx);
>
> - if (desc->pg_ops->pg_cleanup)
> + if (desc->pg_error < 0)
> + nfs_pageio_error_cleanup(desc);
> + else if (desc->pg_ops->pg_cleanup)
> desc->pg_ops->pg_cleanup(desc);
Nice catch, but shouldn't we be calling both nfs_pageio_error_cleanup()
and pg_cleanup()? The former would appear to be cleaning up the page
stuff, while the latter is mainly cleaning up the layout.
> nfs_pageio_cleanup_mirroring(desc);
> }
Should this perhaps be a stable fix?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
