2012-05-01 17:16:37

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH v2 1/3] NFS: Read cleanups

Remove unused variables, and reformat some code.

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Fred Isaman <[email protected]>
---
fs/nfs/read.c | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 35e2dce..20a0293 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -341,8 +341,6 @@ static int nfs_pagein_multi(struct nfs_pageio_descriptor *desc,
struct nfs_read_data *data;
size_t rsize = desc->pg_bsize, nbytes;
unsigned int offset;
- int requests = 0;
- int ret = 0;

nfs_list_remove_request(req);
nfs_list_add_request(req, &hdr->pages);
@@ -358,12 +356,11 @@ static int nfs_pagein_multi(struct nfs_pageio_descriptor *desc,
data->pages.pagevec[0] = page;
nfs_read_rpcsetup(data, len, offset);
list_add(&data->list, &hdr->rpc_list);
- requests++;
nbytes -= len;
offset += len;
- } while(nbytes != 0);
+ } while (nbytes != 0);
desc->pg_rpc_callops = &nfs_read_common_ops;
- return ret;
+ return 0;
out_bad:
while (!list_empty(&hdr->rpc_list)) {
data = list_first_entry(&hdr->rpc_list, struct nfs_read_data, list);
@@ -387,8 +384,7 @@ static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
desc->pg_count));
if (!data) {
desc->pg_completion_ops->error_cleanup(head);
- ret = -ENOMEM;
- goto out;
+ return -ENOMEM;
}

pages = data->pages.pagevec;
@@ -402,8 +398,7 @@ static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
nfs_read_rpcsetup(data, desc->pg_count, 0);
list_add(&data->list, &hdr->rpc_list);
desc->pg_rpc_callops = &nfs_read_common_ops;
-out:
- return ret;
+ return 0;
}

int nfs_generic_pagein(struct nfs_pageio_descriptor *desc,
--
1.7.7.6



2012-05-01 18:40:22

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH v3 1/3] NFS: Read cleanups

Remove unused variables, and reformat some code.

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Fred Isaman <[email protected]>
---
fs/nfs/read.c | 13 ++++---------
1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 35e2dce..20a0293 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -341,8 +341,6 @@ static int nfs_pagein_multi(struct nfs_pageio_descriptor *desc,
struct nfs_read_data *data;
size_t rsize = desc->pg_bsize, nbytes;
unsigned int offset;
- int requests = 0;
- int ret = 0;

nfs_list_remove_request(req);
nfs_list_add_request(req, &hdr->pages);
@@ -358,12 +356,11 @@ static int nfs_pagein_multi(struct nfs_pageio_descriptor *desc,
data->pages.pagevec[0] = page;
nfs_read_rpcsetup(data, len, offset);
list_add(&data->list, &hdr->rpc_list);
- requests++;
nbytes -= len;
offset += len;
- } while(nbytes != 0);
+ } while (nbytes != 0);
desc->pg_rpc_callops = &nfs_read_common_ops;
- return ret;
+ return 0;
out_bad:
while (!list_empty(&hdr->rpc_list)) {
data = list_first_entry(&hdr->rpc_list, struct nfs_read_data, list);
@@ -387,8 +384,7 @@ static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
desc->pg_count));
if (!data) {
desc->pg_completion_ops->error_cleanup(head);
- ret = -ENOMEM;
- goto out;
+ return -ENOMEM;
}

pages = data->pages.pagevec;
@@ -402,8 +398,7 @@ static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
nfs_read_rpcsetup(data, desc->pg_count, 0);
list_add(&data->list, &hdr->rpc_list);
desc->pg_rpc_callops = &nfs_read_common_ops;
-out:
- return ret;
+ return 0;
}

int nfs_generic_pagein(struct nfs_pageio_descriptor *desc,
--
1.7.7.6


2012-05-01 17:16:38

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH v2 3/3] NFS: Simplify the nfs_read_completion functions

...and correct a bug when NFS_IOHDR_ERROR is set: we should not be
setting PageUptodate/set_page_dirty on a page unless the number of
bytes read <= hdr->good_bytes

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Fred Isaman <[email protected]>
---
v2: Actually add the bugfix to direct.c...

fs/nfs/direct.c | 46 +++++++++++++++++++---------------------------
fs/nfs/read.c | 42 +++++++++++++++++-------------------------
2 files changed, 36 insertions(+), 52 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index 78d1ead..34027d5 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -253,36 +253,28 @@ static void nfs_direct_read_completion(struct nfs_pgio_header *hdr)
dreq->count += hdr->good_bytes;
spin_unlock(&dreq->lock);

- if (!test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
- struct page *page = req->wb_page;
-
- if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
- if (bytes > hdr->good_bytes)
- zero_user(page, 0, PAGE_SIZE);
- else if (hdr->good_bytes - bytes < PAGE_SIZE)
- zero_user_segment(page,
- hdr->good_bytes & ~PAGE_MASK,
- PAGE_SIZE);
- }
- bytes += req->wb_bytes;
- nfs_list_remove_request(req);
- if (!PageCompound(page))
- set_page_dirty(page);
- nfs_direct_readpage_release(req);
+ while (!list_empty(&hdr->pages)) {
+ struct nfs_page *req = nfs_list_entry(hdr->pages.next);
+ struct page *page = req->wb_page;
+
+ if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
+ if (bytes > hdr->good_bytes)
+ zero_user(page, 0, PAGE_SIZE);
+ else if (hdr->good_bytes - bytes < PAGE_SIZE)
+ zero_user_segment(page,
+ hdr->good_bytes & ~PAGE_MASK,
+ PAGE_SIZE);
}
- } else {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
-
- if (bytes < hdr->good_bytes)
- if (!PageCompound(req->wb_page))
+ bytes += req->wb_bytes;
+ if (!PageCompound(page)) {
+ if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
+ if (bytes <= hdr->good_bytes)
set_page_dirty(req->wb_page);
- bytes += req->wb_bytes;
- nfs_list_remove_request(req);
- nfs_direct_readpage_release(req);
+ } else
+ set_page_dirty(page);
}
+ nfs_list_remove_request(req);
+ nfs_direct_readpage_release(req);
}
out_put:
if (put_dreq(dreq))
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 1961a19..b81281b 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -179,34 +179,26 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)

if (test_bit(NFS_IOHDR_REDO, &hdr->flags))
goto out;
- if (!test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
- struct page *page = req->wb_page;
-
- if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
- if (bytes > hdr->good_bytes)
- zero_user(page, 0, PAGE_SIZE);
- else if (hdr->good_bytes - bytes < PAGE_SIZE)
- zero_user_segment(page,
- hdr->good_bytes & ~PAGE_MASK,
- PAGE_SIZE);
- }
- SetPageUptodate(page);
- nfs_list_remove_request(req);
- nfs_readpage_release(req);
- bytes += PAGE_SIZE;
+ while (!list_empty(&hdr->pages)) {
+ struct nfs_page *req = nfs_list_entry(hdr->pages.next);
+ struct page *page = req->wb_page;
+
+ if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
+ if (bytes > hdr->good_bytes)
+ zero_user(page, 0, PAGE_SIZE);
+ else if (hdr->good_bytes - bytes < PAGE_SIZE)
+ zero_user_segment(page,
+ hdr->good_bytes & ~PAGE_MASK,
+ PAGE_SIZE);
}
- } else {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
-
- bytes += req->wb_bytes;
+ bytes += req->wb_bytes;
+ if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
if (bytes <= hdr->good_bytes)
SetPageUptodate(req->wb_page);
- nfs_list_remove_request(req);
- nfs_readpage_release(req);
- }
+ } else
+ SetPageUptodate(page);
+ nfs_list_remove_request(req);
+ nfs_readpage_release(req);
}
out:
hdr->release(hdr);
--
1.7.7.6


2012-05-01 19:39:08

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] NFS: Simplify the nfs_read_completion functions

T24gVHVlLCAyMDEyLTA1LTAxIGF0IDE1OjEyIC0wNDAwLCBGcmVkIElzYW1hbiB3cm90ZToNCj4g
T24gVHVlLCBNYXkgMSwgMjAxMiBhdCAyOjQwIFBNLCBUcm9uZCBNeWtsZWJ1c3QNCj4gPFRyb25k
Lk15a2xlYnVzdEBuZXRhcHAuY29tPiB3cm90ZToNCj4gPiBTaWduZWQtb2ZmLWJ5OiBUcm9uZCBN
eWtsZWJ1c3QgPFRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tPg0KPiA+IENjOiBGcmVkIElzYW1h
biA8aWlzYW1hbkBuZXRhcHAuY29tPg0KPiA+IC0tLQ0KPiA+ICBmcy9uZnMvZGlyZWN0LmMgfCAg
IDQ2ICsrKysrKysrKysrKysrKysrKystLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4gPiAg
ZnMvbmZzL3JlYWQuYyAgIHwgICA0MiArKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0NCj4gPiAgMiBmaWxlcyBjaGFuZ2VkLCAzNiBpbnNlcnRpb25zKCspLCA1MiBkZWxl
dGlvbnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9mcy9uZnMvZGlyZWN0LmMgYi9mcy9uZnMv
ZGlyZWN0LmMNCj4gPiBpbmRleCBmMTdlNDY5Li5hMWQzNjZiIDEwMDY0NA0KPiA+IC0tLSBhL2Zz
L25mcy9kaXJlY3QuYw0KPiA+ICsrKyBiL2ZzL25mcy9kaXJlY3QuYw0KPiA+IEBAIC0yNDMsMzYg
KzI0MywyOCBAQCBzdGF0aWMgdm9pZCBuZnNfZGlyZWN0X3JlYWRfY29tcGxldGlvbihzdHJ1Y3Qg
bmZzX3BnaW9faGVhZGVyICpoZHIpDQo+ID4gICAgICAgICAgICAgICAgZHJlcS0+Y291bnQgKz0g
aGRyLT5nb29kX2J5dGVzOw0KPiA+ICAgICAgICBzcGluX3VubG9jaygmZHJlcS0+bG9jayk7DQo+
ID4NCj4gPiAtICAgICAgIGlmICghdGVzdF9iaXQoTkZTX0lPSERSX0VSUk9SLCAmaGRyLT5mbGFn
cykpIHsNCj4gPiAtICAgICAgICAgICAgICAgd2hpbGUgKCFsaXN0X2VtcHR5KCZoZHItPnBhZ2Vz
KSkgew0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIHN0cnVjdCBuZnNfcGFnZSAqcmVxID0g
bmZzX2xpc3RfZW50cnkoaGRyLT5wYWdlcy5uZXh0KTsNCj4gPiAtICAgICAgICAgICAgICAgICAg
ICAgICBzdHJ1Y3QgcGFnZSAqcGFnZSA9IHJlcS0+d2JfcGFnZTsNCj4gPiAtDQo+ID4gLSAgICAg
ICAgICAgICAgICAgICAgICAgaWYgKHRlc3RfYml0KE5GU19JT0hEUl9FT0YsICZoZHItPmZsYWdz
KSkgew0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgaWYgKGJ5dGVzID4gaGRy
LT5nb29kX2J5dGVzKQ0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICB6ZXJvX3VzZXIocGFnZSwgMCwgUEFHRV9TSVpFKTsNCj4gPiAtICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgIGVsc2UgaWYgKGhkci0+Z29vZF9ieXRlcyAtIGJ5dGVzIDwgUEFHRV9TSVpF
KQ0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB6ZXJvX3VzZXJf
c2VnbWVudChwYWdlLA0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgIGhkci0+Z29vZF9ieXRlcyAmIH5QQUdFX01BU0ssDQo+ID4gLSAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgUEFHRV9TSVpFKTsNCj4gPiAtICAg
ICAgICAgICAgICAgICAgICAgICB9DQo+ID4gLSAgICAgICAgICAgICAgICAgICAgICAgYnl0ZXMg
Kz0gcmVxLT53Yl9ieXRlczsNCj4gPiAtICAgICAgICAgICAgICAgICAgICAgICBuZnNfbGlzdF9y
ZW1vdmVfcmVxdWVzdChyZXEpOw0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIGlmICghUGFn
ZUNvbXBvdW5kKHBhZ2UpKQ0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc2V0
X3BhZ2VfZGlydHkocGFnZSk7DQo+ID4gLSAgICAgICAgICAgICAgICAgICAgICAgbmZzX2RpcmVj
dF9yZWFkcGFnZV9yZWxlYXNlKHJlcSk7DQo+ID4gKyAgICAgICB3aGlsZSAoIWxpc3RfZW1wdHko
Jmhkci0+cGFnZXMpKSB7DQo+ID4gKyAgICAgICAgICAgICAgIHN0cnVjdCBuZnNfcGFnZSAqcmVx
ID0gbmZzX2xpc3RfZW50cnkoaGRyLT5wYWdlcy5uZXh0KTsNCj4gPiArICAgICAgICAgICAgICAg
c3RydWN0IHBhZ2UgKnBhZ2UgPSByZXEtPndiX3BhZ2U7DQo+ID4gKw0KPiA+ICsgICAgICAgICAg
ICAgICBpZiAodGVzdF9iaXQoTkZTX0lPSERSX0VPRiwgJmhkci0+ZmxhZ3MpKSB7DQo+ID4gKyAg
ICAgICAgICAgICAgICAgICAgICAgaWYgKGJ5dGVzID4gaGRyLT5nb29kX2J5dGVzKQ0KPiA+ICsg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgemVyb191c2VyKHBhZ2UsIDAsIFBBR0VfU0la
RSk7DQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgZWxzZSBpZiAoaGRyLT5nb29kX2J5dGVz
IC0gYnl0ZXMgPCBQQUdFX1NJWkUpDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICB6ZXJvX3VzZXJfc2VnbWVudChwYWdlLA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICBoZHItPmdvb2RfYnl0ZXMgJiB+UEFHRV9NQVNLLA0KPiA+ICsgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBQQUdFX1NJWkUpOw0KPiA+ICAgICAgICAg
ICAgICAgIH0NCj4gPiAtICAgICAgIH0gZWxzZSB7DQo+ID4gLSAgICAgICAgICAgICAgIHdoaWxl
ICghbGlzdF9lbXB0eSgmaGRyLT5wYWdlcykpIHsNCj4gPiAtICAgICAgICAgICAgICAgICAgICAg
ICBzdHJ1Y3QgbmZzX3BhZ2UgKnJlcSA9IG5mc19saXN0X2VudHJ5KGhkci0+cGFnZXMubmV4dCk7
DQo+ID4gLQ0KPiA+IC0gICAgICAgICAgICAgICAgICAgICAgIGlmIChieXRlcyA8IGhkci0+Z29v
ZF9ieXRlcykNCj4gPiAtICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGlmICghUGFnZUNv
bXBvdW5kKHJlcS0+d2JfcGFnZSkpDQo+ID4gKyAgICAgICAgICAgICAgIGlmICghUGFnZUNvbXBv
dW5kKHBhZ2UpKSB7DQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgaWYgKHRlc3RfYml0KE5G
U19JT0hEUl9FUlJPUiwgJmhkci0+ZmxhZ3MpKSB7DQo+ID4gKyAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICBpZiAoYnl0ZXMgPCBoZHItPmdvb2RfYnl0ZXMpDQo+ID4gICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgc2V0X3BhZ2VfZGlydHkocmVxLT53Yl9wYWdlKTsN
Cj4gDQo+IFlvdSBjYW4gcy9yZXEtPndiX3BhZ2UvcGFnZS8NCg0KWWVwLiBXaWxsIGRvLi4uDQoN
Cj4gT3RoZXIgdGhhbiB0aGF0LCBsb29rcyBmaW5lLCBidXQgSSdsbCBub3RlIHRoYXQgbXkgaW50
ZW50IGluIHdyaXRpbmcNCj4gaXQgYXMgaXQgd2FzIHdhcyB0byBtb3ZlIHRoZSB0ZXN0X2JpdCBv
dXQgb2YgdGhlIGxvb3AuDQo+IEJ1dCBnaXZlbiB0aGUgRU9GIHRlc3QgdGhhdCB3YXMgc3RpbGwg
dGhlcmUsIEkgZ3Vlc3MgaXQgZG9lc24ndCBtYWtlDQo+IG11Y2ggZGlmZmVyZW5jZS4NCg0KVGhl
IG90aGVyIHRoaW5nIGlzIHRoYXQgaXQgYmVjb21lcyB1bm5lY2Vzc2FyaWx5IGRpZmZpY3VsdCB0
byByZWFkDQpiZWNhdXNlIG9mIHRoZSBsb29wcyB0aGF0IGRvIG1vc3RseSB0aGUgc2FtZSB0aGlu
ZywgZXhjZXB0IGZvciB0aGlzIG9uZQ0KbGl0dGxlIGJpdCBpbiB0aGUgbWlkZGxlLg0KDQotLSAN
ClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0K
VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==

