question about fs/nfs/direct.c

question about fs/nfs/direct.c

[Julia Lawall]

The following code, in the function nfs_direct_write_reschedule, looks 
strange to me:

         list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
                 if (!nfs_pageio_add_request(&desc, req)) {
                         nfs_list_add_request(req, &failed);
                         spin_lock(cinfo.lock);
                         dreq->flags = 0;
                         dreq->error = -EIO;
                         spin_unlock(cinfo.lock);
                 }
                 nfs_release_request(req);
         }
         nfs_pageio_complete(&desc);

         while (!list_empty(&failed))
                 nfs_unlock_and_release_request(req);

After the list_for_each_entry_safe, req is an address at some offset from 
the list head.  So it does not seem like an appropriate argument to 
nfs_unlock_and_release_request.

julia

Re: question about fs/nfs/direct.c

[Myklebust, Trond]

T24gU3VuLCAyMDEyLTA3LTA4IGF0IDExOjE1ICswMjAwLCBKdWxpYSBMYXdhbGwgd3JvdGU6DQo+
IFRoZSBmb2xsb3dpbmcgY29kZSwgaW4gdGhlIGZ1bmN0aW9uIG5mc19kaXJlY3Rfd3JpdGVfcmVz
Y2hlZHVsZSwgbG9va3MgDQo+IHN0cmFuZ2UgdG8gbWU6DQo+IA0KPiAgICAgICAgICBsaXN0X2Zv
cl9lYWNoX2VudHJ5X3NhZmUocmVxLCB0bXAsICZyZXFzLCB3Yl9saXN0KSB7DQo+ICAgICAgICAg
ICAgICAgICAgaWYgKCFuZnNfcGFnZWlvX2FkZF9yZXF1ZXN0KCZkZXNjLCByZXEpKSB7DQo+ICAg
ICAgICAgICAgICAgICAgICAgICAgICBuZnNfbGlzdF9hZGRfcmVxdWVzdChyZXEsICZmYWlsZWQp
Ow0KPiAgICAgICAgICAgICAgICAgICAgICAgICAgc3Bpbl9sb2NrKGNpbmZvLmxvY2spOw0KPiAg
ICAgICAgICAgICAgICAgICAgICAgICAgZHJlcS0+ZmxhZ3MgPSAwOw0KPiAgICAgICAgICAgICAg
ICAgICAgICAgICAgZHJlcS0+ZXJyb3IgPSAtRUlPOw0KPiAgICAgICAgICAgICAgICAgICAgICAg
ICAgc3Bpbl91bmxvY2soY2luZm8ubG9jayk7DQo+ICAgICAgICAgICAgICAgICAgfQ0KPiAgICAg
ICAgICAgICAgICAgIG5mc19yZWxlYXNlX3JlcXVlc3QocmVxKTsNCj4gICAgICAgICAgfQ0KPiAg
ICAgICAgICBuZnNfcGFnZWlvX2NvbXBsZXRlKCZkZXNjKTsNCj4gDQo+ICAgICAgICAgIHdoaWxl
ICghbGlzdF9lbXB0eSgmZmFpbGVkKSkNCj4gICAgICAgICAgICAgICAgICBuZnNfdW5sb2NrX2Fu
ZF9yZWxlYXNlX3JlcXVlc3QocmVxKTsNCj4gDQo+IEFmdGVyIHRoZSBsaXN0X2Zvcl9lYWNoX2Vu
dHJ5X3NhZmUsIHJlcSBpcyBhbiBhZGRyZXNzIGF0IHNvbWUgb2Zmc2V0IGZyb20gDQo+IHRoZSBs
aXN0IGhlYWQuICBTbyBpdCBkb2VzIG5vdCBzZWVtIGxpa2UgYW4gYXBwcm9wcmlhdGUgYXJndW1l
bnQgdG8gDQo+IG5mc191bmxvY2tfYW5kX3JlbGVhc2VfcmVxdWVzdC4NCg0KRG9oIS4uLiBUaGF0
J3MgYSBidWcgdGhhdCBjcmVwdCBpbiB2aWEgY29tbWl0DQoxNzYzZGExMjM0Y2JhNjYzYjg0OTQ3
NmQ0NTFiZGNjYWM1MTQ3ODU5IChORlM6IHJld3JpdGUgZGlyZWN0aW8gd3JpdGUgdG8NCnVzZSBh
c3luYyBjb2FsZXNjZSBjb2RlKSBhbmQgaGFzIGJlZW4gInBvbGlzaGVkIiB1bnRpbCBpdCBnbGVh
bnMgc2V2ZXJhbA0KdGltZXMgd2l0aCBhc3NvcnRlZCBjbGVhbnVwcy4uLg0KDQpIb3cgYWJvdXQg
c29tZXRoaW5nIGxpa2UgdGhlIGZvbGxvd2luZyBmaXg/DQo4PC0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KRnJvbSA0
MDM1YzI0ODdmMTc5MzI3ZmFlODdhZjM0Nzc2NTk0MDJiNzk3NTg0IE1vbiBTZXAgMTcgMDA6MDA6
MDAgMjAwMQ0KRnJvbTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNv
bT4NCkRhdGU6IFN1biwgOCBKdWwgMjAxMiAxMDoyNDoxMCAtMDQwMA0KU3ViamVjdDogW1BBVENI
XSBORlM6IEZpeCBsaXN0IG1hbmlwdWxhdGlvbiBzbmFmdXMgaW4gZnMvbmZzL2RpcmVjdC5jDQoN
CkZpeCAyIGJ1Z3MgaW4gbmZzX2RpcmVjdF93cml0ZV9yZXNjaGVkdWxlOg0KDQogLSBUaGUgcmVx
dWVzdCBuZWVkcyB0byBiZSByZW1vdmVkIGZyb20gdGhlICdyZXFzJyBsaXN0IGJlZm9yZSBpdCBj
YW4NCiAgIGJlIGFkZGVkIHRvICdmYWlsZWQnLg0KIC0gRml4IGFuIGluZmluaXRlIGxvb3AgaWYg
dGhlICdmYWlsZWQnIGxpc3QgaXMgbm9uLWVtcHR5Lg0KDQpSZXBvcnRlZC1ieTogSnVsaWEgTGF3
YWxsIDxqdWxpYS5sYXdhbGxAbGlwNi5mcj4NClNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVz
dCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQotLS0NCiBmcy9uZnMvZGlyZWN0LmMgfCAg
ICA2ICsrKysrLQ0KIDEgZmlsZSBjaGFuZ2VkLCA1IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24o
LSkNCg0KZGlmZiAtLWdpdCBhL2ZzL25mcy9kaXJlY3QuYyBiL2ZzL25mcy9kaXJlY3QuYw0KaW5k
ZXggOWE0Y2JmYy4uNDgyNTMzNyAxMDA2NDQNCi0tLSBhL2ZzL25mcy9kaXJlY3QuYw0KKysrIGIv
ZnMvbmZzL2RpcmVjdC5jDQpAQCAtNDg0LDYgKzQ4NCw3IEBAIHN0YXRpYyB2b2lkIG5mc19kaXJl
Y3Rfd3JpdGVfcmVzY2hlZHVsZShzdHJ1Y3QgbmZzX2RpcmVjdF9yZXEgKmRyZXEpDQogDQogCWxp
c3RfZm9yX2VhY2hfZW50cnlfc2FmZShyZXEsIHRtcCwgJnJlcXMsIHdiX2xpc3QpIHsNCiAJCWlm
ICghbmZzX3BhZ2Vpb19hZGRfcmVxdWVzdCgmZGVzYywgcmVxKSkgew0KKwkJCW5mc19saXN0X3Jl
bW92ZV9yZXF1ZXN0KHJlcSk7DQogCQkJbmZzX2xpc3RfYWRkX3JlcXVlc3QocmVxLCAmZmFpbGVk
KTsNCiAJCQlzcGluX2xvY2soY2luZm8ubG9jayk7DQogCQkJZHJlcS0+ZmxhZ3MgPSAwOw0KQEAg
LTQ5NCw4ICs0OTUsMTEgQEAgc3RhdGljIHZvaWQgbmZzX2RpcmVjdF93cml0ZV9yZXNjaGVkdWxl
KHN0cnVjdCBuZnNfZGlyZWN0X3JlcSAqZHJlcSkNCiAJfQ0KIAluZnNfcGFnZWlvX2NvbXBsZXRl
KCZkZXNjKTsNCiANCi0Jd2hpbGUgKCFsaXN0X2VtcHR5KCZmYWlsZWQpKQ0KKwl3aGlsZSAoIWxp
c3RfZW1wdHkoJmZhaWxlZCkpIHsNCisJCXJlcSA9IG5mc19saXN0X2VudHJ5KGZhaWxlZC5uZXh0
KTsNCisJCW5mc19saXN0X3JlbW92ZV9yZXF1ZXN0KHJlcSk7DQogCQluZnNfdW5sb2NrX2FuZF9y
ZWxlYXNlX3JlcXVlc3QocmVxKTsNCisJfQ0KIA0KIAlpZiAocHV0X2RyZXEoZHJlcSkpDQogCQlu
ZnNfZGlyZWN0X3dyaXRlX2NvbXBsZXRlKGRyZXEsIGRyZXEtPmlub2RlKTsNCi0tIA0KMS43LjEw
LjQNCg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVy
DQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoN
Cg==

