* question about fs/nfs/direct.c
@ 2012-07-08 9:15 Julia Lawall
2012-07-08 14:34 ` Myklebust, Trond
0 siblings, 1 reply; 3+ messages in thread
From: Julia Lawall @ 2012-07-08 9:15 UTC (permalink / raw)
To: Trond.Myklebust, linux-nfs
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: question about fs/nfs/direct.c 2012-07-08 9:15 question about fs/nfs/direct.c Julia Lawall @ 2012-07-08 14:34 ` Myklebust, Trond 2012-07-08 14:42 ` Julia Lawall 0 siblings, 1 reply; 3+ messages in thread From: Myklebust, Trond @ 2012-07-08 14:34 UTC (permalink / raw) To: Julia Lawall; +Cc: linux-nfs@vger.kernel.org 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== ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: question about fs/nfs/direct.c 2012-07-08 14:34 ` Myklebust, Trond @ 2012-07-08 14:42 ` Julia Lawall 0 siblings, 0 replies; 3+ messages in thread From: Julia Lawall @ 2012-07-08 14:42 UTC (permalink / raw) To: Myklebust, Trond; +Cc: linux-nfs@vger.kernel.org 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 > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-07-08 14:42 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-08 9:15 question about fs/nfs/direct.c Julia Lawall 2012-07-08 14:34 ` Myklebust, Trond 2012-07-08 14:42 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox