We don't need to call this in the direct, read, or pnfs resend paths and
the only other caller is the write path in nfs_page_async_flush() which
already checks and sets the pg_error on the context.
Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/pagelist.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/nfs/pagelist.c b/fs/nfs/pagelist.c
index 67d19cd92e44..cd3bc41ab68d 100644
--- a/fs/nfs/pagelist.c
+++ b/fs/nfs/pagelist.c
@@ -1168,11 +1168,6 @@ int nfs_pageio_add_request(struct nfs_pageio_descriptor *desc,
struct nfs_pgio_mirror *mirror;
void (*func)(struct list_head *);
- /* remember fatal errors */
- if (nfs_error_is_fatal(desc->pg_error))
- nfs_context_set_write_error(req->wb_context,
- desc->pg_error);
-
func = desc->pg_completion_ops->error_cleanup;
for (midx = 0; midx < desc->pg_mirror_count; midx++) {
mirror = &desc->pg_mirrors[midx];
--
2.14.3
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 <[email protected]>
---
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);
nfs_pageio_cleanup_mirroring(desc);
}
--
2.14.3
T24gV2VkLCAyMDE4LTEwLTE3IGF0IDEyOjA1IC0wNDAwLCBCZW5qYW1pbiBDb2RkaW5ndG9uIHdy
b3RlOg0KPiBXZSBtdXN0IGNoZWNrIHBnX2Vycm9yIGFuZCBjYWxsIGVycm9yX2NsZWFudXAgYWZ0
ZXIgYW55IGNhbGwgdG8NCj4gcGdfZG9pby4NCj4gQ3VycmVudGx5LCB3ZSBhcmUgc2tpcHBpbmcg
dGhlIHVubG9jayBvZiBhIHBhZ2UgaWYgd2UgZW5jb3VudGVyIGFuDQo+IGVycm9yIGluDQo+IG5m
c19wYWdlaW9fY29tcGxldGUoKSBiZWZvcmUgaGFuZGluZyBvZmYgdGhlIHdvcmsgdG8gdGhlIFJQ
QyBsYXllci4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IEJlbmphbWluIENvZGRpbmd0b24gPGJjb2Rk
aW5nQHJlZGhhdC5jb20+DQo+IC0tLQ0KPiAgZnMvbmZzL3BhZ2VsaXN0LmMgfCAzMyArKysrKysr
KysrKysrKysrKystLS0tLS0tLS0tLS0tLS0NCj4gIDEgZmlsZSBjaGFuZ2VkLCAxOCBpbnNlcnRp
b25zKCspLCAxNSBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvcGFnZWxp
c3QuYyBiL2ZzL25mcy9wYWdlbGlzdC5jDQo+IGluZGV4IGNkM2JjNDFhYjY4ZC4uNTRjMmJmYzQ1
YTU3IDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvcGFnZWxpc3QuYw0KPiArKysgYi9mcy9uZnMvcGFn
ZWxpc3QuYw0KPiBAQCAtMTExMCw2ICsxMTEwLDIwIEBAIHN0YXRpYyBpbnQNCj4gbmZzX3BhZ2Vp
b19hZGRfcmVxdWVzdF9taXJyb3Ioc3RydWN0IG5mc19wYWdlaW9fZGVzY3JpcHRvciAqZGVzYywN
Cj4gIAlyZXR1cm4gcmV0Ow0KPiAgfQ0KPiAgDQo+ICtzdGF0aWMgdm9pZCBuZnNfcGFnZWlvX2Vy
cm9yX2NsZWFudXAoc3RydWN0IG5mc19wYWdlaW9fZGVzY3JpcHRvcg0KPiAqZGVzYykNCj4gK3sN
Cj4gKwl1MzIgbWlkeDsNCj4gKwlzdHJ1Y3QgbmZzX3BnaW9fbWlycm9yICptaXJyb3I7DQo+ICsN
Cj4gKwlpZiAoIWRlc2MtPnBnX2Vycm9yKQ0KPiArCQlyZXR1cm47DQo+ICsNCj4gKwlmb3IgKG1p
ZHggPSAwOyBtaWR4IDwgZGVzYy0+cGdfbWlycm9yX2NvdW50OyBtaWR4KyspIHsNCj4gKwkJbWly
cm9yID0gJmRlc2MtPnBnX21pcnJvcnNbbWlkeF07DQo+ICsJCWRlc2MtPnBnX2NvbXBsZXRpb25f
b3BzLT5lcnJvcl9jbGVhbnVwKCZtaXJyb3ItDQo+ID5wZ19saXN0KTsNCj4gKwl9DQo+ICt9DQo+
ICsNCj4gIGludCBuZnNfcGFnZWlvX2FkZF9yZXF1ZXN0KHN0cnVjdCBuZnNfcGFnZWlvX2Rlc2Ny
aXB0b3IgKmRlc2MsDQo+ICAJCQkgICBzdHJ1Y3QgbmZzX3BhZ2UgKnJlcSkNCj4gIHsNCj4gQEAg
LTExNjAsMjAgKzExNzQsNyBAQCBpbnQgbmZzX3BhZ2Vpb19hZGRfcmVxdWVzdChzdHJ1Y3QNCj4g
bmZzX3BhZ2Vpb19kZXNjcmlwdG9yICpkZXNjLA0KPiAgCXJldHVybiAxOw0KPiAgDQo+ICBvdXRf
ZmFpbGVkOg0KPiAtCS8qDQo+IC0JICogV2UgbWlnaHQgaGF2ZSBmYWlsZWQgYmVmb3JlIHNlbmRp
bmcgYW55IHJlcXMgb3ZlciB3aXJlLg0KPiAtCSAqIENsZWFuIHVwIHJlc3Qgb2YgdGhlIHJlcXMg
aW4gbWlycm9yIHBnX2xpc3QuDQo+IC0JICovDQo+IC0JaWYgKGRlc2MtPnBnX2Vycm9yKSB7DQo+
IC0JCXN0cnVjdCBuZnNfcGdpb19taXJyb3IgKm1pcnJvcjsNCj4gLQkJdm9pZCAoKmZ1bmMpKHN0
cnVjdCBsaXN0X2hlYWQgKik7DQo+IC0NCj4gLQkJZnVuYyA9IGRlc2MtPnBnX2NvbXBsZXRpb25f
b3BzLT5lcnJvcl9jbGVhbnVwOw0KPiAtCQlmb3IgKG1pZHggPSAwOyBtaWR4IDwgZGVzYy0+cGdf
bWlycm9yX2NvdW50OyBtaWR4KyspIHsNCj4gLQkJCW1pcnJvciA9ICZkZXNjLT5wZ19taXJyb3Jz
W21pZHhdOw0KPiAtCQkJZnVuYygmbWlycm9yLT5wZ19saXN0KTsNCj4gLQkJfQ0KPiAtCX0NCj4g
KwluZnNfcGFnZWlvX2Vycm9yX2NsZWFudXAoZGVzYyk7DQo+ICAJcmV0dXJuIDA7DQo+ICB9DQo+
ICANCj4gQEAgLTEyNDUsNyArMTI0Niw5IEBAIHZvaWQgbmZzX3BhZ2Vpb19jb21wbGV0ZShzdHJ1
Y3QNCj4gbmZzX3BhZ2Vpb19kZXNjcmlwdG9yICpkZXNjKQ0KPiAgCWZvciAobWlkeCA9IDA7IG1p
ZHggPCBkZXNjLT5wZ19taXJyb3JfY291bnQ7IG1pZHgrKykNCj4gIAkJbmZzX3BhZ2Vpb19jb21w
bGV0ZV9taXJyb3IoZGVzYywgbWlkeCk7DQo+ICANCj4gLQlpZiAoZGVzYy0+cGdfb3BzLT5wZ19j
bGVhbnVwKQ0KPiArCWlmIChkZXNjLT5wZ19lcnJvciA8IDApDQo+ICsJCW5mc19wYWdlaW9fZXJy
b3JfY2xlYW51cChkZXNjKTsNCj4gKwllbHNlIGlmIChkZXNjLT5wZ19vcHMtPnBnX2NsZWFudXAp
DQo+ICAJCWRlc2MtPnBnX29wcy0+cGdfY2xlYW51cChkZXNjKTsNCg0KTmljZSBjYXRjaCwgYnV0
IHNob3VsZG4ndCB3ZSBiZSBjYWxsaW5nIGJvdGggbmZzX3BhZ2Vpb19lcnJvcl9jbGVhbnVwKCkN
CmFuZCBwZ19jbGVhbnVwKCk/IFRoZSBmb3JtZXIgd291bGQgYXBwZWFyIHRvIGJlIGNsZWFuaW5n
IHVwIHRoZSBwYWdlDQpzdHVmZiwgd2hpbGUgdGhlIGxhdHRlciBpcyBtYWlubHkgY2xlYW5pbmcg
dXAgdGhlIGxheW91dC4NCg0KPiAgCW5mc19wYWdlaW9fY2xlYW51cF9taXJyb3JpbmcoZGVzYyk7
DQo+ICB9DQoNClNob3VsZCB0aGlzIHBlcmhhcHMgYmUgYSBzdGFibGUgZml4Pw0KDQotLSANClRy
b25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBIYW1tZXJzcGFjZQ0K
dHJvbmQubXlrbGVidXN0QGhhbW1lcnNwYWNlLmNvbQ0KDQoNCg==
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