[PATCH 1/2] nfs: remove redundant call to nfs_context_set_write_error()

[PATCH 1/2] nfs: remove redundant call to nfs_context_set_write_error()

[Benjamin Coddington]

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 <bcodding@redhat.com>
---
 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

[PATCH 2/2] nfs: Fix a missed page unlock after pg_doio()

[Benjamin Coddington]

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);
 	nfs_pageio_cleanup_mirroring(desc);
 }
-- 
2.14.3

Re: [PATCH 2/2] nfs: Fix a missed page unlock after pg_doio()

[Trond Myklebust]

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==

Re: [PATCH 2/2] nfs: Fix a missed page unlock after pg_doio()

[Trond Myklebust]

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?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 2/2] nfs: Fix a missed page unlock after pg_doio()

[Benjamin Coddington]

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