Re: question about fs/nfs/direct.c

[Julia Lawall]

On Sun, 8 Jul 2012, Myklebust, Trond wrote:

> On Sun, 2012-07-08 at 11:15 +0200, Julia Lawall wrote:
>> The following code, in the function nfs_direct_write_reschedule, looks
>> strange to me:
>>
>>          list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
>>                  if (!nfs_pageio_add_request(&desc, req)) {
>>                          nfs_list_add_request(req, &failed);
>>                          spin_lock(cinfo.lock);
>>                          dreq->flags = 0;
>>                          dreq->error = -EIO;
>>                          spin_unlock(cinfo.lock);
>>                  }
>>                  nfs_release_request(req);
>>          }
>>          nfs_pageio_complete(&desc);
>>
>>          while (!list_empty(&failed))
>>                  nfs_unlock_and_release_request(req);
>>
>> After the list_for_each_entry_safe, req is an address at some offset from
>> the list head.  So it does not seem like an appropriate argument to
>> nfs_unlock_and_release_request.
>
> Doh!... That's a bug that crept in via commit
> 1763da1234cba663b849476d451bdccac5147859 (NFS: rewrite directio write to
> use async coalesce code) and has been "polished" until it gleans several
> times with assorted cleanups...
>
> How about something like the following fix?
> 8<---------------------------------------------------------------------
> From 4035c2487f179327fae87af3477659402b797584 Mon Sep 17 00:00:00 2001
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date: Sun, 8 Jul 2012 10:24:10 -0400
> Subject: [PATCH] NFS: Fix list manipulation snafus in fs/nfs/direct.c
>
> Fix 2 bugs in nfs_direct_write_reschedule:
>
> - The request needs to be removed from the 'reqs' list before it can
>   be added to 'failed'.
> - Fix an infinite loop if the 'failed' list is non-empty.
>
> Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> fs/nfs/direct.c |    6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 9a4cbfc..4825337 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -484,6 +484,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
>
> 	list_for_each_entry_safe(req, tmp, &reqs, wb_list) {
> 		if (!nfs_pageio_add_request(&desc, req)) {
> +			nfs_list_remove_request(req);
> 			nfs_list_add_request(req, &failed);
> 			spin_lock(cinfo.lock);
> 			dreq->flags = 0;
> @@ -494,8 +495,11 @@ static void nfs_direct_write_reschedule(struct nfs_direct_req *dreq)
> 	}
> 	nfs_pageio_complete(&desc);
>
> -	while (!list_empty(&failed))
> +	while (!list_empty(&failed)) {
> +		req = nfs_list_entry(failed.next);
> +		nfs_list_remove_request(req);
> 		nfs_unlock_and_release_request(req);
> +	}

It seems much more reasonable.

julia


>
> 	if (put_dreq(dreq))
> 		nfs_direct_write_complete(dreq, dreq->inode);
> -- 
> 1.7.10.4
>
>
> -- 
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
>
>