2012-05-01 18:40:23

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH v3 2/3] NFS: Clean up nfs read and write error paths

Move the error handling for nfs_generic_pagein() into a single function.
Ditto for nfs_generic_flush().

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Fred Isaman <[email protected]>
---
fs/nfs/pnfs.c | 2 --
fs/nfs/read.c | 38 +++++++++++++++++++++-----------------
fs/nfs/write.c | 46 +++++++++++++++++++++++-----------------------
3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 39cbac5..6fdeca2 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1321,7 +1321,6 @@ pnfs_generic_pg_writepages(struct nfs_pageio_descriptor *desc)
if (ret != 0) {
put_lseg(desc->pg_lseg);
desc->pg_lseg = NULL;
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
} else
pnfs_do_multiple_writes(desc, &hdr->rpc_list, desc->pg_ioflags);
if (atomic_dec_and_test(&hdr->refcnt))
@@ -1476,7 +1475,6 @@ pnfs_generic_pg_readpages(struct nfs_pageio_descriptor *desc)
if (ret != 0) {
put_lseg(desc->pg_lseg);
desc->pg_lseg = NULL;
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
} else
pnfs_do_multiple_reads(desc, &hdr->rpc_list);
if (atomic_dec_and_test(&hdr->refcnt))
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 20a0293..1961a19 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -320,6 +320,19 @@ static const struct nfs_pgio_completion_ops nfs_async_read_completion_ops = {
.completion = nfs_read_completion,
};

