It was reporting success even if it did nothing.
Note this is still not right. Caller nfs_commit_unstable_pages
assumes this is all or none, and count is going to be off.
Signed-off-by: Fred Isaman <[email protected]>
---
fs/nfs/nfs4filelayout.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index e0bdbf4..08625a8 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -1027,6 +1027,7 @@ filelayout_commit_pagelist(struct inode *inode, struct list_head *mds_pages,
struct nfs_write_data *data, *tmp;
LIST_HEAD(list);
unsigned int nreq = 0;
+ int status = PNFS_ATTEMPTED;
if (!list_empty(mds_pages)) {
data = nfs_commitdata_alloc();
@@ -1042,6 +1043,7 @@ filelayout_commit_pagelist(struct inode *inode, struct list_head *mds_pages,
if (nreq == 0) {
nfs_commit_clear_lock(NFS_I(inode));
+ status = -ENOMEM;
goto out;
}
@@ -1059,7 +1061,7 @@ filelayout_commit_pagelist(struct inode *inode, struct list_head *mds_pages,
}
}
out:
- return PNFS_ATTEMPTED;
+ return status;
}
static void
--
1.7.2.1
T24gVHVlLCAyMDEyLTAzLTIwIGF0IDEyOjI1IC0wNDAwLCBGcmVkIElzYW1hbiB3cm90ZToNCj4g
SXQgd2FzIHJlcG9ydGluZyBzdWNjZXNzIGV2ZW4gaWYgaXQgZGlkIG5vdGhpbmcuDQo+IA0KPiBO
b3RlIHRoaXMgaXMgc3RpbGwgbm90IHJpZ2h0LiAgQ2FsbGVyIG5mc19jb21taXRfdW5zdGFibGVf
cGFnZXMNCj4gYXNzdW1lcyB0aGlzIGlzIGFsbCBvciBub25lLCBhbmQgY291bnQgaXMgZ29pbmcg
dG8gYmUgb2ZmLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogRnJlZCBJc2FtYW4gPGlpc2FtYW5AbmV0
YXBwLmNvbT4NCj4gLS0tDQo+ICBmcy9uZnMvbmZzNGZpbGVsYXlvdXQuYyB8ICAgIDQgKysrLQ0K
PiAgMSBmaWxlcyBjaGFuZ2VkLCAzIGluc2VydGlvbnMoKyksIDEgZGVsZXRpb25zKC0pDQo+IA0K
PiBkaWZmIC0tZ2l0IGEvZnMvbmZzL25mczRmaWxlbGF5b3V0LmMgYi9mcy9uZnMvbmZzNGZpbGVs
YXlvdXQuYw0KPiBpbmRleCBlMGJkYmY0Li4wODYyNWE4IDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMv
bmZzNGZpbGVsYXlvdXQuYw0KPiArKysgYi9mcy9uZnMvbmZzNGZpbGVsYXlvdXQuYw0KPiBAQCAt
MTAyNyw2ICsxMDI3LDcgQEAgZmlsZWxheW91dF9jb21taXRfcGFnZWxpc3Qoc3RydWN0IGlub2Rl
ICppbm9kZSwgc3RydWN0IGxpc3RfaGVhZCAqbWRzX3BhZ2VzLA0KPiAgCXN0cnVjdCBuZnNfd3Jp
dGVfZGF0YQkqZGF0YSwgKnRtcDsNCj4gIAlMSVNUX0hFQUQobGlzdCk7DQo+ICAJdW5zaWduZWQg
aW50IG5yZXEgPSAwOw0KPiArCWludCBzdGF0dXMgPSBQTkZTX0FUVEVNUFRFRDsNCj4gIA0KPiAg
CWlmICghbGlzdF9lbXB0eShtZHNfcGFnZXMpKSB7DQo+ICAJCWRhdGEgPSBuZnNfY29tbWl0ZGF0
YV9hbGxvYygpOw0KPiBAQCAtMTA0Miw2ICsxMDQzLDcgQEAgZmlsZWxheW91dF9jb21taXRfcGFn
ZWxpc3Qoc3RydWN0IGlub2RlICppbm9kZSwgc3RydWN0IGxpc3RfaGVhZCAqbWRzX3BhZ2VzLA0K
PiAgDQo+ICAJaWYgKG5yZXEgPT0gMCkgew0KPiAgCQluZnNfY29tbWl0X2NsZWFyX2xvY2soTkZT
X0koaW5vZGUpKTsNCj4gKwkJc3RhdHVzID0gLUVOT01FTTsNCg0KV2hhdCBpZiBucmVxICE9IDAs
IGJ1dCBzb21lIG9mIHRoZSBhbGxvY2F0aW9ucyBzdGlsbCBmYWlsZWQ/DQoNClRoZW4gdGhlcmUn
cyB0aGUgaXNzdWUgb2Ygd2hhdCBpZiB0aGUgb25lIG9yIG1vcmUgb2YgdGhlIGNhbGxzIHRvDQpu
ZnNfaW5pdGlhdGVfY29tbWl0KCkgZmFpbD8NCg0KRmluYWxseSwgdGhlcmUgaXMgdGhlIHF1ZXN0
aW9uIG9mIHdoYXQgaWYgdGhlIGNvbW1pdCBSUEMgY2FsbHMNCnRoZW1zZWx2ZXMgZmFpbCwgb3Ig
Y2F1c2UgYSByZXRyYW5zbWlzc2lvbj8NCg0KSU9XOiBXaHkgZG8gd2Ugc2luZ2xlIG91dCBzb21l
IG9mIHRoZXNlIGVycm9ycyBhcyB3b3J0aHkgb2YgcmVwb3J0aW5nLA0KYnV0IG5vdCBvdGhlcnM/
IFRoZSBjbGVhbnVwIHNlZW1zIHRvIGJlIHRoZSBzYW1lIGluIGFsbCBjYXNlcyBhYm92ZQ0KKG1h
cmsgdGhlIHJlcXVlc3RzIGZvciByZXRyYW5zbWlzc2lvbiBhbmQgbWFyayB0aGUgaW5vZGUgZGly
dHkpLg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVy
DQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoN
Cg==
On Tue, Mar 20, 2012 at 12:39 PM, Myklebust, Trond
<[email protected]> wrote:
> On Tue, 2012-03-20 at 12:25 -0400, Fred Isaman wrote:
>> The decrement is handled by each call to nfs_request_remove_commit_list,
>> no need to do it again in nfs_scan_commit.
>>
>> Signed-off-by: Fred Isaman <[email protected]>
>> ---
>> ?fs/nfs/write.c | ? ?5 +----
>> ?1 files changed, 1 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index 0de19f4..e39ddfd 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -595,12 +595,9 @@ nfs_scan_commit(struct inode *inode, struct list_head *dst)
>> ? ? ? spin_lock(&inode->i_lock);
>> ? ? ? if (nfsi->ncommit > 0) {
>> ? ? ? ? ? ? ? const int max = INT_MAX;
>> - ? ? ? ? ? ? int pnfs_ret;
>>
>> ? ? ? ? ? ? ? ret = nfs_scan_commit_list(&nfsi->commit_list, dst, max);
>> - ? ? ? ? ? ? pnfs_ret = pnfs_scan_commit_lists(inode, max - ret);
>> - ? ? ? ? ? ? ret += pnfs_ret;
>
> No! If we've scanned pNFS pages to commit, then we need to report this
> to nfs_commit_inode. Otherwise it will skip sending the RPC calls.
>
Oops, I got carried away. I'll resend.
Fred
>> - ? ? ? ? ? ? nfsi->ncommit -= ret;
> ? ? ? ? ? ? ? ?^^^^^^^^^^^^^^^^^^^^^^
> I agree with this... I forgot to remove it.
>
>> + ? ? ? ? ? ? pnfs_scan_commit_lists(inode, max - ret);
>> ? ? ? }
>> ? ? ? spin_unlock(&inode->i_lock);
>> ? ? ? return ret;
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>
The decrement is handled by each call to nfs_request_remove_commit_list,
no need to do it again in nfs_scan_commit.
Signed-off-by: Fred Isaman <[email protected]>
---
fs/nfs/write.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 0de19f4..e39ddfd 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -595,12 +595,9 @@ nfs_scan_commit(struct inode *inode, struct list_head *dst)
spin_lock(&inode->i_lock);
if (nfsi->ncommit > 0) {
const int max = INT_MAX;
- int pnfs_ret;
ret = nfs_scan_commit_list(&nfsi->commit_list, dst, max);
- pnfs_ret = pnfs_scan_commit_lists(inode, max - ret);
- ret += pnfs_ret;
- nfsi->ncommit -= ret;
+ pnfs_scan_commit_lists(inode, max - ret);
}
spin_unlock(&inode->i_lock);
return ret;
--
1.7.2.1
On Tue, Mar 20, 2012 at 12:56 PM, Myklebust, Trond
<[email protected]> wrote:
> On Tue, 2012-03-20 at 12:25 -0400, Fred Isaman wrote:
>> It was reporting success even if it did nothing.
>>
>> Note this is still not right. ?Caller nfs_commit_unstable_pages
>> assumes this is all or none, and count is going to be off.
>>
>> Signed-off-by: Fred Isaman <[email protected]>
>> ---
>> ?fs/nfs/nfs4filelayout.c | ? ?4 +++-
>> ?1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>> index e0bdbf4..08625a8 100644
>> --- a/fs/nfs/nfs4filelayout.c
>> +++ b/fs/nfs/nfs4filelayout.c
>> @@ -1027,6 +1027,7 @@ filelayout_commit_pagelist(struct inode *inode, struct list_head *mds_pages,
>> ? ? ? struct nfs_write_data ? *data, *tmp;
>> ? ? ? LIST_HEAD(list);
>> ? ? ? unsigned int nreq = 0;
>> + ? ? int status = PNFS_ATTEMPTED;
>>
>> ? ? ? if (!list_empty(mds_pages)) {
>> ? ? ? ? ? ? ? data = nfs_commitdata_alloc();
>> @@ -1042,6 +1043,7 @@ filelayout_commit_pagelist(struct inode *inode, struct list_head *mds_pages,
>>
>> ? ? ? if (nreq == 0) {
>> ? ? ? ? ? ? ? nfs_commit_clear_lock(NFS_I(inode));
>> + ? ? ? ? ? ? status = -ENOMEM;
>
> What if nreq != 0, but some of the allocations still failed?
>
> Then there's the issue of what if the one or more of the calls to
> nfs_initiate_commit() fail?
>
> Finally, there is the question of what if the commit RPC calls
> themselves fail, or cause a retransmission?
>
> IOW: Why do we single out some of these errors as worthy of reporting,
> but not others? The cleanup seems to be the same in all cases above
> (mark the requests for retransmission and mark the inode dirty).
>
I agree (which is why I didn't fix it "in full"), but I haven't traced
all the VFS callers and their expectations. However, at one point
nfs_commit_inode returned the count of the number of pages sent to RPC
(and nfs_commit_unstable_pages uses that fact). It would not be too
hard to get that to happen again. Or stop returning a count
entirely...which means doing something about
nfs_commit_unstable_pages. Or at least make clear in comments that
the count returned is an undercount
Fred
T24gVHVlLCAyMDEyLTAzLTIwIGF0IDEyOjI1IC0wNDAwLCBGcmVkIElzYW1hbiB3cm90ZToNCj4g
VGhlIGRlY3JlbWVudCBpcyBoYW5kbGVkIGJ5IGVhY2ggY2FsbCB0byBuZnNfcmVxdWVzdF9yZW1v
dmVfY29tbWl0X2xpc3QsDQo+IG5vIG5lZWQgdG8gZG8gaXQgYWdhaW4gaW4gbmZzX3NjYW5fY29t
bWl0Lg0KPiANCj4gU2lnbmVkLW9mZi1ieTogRnJlZCBJc2FtYW4gPGlpc2FtYW5AbmV0YXBwLmNv
bT4NCj4gLS0tDQo+ICBmcy9uZnMvd3JpdGUuYyB8ICAgIDUgKy0tLS0NCj4gIDEgZmlsZXMgY2hh
bmdlZCwgMSBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdpdCBh
L2ZzL25mcy93cml0ZS5jIGIvZnMvbmZzL3dyaXRlLmMNCj4gaW5kZXggMGRlMTlmNC4uZTM5ZGRm
ZCAxMDA2NDQNCj4gLS0tIGEvZnMvbmZzL3dyaXRlLmMNCj4gKysrIGIvZnMvbmZzL3dyaXRlLmMN
Cj4gQEAgLTU5NSwxMiArNTk1LDkgQEAgbmZzX3NjYW5fY29tbWl0KHN0cnVjdCBpbm9kZSAqaW5v
ZGUsIHN0cnVjdCBsaXN0X2hlYWQgKmRzdCkNCj4gIAlzcGluX2xvY2soJmlub2RlLT5pX2xvY2sp
Ow0KPiAgCWlmIChuZnNpLT5uY29tbWl0ID4gMCkgew0KPiAgCQljb25zdCBpbnQgbWF4ID0gSU5U
X01BWDsNCj4gLQkJaW50IHBuZnNfcmV0Ow0KPiAgDQo+ICAJCXJldCA9IG5mc19zY2FuX2NvbW1p
dF9saXN0KCZuZnNpLT5jb21taXRfbGlzdCwgZHN0LCBtYXgpOw0KPiAtCQlwbmZzX3JldCA9IHBu
ZnNfc2Nhbl9jb21taXRfbGlzdHMoaW5vZGUsIG1heCAtIHJldCk7DQo+IC0JCXJldCArPSBwbmZz
X3JldDsNCg0KTm8hIElmIHdlJ3ZlIHNjYW5uZWQgcE5GUyBwYWdlcyB0byBjb21taXQsIHRoZW4g
d2UgbmVlZCB0byByZXBvcnQgdGhpcw0KdG8gbmZzX2NvbW1pdF9pbm9kZS4gT3RoZXJ3aXNlIGl0
IHdpbGwgc2tpcCBzZW5kaW5nIHRoZSBSUEMgY2FsbHMuDQoNCj4gLQkJbmZzaS0+bmNvbW1pdCAt
PSByZXQ7DQoJCV5eXl5eXl5eXl5eXl5eXl5eXl5eXl4NCkkgYWdyZWUgd2l0aCB0aGlzLi4uIEkg
Zm9yZ290IHRvIHJlbW92ZSBpdC4NCg0KPiArCQlwbmZzX3NjYW5fY29tbWl0X2xpc3RzKGlub2Rl
LCBtYXggLSByZXQpOw0KPiAgCX0NCj4gIAlzcGluX3VubG9jaygmaW5vZGUtPmlfbG9jayk7DQo+
ICAJcmV0dXJuIHJldDsNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQg
bWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0
YXBwLmNvbQ0KDQo=