| Message ID | 63d8e14cfc732a5224bfd7ecb662f2da4d708ad0.1539792123.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] nfs: remove redundant call to nfs_context_set_write_error() | expand |
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@redhat.com> > --- > 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?
On 17 Oct 2018, at 13:34, Trond Myklebust wrote: > On Wed, 2018-10-17 at 12:05 -0400, Benjamin Coddington wrote: >> @@ -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. Ah, yes .. I got pg_cleanup mixed up with pg_completion_ops->completion. Hmm, pg_cleanup seems unnecessary at the moment. They all point to pnfs_generic_pg_cleanup. I'll send a v2 after testing with some pNFS.. > >> nfs_pageio_cleanup_mirroring(desc); >> } > > Should this perhaps be a stable fix? There's a lot of churn in there so I gave up looking for a Fixes: tag. I'll take another look and see if I can figure out how far back to go. Thanks for the look, Ben
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); nfs_pageio_cleanup_mirroring(desc); }
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@redhat.com> --- fs/nfs/pagelist.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)