+static void nfs_pagein_error(struct nfs_pageio_descriptor *desc,
+ struct nfs_pgio_header *hdr)
+{
+ set_bit(NFS_IOHDR_REDO, &hdr->flags);
+ while (!list_empty(&hdr->rpc_list)) {
+ struct nfs_read_data *data = list_first_entry(&hdr->rpc_list,
+ struct nfs_read_data, list);
+ list_del(&data->list);
+ nfs_readdata_release(data);
+ }
+ desc->pg_completion_ops->error_cleanup(&desc->pg_list);
+}
+
/*
* Generate multiple requests to fill a single page.
*
@@ -342,33 +355,27 @@ static int nfs_pagein_multi(struct nfs_pageio_descriptor *desc,
size_t rsize = desc->pg_bsize, nbytes;
unsigned int offset;

- nfs_list_remove_request(req);
- nfs_list_add_request(req, &hdr->pages);
-
offset = 0;
nbytes = desc->pg_count;
do {
size_t len = min(nbytes,rsize);

data = nfs_readdata_alloc(hdr, 1);
- if (!data)
- goto out_bad;
+ if (!data) {
+ nfs_pagein_error(desc, hdr);
+ return -ENOMEM;
+ }
data->pages.pagevec[0] = page;
nfs_read_rpcsetup(data, len, offset);
list_add(&data->list, &hdr->rpc_list);
nbytes -= len;
offset += len;
} while (nbytes != 0);
+
+ nfs_list_remove_request(req);
+ nfs_list_add_request(req, &hdr->pages);
desc->pg_rpc_callops = &nfs_read_common_ops;
return 0;
-out_bad:
- while (!list_empty(&hdr->rpc_list)) {
- data = list_first_entry(&hdr->rpc_list, struct nfs_read_data, list);
- list_del(&data->list);
- nfs_readdata_release(data);
- }
- desc->pg_completion_ops->error_cleanup(&hdr->pages);
- return -ENOMEM;
}

static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
@@ -378,12 +385,11 @@ static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
struct page **pages;
struct nfs_read_data *data;
struct list_head *head = &desc->pg_list;
- int ret = 0;

data = nfs_readdata_alloc(hdr, nfs_page_array_len(desc->pg_base,
desc->pg_count));
if (!data) {
- desc->pg_completion_ops->error_cleanup(head);
+ nfs_pagein_error(desc, hdr);
return -ENOMEM;
}

@@ -427,8 +433,6 @@ static int nfs_generic_pg_readpages(struct nfs_pageio_descriptor *desc)
if (ret == 0)
ret = nfs_do_multiple_reads(&hdr->rpc_list,
desc->pg_rpc_callops);
- else
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
if (atomic_dec_and_test(&hdr->refcnt))
hdr->completion_ops->completion(hdr);
return ret;
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 2f80aa5..d1e4f81 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1058,6 +1058,19 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops = {
.completion = nfs_write_completion,
};

+static void nfs_flush_error(struct nfs_pageio_descriptor *desc,
+ struct nfs_pgio_header *hdr)
+{
+ set_bit(NFS_IOHDR_REDO, &hdr->flags);
+ while (!list_empty(&hdr->rpc_list)) {
+ struct nfs_write_data *data = list_first_entry(&hdr->rpc_list,
+ struct nfs_write_data, list);
+ list_del(&data->list);
+ nfs_writedata_release(data);
+ }
+ desc->pg_completion_ops->error_cleanup(&desc->pg_list);
+}
+
/*
* Generate multiple small requests to write out a single
* contiguous dirty area on one page.
@@ -1071,12 +1084,9 @@ static int nfs_flush_multi(struct nfs_pageio_descriptor *desc,
size_t wsize = desc->pg_bsize, nbytes;
unsigned int offset;
int requests = 0;
- int ret = 0;
struct nfs_commit_info cinfo;

nfs_init_cinfo(&cinfo, desc->pg_inode, desc->pg_dreq);
- nfs_list_remove_request(req);
- nfs_list_add_request(req, &hdr->pages);

if ((desc->pg_ioflags & FLUSH_COND_STABLE) &&
(desc->pg_moreio || nfs_reqs_to_commit(&cinfo) ||
@@ -1090,8 +1100,10 @@ static int nfs_flush_multi(struct nfs_pageio_descriptor *desc,
size_t len = min(nbytes, wsize);

data = nfs_writedata_alloc(hdr, 1);
- if (!data)
- goto out_bad;
+ if (!data) {
+ nfs_flush_error(desc, hdr);
+ return -ENOMEM;
+ }
data->pages.pagevec[0] = page;
nfs_write_rpcsetup(data, len, offset, desc->pg_ioflags, &cinfo);
list_add(&data->list, &hdr->rpc_list);
@@ -1099,17 +1111,10 @@ static int nfs_flush_multi(struct nfs_pageio_descriptor *desc,
nbytes -= len;
offset += len;
} while (nbytes != 0);
+ nfs_list_remove_request(req);
+ nfs_list_add_request(req, &hdr->pages);
desc->pg_rpc_callops = &nfs_write_common_ops;
- return ret;
-
-out_bad:
- while (!list_empty(&hdr->rpc_list)) {
- data = list_first_entry(&hdr->rpc_list, struct nfs_write_data, list);
- list_del(&data->list);
- nfs_writedata_release(data);
- }
- desc->pg_completion_ops->error_cleanup(&hdr->pages);
- return -ENOMEM;
+ return 0;
}

/*
@@ -1127,15 +1132,13 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc,
struct page **pages;
struct nfs_write_data *data;
struct list_head *head = &desc->pg_list;
- int ret = 0;
struct nfs_commit_info cinfo;

data = nfs_writedata_alloc(hdr, nfs_page_array_len(desc->pg_base,
desc->pg_count));
if (!data) {
- desc->pg_completion_ops->error_cleanup(head);
- ret = -ENOMEM;
- goto out;
+ nfs_flush_error(desc, hdr);
+ return -ENOMEM;
}

nfs_init_cinfo(&cinfo, desc->pg_inode, desc->pg_dreq);
@@ -1155,8 +1158,7 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc,
nfs_write_rpcsetup(data, desc->pg_count, 0, desc->pg_ioflags, &cinfo);
list_add(&data->list, &hdr->rpc_list);
desc->pg_rpc_callops = &nfs_write_common_ops;
-out:
- return ret;
+ return 0;
}

int nfs_generic_flush(struct nfs_pageio_descriptor *desc,
@@ -1186,8 +1188,6 @@ static int nfs_generic_pg_writepages(struct nfs_pageio_descriptor *desc)
ret = nfs_do_multiple_writes(&hdr->rpc_list,
desc->pg_rpc_callops,
desc->pg_ioflags);
- else
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
if (atomic_dec_and_test(&hdr->refcnt))
hdr->completion_ops->completion(hdr);
return ret;
--
1.7.7.6


2012-05-01 17:16:38

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH v2 2/3] NFS: Clean up nfs read and write error paths

Move the error handling for nfs_generic_pagein() into a single function.
Ditto for nfs_generic_flush().

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Fred Isaman <[email protected]>
---
fs/nfs/pnfs.c | 2 --
fs/nfs/read.c | 38 +++++++++++++++++++++-----------------
fs/nfs/write.c | 46 +++++++++++++++++++++++-----------------------
3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 39cbac5..6fdeca2 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1321,7 +1321,6 @@ pnfs_generic_pg_writepages(struct nfs_pageio_descriptor *desc)
if (ret != 0) {
put_lseg(desc->pg_lseg);
desc->pg_lseg = NULL;
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
} else
pnfs_do_multiple_writes(desc, &hdr->rpc_list, desc->pg_ioflags);
if (atomic_dec_and_test(&hdr->refcnt))
@@ -1476,7 +1475,6 @@ pnfs_generic_pg_readpages(struct nfs_pageio_descriptor *desc)
if (ret != 0) {
put_lseg(desc->pg_lseg);
desc->pg_lseg = NULL;
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
} else
pnfs_do_multiple_reads(desc, &hdr->rpc_list);
if (atomic_dec_and_test(&hdr->refcnt))
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 20a0293..1961a19 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -320,6 +320,19 @@ static const struct nfs_pgio_completion_ops nfs_async_read_completion_ops = {
.completion = nfs_read_completion,
};

+static void nfs_pagein_error(struct nfs_pageio_descriptor *desc,
+ struct nfs_pgio_header *hdr)
+{
+ set_bit(NFS_IOHDR_REDO, &hdr->flags);
+ while (!list_empty(&hdr->rpc_list)) {
+ struct nfs_read_data *data = list_first_entry(&hdr->rpc_list,
+ struct nfs_read_data, list);
+ list_del(&data->list);
+ nfs_readdata_release(data);
+ }
+ desc->pg_completion_ops->error_cleanup(&desc->pg_list);
+}
+
/*
* Generate multiple requests to fill a single page.
*
@@ -342,33 +355,27 @@ static int nfs_pagein_multi(struct nfs_pageio_descriptor *desc,
size_t rsize = desc->pg_bsize, nbytes;
unsigned int offset;

- nfs_list_remove_request(req);
- nfs_list_add_request(req, &hdr->pages);
-
offset = 0;
nbytes = desc->pg_count;
do {
size_t len = min(nbytes,rsize);

data = nfs_readdata_alloc(hdr, 1);
- if (!data)
- goto out_bad;
+ if (!data) {
+ nfs_pagein_error(desc, hdr);
+ return -ENOMEM;
+ }
data->pages.pagevec[0] = page;
nfs_read_rpcsetup(data, len, offset);
list_add(&data->list, &hdr->rpc_list);
nbytes -= len;
offset += len;
} while (nbytes != 0);
+
+ nfs_list_remove_request(req);
+ nfs_list_add_request(req, &hdr->pages);
desc->pg_rpc_callops = &nfs_read_common_ops;
return 0;
-out_bad:
- while (!list_empty(&hdr->rpc_list)) {
- data = list_first_entry(&hdr->rpc_list, struct nfs_read_data, list);
- list_del(&data->list);
- nfs_readdata_release(data);
- }
- desc->pg_completion_ops->error_cleanup(&hdr->pages);
- return -ENOMEM;
}

static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
@@ -378,12 +385,11 @@ static int nfs_pagein_one(struct nfs_pageio_descriptor *desc,
struct page **pages;
struct nfs_read_data *data;
struct list_head *head = &desc->pg_list;
- int ret = 0;

data = nfs_readdata_alloc(hdr, nfs_page_array_len(desc->pg_base,
desc->pg_count));
if (!data) {
- desc->pg_completion_ops->error_cleanup(head);
+ nfs_pagein_error(desc, hdr);
return -ENOMEM;
}

@@ -427,8 +433,6 @@ static int nfs_generic_pg_readpages(struct nfs_pageio_descriptor *desc)
if (ret == 0)
ret = nfs_do_multiple_reads(&hdr->rpc_list,
desc->pg_rpc_callops);
- else
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
if (atomic_dec_and_test(&hdr->refcnt))
hdr->completion_ops->completion(hdr);
return ret;
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 41db7b7..6f263da 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1062,6 +1062,19 @@ static const struct nfs_pgio_completion_ops nfs_async_write_completion_ops = {
.completion = nfs_write_completion,
};

+static void nfs_flush_error(struct nfs_pageio_descriptor *desc,
+ struct nfs_pgio_header *hdr)
+{
+ set_bit(NFS_IOHDR_REDO, &hdr->flags);
+ while (!list_empty(&hdr->rpc_list)) {
+ struct nfs_write_data *data = list_first_entry(&hdr->rpc_list,
+ struct nfs_write_data, list);
+ list_del(&data->list);
+ nfs_writedata_release(data);
+ }
+ desc->pg_completion_ops->error_cleanup(&desc->pg_list);
+}
+
/*
* Generate multiple small requests to write out a single
* contiguous dirty area on one page.
@@ -1075,12 +1088,9 @@ static int nfs_flush_multi(struct nfs_pageio_descriptor *desc,
size_t wsize = desc->pg_bsize, nbytes;
unsigned int offset;
int requests = 0;
- int ret = 0;
struct nfs_commit_info cinfo;

nfs_init_cinfo(&cinfo, desc->pg_inode, desc->pg_dreq);
- nfs_list_remove_request(req);
- nfs_list_add_request(req, &hdr->pages);

if ((desc->pg_ioflags & FLUSH_COND_STABLE) &&
(desc->pg_moreio || nfs_reqs_to_commit(&cinfo) ||
@@ -1094,8 +1104,10 @@ static int nfs_flush_multi(struct nfs_pageio_descriptor *desc,
size_t len = min(nbytes, wsize);

data = nfs_writedata_alloc(hdr, 1);
- if (!data)
- goto out_bad;
+ if (!data) {
+ nfs_flush_error(desc, hdr);
+ return -ENOMEM;
+ }
data->pages.pagevec[0] = page;
nfs_write_rpcsetup(data, len, offset, desc->pg_ioflags, &cinfo);
list_add(&data->list, &hdr->rpc_list);
@@ -1103,17 +1115,10 @@ static int nfs_flush_multi(struct nfs_pageio_descriptor *desc,
nbytes -= len;
offset += len;
} while (nbytes != 0);
+ nfs_list_remove_request(req);
+ nfs_list_add_request(req, &hdr->pages);
desc->pg_rpc_callops = &nfs_write_common_ops;
- return ret;
-
-out_bad:
- while (!list_empty(&hdr->rpc_list)) {
- data = list_first_entry(&hdr->rpc_list, struct nfs_write_data, list);
- list_del(&data->list);
- nfs_writedata_release(data);
- }
- desc->pg_completion_ops->error_cleanup(&hdr->pages);
- return -ENOMEM;
+ return 0;
}

/*
@@ -1131,15 +1136,13 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc,
struct page **pages;
struct nfs_write_data *data;
struct list_head *head = &desc->pg_list;
- int ret = 0;
struct nfs_commit_info cinfo;

data = nfs_writedata_alloc(hdr, nfs_page_array_len(desc->pg_base,
desc->pg_count));
if (!data) {
- desc->pg_completion_ops->error_cleanup(head);
- ret = -ENOMEM;
- goto out;
+ nfs_flush_error(desc, hdr);
+ return -ENOMEM;
}

nfs_init_cinfo(&cinfo, desc->pg_inode, desc->pg_dreq);
@@ -1159,8 +1162,7 @@ static int nfs_flush_one(struct nfs_pageio_descriptor *desc,
nfs_write_rpcsetup(data, desc->pg_count, 0, desc->pg_ioflags, &cinfo);
list_add(&data->list, &hdr->rpc_list);
desc->pg_rpc_callops = &nfs_write_common_ops;
-out:
- return ret;
+ return 0;
}

int nfs_generic_flush(struct nfs_pageio_descriptor *desc,
@@ -1190,8 +1192,6 @@ static int nfs_generic_pg_writepages(struct nfs_pageio_descriptor *desc)
ret = nfs_do_multiple_writes(&hdr->rpc_list,
desc->pg_rpc_callops,
desc->pg_ioflags);
- else
- set_bit(NFS_IOHDR_REDO, &hdr->flags);
if (atomic_dec_and_test(&hdr->refcnt))
hdr->completion_ops->completion(hdr);
return ret;
--
1.7.7.6


2012-05-01 18:37:17

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] NFS: Simplify the nfs_read_completion functions

T24gVHVlLCAyMDEyLTA1LTAxIGF0IDEzOjE2IC0wNDAwLCBUcm9uZCBNeWtsZWJ1c3Qgd3JvdGU6
DQo+IC4uLmFuZCBjb3JyZWN0IGEgYnVnIHdoZW4gTkZTX0lPSERSX0VSUk9SIGlzIHNldDogd2Ug
c2hvdWxkIG5vdCBiZQ0KPiBzZXR0aW5nIFBhZ2VVcHRvZGF0ZS9zZXRfcGFnZV9kaXJ0eSBvbiBh
IHBhZ2UgdW5sZXNzIHRoZSBudW1iZXIgb2YNCj4gYnl0ZXMgcmVhZCA8PSBoZHItPmdvb2RfYnl0
ZXMNCj4gDQo+IFNpZ25lZC1vZmYtYnk6IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0
QG5ldGFwcC5jb20+DQo+IENjOiBGcmVkIElzYW1hbiA8aWlzYW1hbkBuZXRhcHAuY29tPg0KPiAt
LS0NCj4gdjI6IEFjdHVhbGx5IGFkZCB0aGUgYnVnZml4IHRvIGRpcmVjdC5jLi4uDQo+IA0KPiAg
ZnMvbmZzL2RpcmVjdC5jIHwgICA0NiArKysrKysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tDQo+ICBmcy9uZnMvcmVhZC5jICAgfCAgIDQyICsrKysrKysrKysrKysrKysr
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPiAgMiBmaWxlcyBjaGFuZ2VkLCAzNiBpbnNlcnRp
b25zKCspLCA1MiBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9uZnMvZGlyZWN0
LmMgYi9mcy9uZnMvZGlyZWN0LmMNCj4gaW5kZXggNzhkMWVhZC4uMzQwMjdkNSAxMDA2NDQNCj4g
LS0tIGEvZnMvbmZzL2RpcmVjdC5jDQo+ICsrKyBiL2ZzL25mcy9kaXJlY3QuYw0KPiBAQCAtMjUz
LDM2ICsyNTMsMjggQEAgc3RhdGljIHZvaWQgbmZzX2RpcmVjdF9yZWFkX2NvbXBsZXRpb24oc3Ry
dWN0IG5mc19wZ2lvX2hlYWRlciAqaGRyKQ0KPiAgCQlkcmVxLT5jb3VudCArPSBoZHItPmdvb2Rf
Ynl0ZXM7DQo+ICAJc3Bpbl91bmxvY2soJmRyZXEtPmxvY2spOw0KPiAgDQo+IC0JaWYgKCF0ZXN0
X2JpdChORlNfSU9IRFJfRVJST1IsICZoZHItPmZsYWdzKSkgew0KPiAtCQl3aGlsZSAoIWxpc3Rf
ZW1wdHkoJmhkci0+cGFnZXMpKSB7DQo+IC0JCQlzdHJ1Y3QgbmZzX3BhZ2UgKnJlcSA9IG5mc19s
aXN0X2VudHJ5KGhkci0+cGFnZXMubmV4dCk7DQo+IC0JCQlzdHJ1Y3QgcGFnZSAqcGFnZSA9IHJl
cS0+d2JfcGFnZTsNCj4gLQ0KPiAtCQkJaWYgKHRlc3RfYml0KE5GU19JT0hEUl9FT0YsICZoZHIt
PmZsYWdzKSkgew0KPiAtCQkJCWlmIChieXRlcyA+IGhkci0+Z29vZF9ieXRlcykNCj4gLQkJCQkJ
emVyb191c2VyKHBhZ2UsIDAsIFBBR0VfU0laRSk7DQo+IC0JCQkJZWxzZSBpZiAoaGRyLT5nb29k
X2J5dGVzIC0gYnl0ZXMgPCBQQUdFX1NJWkUpDQo+IC0JCQkJCXplcm9fdXNlcl9zZWdtZW50KHBh
Z2UsDQo+IC0JCQkJCQloZHItPmdvb2RfYnl0ZXMgJiB+UEFHRV9NQVNLLA0KPiAtCQkJCQkJUEFH
RV9TSVpFKTsNCj4gLQkJCX0NCj4gLQkJCWJ5dGVzICs9IHJlcS0+d2JfYnl0ZXM7DQo+IC0JCQlu
ZnNfbGlzdF9yZW1vdmVfcmVxdWVzdChyZXEpOw0KPiAtCQkJaWYgKCFQYWdlQ29tcG91bmQocGFn
ZSkpDQo+IC0JCQkJc2V0X3BhZ2VfZGlydHkocGFnZSk7DQo+IC0JCQluZnNfZGlyZWN0X3JlYWRw
YWdlX3JlbGVhc2UocmVxKTsNCj4gKwl3aGlsZSAoIWxpc3RfZW1wdHkoJmhkci0+cGFnZXMpKSB7
DQo+ICsJCXN0cnVjdCBuZnNfcGFnZSAqcmVxID0gbmZzX2xpc3RfZW50cnkoaGRyLT5wYWdlcy5u
ZXh0KTsNCj4gKwkJc3RydWN0IHBhZ2UgKnBhZ2UgPSByZXEtPndiX3BhZ2U7DQo+ICsNCj4gKwkJ
aWYgKHRlc3RfYml0KE5GU19JT0hEUl9FT0YsICZoZHItPmZsYWdzKSkgew0KPiArCQkJaWYgKGJ5
dGVzID4gaGRyLT5nb29kX2J5dGVzKQ0KPiArCQkJCXplcm9fdXNlcihwYWdlLCAwLCBQQUdFX1NJ
WkUpOw0KPiArCQkJZWxzZSBpZiAoaGRyLT5nb29kX2J5dGVzIC0gYnl0ZXMgPCBQQUdFX1NJWkUp
DQo+ICsJCQkJemVyb191c2VyX3NlZ21lbnQocGFnZSwNCj4gKwkJCQkJaGRyLT5nb29kX2J5dGVz
ICYgflBBR0VfTUFTSywNCj4gKwkJCQkJUEFHRV9TSVpFKTsNCj4gIAkJfQ0KPiAtCX0gZWxzZSB7
DQo+IC0JCXdoaWxlICghbGlzdF9lbXB0eSgmaGRyLT5wYWdlcykpIHsNCj4gLQkJCXN0cnVjdCBu
ZnNfcGFnZSAqcmVxID0gbmZzX2xpc3RfZW50cnkoaGRyLT5wYWdlcy5uZXh0KTsNCj4gLQ0KPiAt
CQkJaWYgKGJ5dGVzIDwgaGRyLT5nb29kX2J5dGVzKQ0KPiAtCQkJCWlmICghUGFnZUNvbXBvdW5k
KHJlcS0+d2JfcGFnZSkpDQo+ICsJCWJ5dGVzICs9IHJlcS0+d2JfYnl0ZXM7DQo+ICsJCWlmICgh
UGFnZUNvbXBvdW5kKHBhZ2UpKSB7DQo+ICsJCQlpZiAodGVzdF9iaXQoTkZTX0lPSERSX0VSUk9S
LCAmaGRyLT5mbGFncykpIHsNCj4gKwkJCQlpZiAoYnl0ZXMgPD0gaGRyLT5nb29kX2J5dGVzKQ0K
PiAgCQkJCQlzZXRfcGFnZV9kaXJ0eShyZXEtPndiX3BhZ2UpOw0KDQpPSy4uLiBMb29raW5nIGF0
IHRoaXMgbW9yZSBjbG9zZWx5LCBJIHRoaW5rIHRoYXQgSSdtIHdyb25nIGluIGNhbGxpbmcNCnRo
ZSBvbGQgYmVoYXZpb3VyIGEgYnVnLiBTaW5jZSB3ZSBhcmUgc2F5aW5nIHRoYXQgdGhlIGJ1ZmZl
ciBpcyBnb29kIHVwDQp0byBhbmQgaW5jbHVkaW5nIGhkci0+Z29vZF9ieXRlcyBvZiByZWFkIGJ5
dGVzLCB3ZSBzaG91bGQgbWFyayB0aGUgcGFnZQ0KYXMgYmVpbmcgZGlydHkgc28gbG9uZyBhcyBp
dCBjb250YWlucyBvbmUgb3IgbW9yZSBnb29kIGJ5dGVzLg0KDQpJJ2xsIHJlc2VuZCB0aGUgY2xl
YW51cCB3aXRob3V0IHRoZSBlcnJvbmVvdXMgZml4Li4uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0
DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RA
bmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20NCg0K

2012-05-01 18:40:24

by Myklebust, Trond

[permalink] [raw]
Subject: [PATCH v3 3/3] NFS: Simplify the nfs_read_completion functions

Signed-off-by: Trond Myklebust <[email protected]>
Cc: Fred Isaman <[email protected]>
---
fs/nfs/direct.c | 46 +++++++++++++++++++---------------------------
fs/nfs/read.c | 42 +++++++++++++++++-------------------------
2 files changed, 36 insertions(+), 52 deletions(-)

diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
index f17e469..a1d366b 100644
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -243,36 +243,28 @@ static void nfs_direct_read_completion(struct nfs_pgio_header *hdr)
dreq->count += hdr->good_bytes;
spin_unlock(&dreq->lock);

- if (!test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
- struct page *page = req->wb_page;
-
- if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
- if (bytes > hdr->good_bytes)
- zero_user(page, 0, PAGE_SIZE);
- else if (hdr->good_bytes - bytes < PAGE_SIZE)
- zero_user_segment(page,
- hdr->good_bytes & ~PAGE_MASK,
- PAGE_SIZE);
- }
- bytes += req->wb_bytes;
- nfs_list_remove_request(req);
- if (!PageCompound(page))
- set_page_dirty(page);
- nfs_direct_readpage_release(req);
+ while (!list_empty(&hdr->pages)) {
+ struct nfs_page *req = nfs_list_entry(hdr->pages.next);
+ struct page *page = req->wb_page;
+
+ if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
+ if (bytes > hdr->good_bytes)
+ zero_user(page, 0, PAGE_SIZE);
+ else if (hdr->good_bytes - bytes < PAGE_SIZE)
+ zero_user_segment(page,
+ hdr->good_bytes & ~PAGE_MASK,
+ PAGE_SIZE);
}
- } else {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
-
- if (bytes < hdr->good_bytes)
- if (!PageCompound(req->wb_page))
+ if (!PageCompound(page)) {
+ if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
+ if (bytes < hdr->good_bytes)
set_page_dirty(req->wb_page);
- bytes += req->wb_bytes;
- nfs_list_remove_request(req);
- nfs_direct_readpage_release(req);
+ } else
+ set_page_dirty(page);
}
+ bytes += req->wb_bytes;
+ nfs_list_remove_request(req);
+ nfs_direct_readpage_release(req);
}
out_put:
if (put_dreq(dreq))
diff --git a/fs/nfs/read.c b/fs/nfs/read.c
index 1961a19..b81281b 100644
--- a/fs/nfs/read.c
+++ b/fs/nfs/read.c
@@ -179,34 +179,26 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)

if (test_bit(NFS_IOHDR_REDO, &hdr->flags))
goto out;
- if (!test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
- struct page *page = req->wb_page;
-
- if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
- if (bytes > hdr->good_bytes)
- zero_user(page, 0, PAGE_SIZE);
- else if (hdr->good_bytes - bytes < PAGE_SIZE)
- zero_user_segment(page,
- hdr->good_bytes & ~PAGE_MASK,
- PAGE_SIZE);
- }
- SetPageUptodate(page);
- nfs_list_remove_request(req);
- nfs_readpage_release(req);
- bytes += PAGE_SIZE;
+ while (!list_empty(&hdr->pages)) {
+ struct nfs_page *req = nfs_list_entry(hdr->pages.next);
+ struct page *page = req->wb_page;
+
+ if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
+ if (bytes > hdr->good_bytes)
+ zero_user(page, 0, PAGE_SIZE);
+ else if (hdr->good_bytes - bytes < PAGE_SIZE)
+ zero_user_segment(page,
+ hdr->good_bytes & ~PAGE_MASK,
+ PAGE_SIZE);
}
- } else {
- while (!list_empty(&hdr->pages)) {
- struct nfs_page *req = nfs_list_entry(hdr->pages.next);
-
- bytes += req->wb_bytes;
+ bytes += req->wb_bytes;
+ if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
if (bytes <= hdr->good_bytes)
SetPageUptodate(req->wb_page);
- nfs_list_remove_request(req);
- nfs_readpage_release(req);
- }
+ } else
+ SetPageUptodate(page);
+ nfs_list_remove_request(req);
+ nfs_readpage_release(req);
}
out:
hdr->release(hdr);
--
1.7.7.6


2012-05-01 19:12:12

by Fred Isaman

[permalink] [raw]
Subject: Re: [PATCH v3 3/3] NFS: Simplify the nfs_read_completion functions

On Tue, May 1, 2012 at 2:40 PM, Trond Myklebust
<[email protected]> wrote:
> Signed-off-by: Trond Myklebust <[email protected]>
> Cc: Fred Isaman <[email protected]>
> ---
> ?fs/nfs/direct.c | ? 46 +++++++++++++++++++---------------------------
> ?fs/nfs/read.c ? | ? 42 +++++++++++++++++-------------------------
> ?2 files changed, 36 insertions(+), 52 deletions(-)
>
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index f17e469..a1d366b 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -243,36 +243,28 @@ static void nfs_direct_read_completion(struct nfs_pgio_header *hdr)
> ? ? ? ? ? ? ? ?dreq->count += hdr->good_bytes;
> ? ? ? ?spin_unlock(&dreq->lock);
>
> - ? ? ? if (!test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
> - ? ? ? ? ? ? ? while (!list_empty(&hdr->pages)) {
> - ? ? ? ? ? ? ? ? ? ? ? struct nfs_page *req = nfs_list_entry(hdr->pages.next);
> - ? ? ? ? ? ? ? ? ? ? ? struct page *page = req->wb_page;
> -
> - ? ? ? ? ? ? ? ? ? ? ? if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (bytes > hdr->good_bytes)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user(page, 0, PAGE_SIZE);
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? else if (hdr->good_bytes - bytes < PAGE_SIZE)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user_segment(page,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->good_bytes & ~PAGE_MASK,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PAGE_SIZE);
> - ? ? ? ? ? ? ? ? ? ? ? }
> - ? ? ? ? ? ? ? ? ? ? ? bytes += req->wb_bytes;
> - ? ? ? ? ? ? ? ? ? ? ? nfs_list_remove_request(req);
> - ? ? ? ? ? ? ? ? ? ? ? if (!PageCompound(page))
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_page_dirty(page);
> - ? ? ? ? ? ? ? ? ? ? ? nfs_direct_readpage_release(req);
> + ? ? ? while (!list_empty(&hdr->pages)) {
> + ? ? ? ? ? ? ? struct nfs_page *req = nfs_list_entry(hdr->pages.next);
> + ? ? ? ? ? ? ? struct page *page = req->wb_page;
> +
> + ? ? ? ? ? ? ? if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
> + ? ? ? ? ? ? ? ? ? ? ? if (bytes > hdr->good_bytes)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user(page, 0, PAGE_SIZE);
> + ? ? ? ? ? ? ? ? ? ? ? else if (hdr->good_bytes - bytes < PAGE_SIZE)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user_segment(page,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->good_bytes & ~PAGE_MASK,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PAGE_SIZE);
> ? ? ? ? ? ? ? ?}
> - ? ? ? } else {
> - ? ? ? ? ? ? ? while (!list_empty(&hdr->pages)) {
> - ? ? ? ? ? ? ? ? ? ? ? struct nfs_page *req = nfs_list_entry(hdr->pages.next);
> -
> - ? ? ? ? ? ? ? ? ? ? ? if (bytes < hdr->good_bytes)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (!PageCompound(req->wb_page))
> + ? ? ? ? ? ? ? if (!PageCompound(page)) {
> + ? ? ? ? ? ? ? ? ? ? ? if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (bytes < hdr->good_bytes)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?set_page_dirty(req->wb_page);

You can s/req->wb_page/page/

Other than that, looks fine, but I'll note that my intent in writing
it as it was was to move the test_bit out of the loop.
But given the EOF test that was still there, I guess it doesn't make
much difference.

Fred

> - ? ? ? ? ? ? ? ? ? ? ? bytes += req->wb_bytes;
> - ? ? ? ? ? ? ? ? ? ? ? nfs_list_remove_request(req);
> - ? ? ? ? ? ? ? ? ? ? ? nfs_direct_readpage_release(req);
> + ? ? ? ? ? ? ? ? ? ? ? } else
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? set_page_dirty(page);
> ? ? ? ? ? ? ? ?}
> + ? ? ? ? ? ? ? bytes += req->wb_bytes;
> + ? ? ? ? ? ? ? nfs_list_remove_request(req);
> + ? ? ? ? ? ? ? nfs_direct_readpage_release(req);
> ? ? ? ?}
> ?out_put:
> ? ? ? ?if (put_dreq(dreq))
> diff --git a/fs/nfs/read.c b/fs/nfs/read.c
> index 1961a19..b81281b 100644
> --- a/fs/nfs/read.c
> +++ b/fs/nfs/read.c
> @@ -179,34 +179,26 @@ static void nfs_read_completion(struct nfs_pgio_header *hdr)
>
> ? ? ? ?if (test_bit(NFS_IOHDR_REDO, &hdr->flags))
> ? ? ? ? ? ? ? ?goto out;
> - ? ? ? if (!test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
> - ? ? ? ? ? ? ? while (!list_empty(&hdr->pages)) {
> - ? ? ? ? ? ? ? ? ? ? ? struct nfs_page *req = nfs_list_entry(hdr->pages.next);
> - ? ? ? ? ? ? ? ? ? ? ? struct page *page = req->wb_page;
> -
> - ? ? ? ? ? ? ? ? ? ? ? if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (bytes > hdr->good_bytes)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user(page, 0, PAGE_SIZE);
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? else if (hdr->good_bytes - bytes < PAGE_SIZE)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user_segment(page,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->good_bytes & ~PAGE_MASK,
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PAGE_SIZE);
> - ? ? ? ? ? ? ? ? ? ? ? }
> - ? ? ? ? ? ? ? ? ? ? ? SetPageUptodate(page);
> - ? ? ? ? ? ? ? ? ? ? ? nfs_list_remove_request(req);
> - ? ? ? ? ? ? ? ? ? ? ? nfs_readpage_release(req);
> - ? ? ? ? ? ? ? ? ? ? ? bytes += PAGE_SIZE;
> + ? ? ? while (!list_empty(&hdr->pages)) {
> + ? ? ? ? ? ? ? struct nfs_page *req = nfs_list_entry(hdr->pages.next);
> + ? ? ? ? ? ? ? struct page *page = req->wb_page;
> +
> + ? ? ? ? ? ? ? if (test_bit(NFS_IOHDR_EOF, &hdr->flags)) {
> + ? ? ? ? ? ? ? ? ? ? ? if (bytes > hdr->good_bytes)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user(page, 0, PAGE_SIZE);
> + ? ? ? ? ? ? ? ? ? ? ? else if (hdr->good_bytes - bytes < PAGE_SIZE)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? zero_user_segment(page,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? hdr->good_bytes & ~PAGE_MASK,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? PAGE_SIZE);
> ? ? ? ? ? ? ? ?}
> - ? ? ? } else {
> - ? ? ? ? ? ? ? while (!list_empty(&hdr->pages)) {
> - ? ? ? ? ? ? ? ? ? ? ? struct nfs_page *req = nfs_list_entry(hdr->pages.next);
> -
> - ? ? ? ? ? ? ? ? ? ? ? bytes += req->wb_bytes;
> + ? ? ? ? ? ? ? bytes += req->wb_bytes;
> + ? ? ? ? ? ? ? if (test_bit(NFS_IOHDR_ERROR, &hdr->flags)) {
> ? ? ? ? ? ? ? ? ? ? ? ?if (bytes <= hdr->good_bytes)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?SetPageUptodate(req->wb_page);
> - ? ? ? ? ? ? ? ? ? ? ? nfs_list_remove_request(req);
> - ? ? ? ? ? ? ? ? ? ? ? nfs_readpage_release(req);
> - ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? } else
> + ? ? ? ? ? ? ? ? ? ? ? SetPageUptodate(page);
> + ? ? ? ? ? ? ? nfs_list_remove_request(req);
> + ? ? ? ? ? ? ? nfs_readpage_release(req);
> ? ? ? ?}
> ?out:
> ? ? ? ?hdr->release(hdr);
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html