Thanks for the review Dan. The DS COMMIT can be called with a NULL state.
Fix the "variable dereferenced before check" reported by Smatch.
-->Andy
From: Andy Adamson <[email protected]>
Handle DS READ and WRITE stateid errors by recovering the stateid on the MDS.
NFS4ERR_OLD_STATEID is ignored as the client always sends a
state sequenceid of zero for DS READ and WRITE stateids.
Signed-off-by: Andy Adamson <[email protected]>
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/delegation.c | 1 +
fs/nfs/nfs4filelayout.c | 33 ++++++++++++++++++++++++++++++++-
fs/nfs/nfs4state.c | 2 ++
3 files changed, 35 insertions(+), 1 deletions(-)
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 87f7544..97d5357 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -474,6 +474,7 @@ void nfs_remove_bad_delegation(struct inode *inode)
nfs_free_delegation(delegation);
}
}
+EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
/**
* nfs_expire_all_delegation_types
diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
index 47e8f34..cdaee60 100644
--- a/fs/nfs/nfs4filelayout.c
+++ b/fs/nfs/nfs4filelayout.c
@@ -36,6 +36,7 @@
#include <linux/sunrpc/metrics.h>
#include "internal.h"
+#include "delegation.h"
#include "nfs4filelayout.h"
#define NFSDBG_FACILITY NFSDBG_PNFS_LD
@@ -86,12 +87,35 @@ static int filelayout_async_handle_error(struct rpc_task *task,
struct nfs_client *clp,
int *reset)
{
+ struct nfs_server *mds_server;
+ struct nfs_client *mds_client;
+
if (task->tk_status >= 0)
return 0;
-
*reset = 0;
+ if (state != NULL) {
+ mds_server = NFS_SERVER(state->inode);
+ mds_client = mds_server->nfs_client;
+ }
switch (task->tk_status) {
+ /* MDS state errors */
+ case -NFS4ERR_DELEG_REVOKED:
+ case -NFS4ERR_ADMIN_REVOKED:
+ case -NFS4ERR_BAD_STATEID:
+ if (state != NULL)
+ nfs_remove_bad_delegation(state->inode);
+ case -NFS4ERR_OPENMODE:
+ if (state == NULL)
+ break;
+ nfs4_schedule_stateid_recovery(mds_server, state);
+ goto wait_on_recovery;
+ case -NFS4ERR_EXPIRED:
+ if (state != NULL)
+ nfs4_schedule_stateid_recovery(mds_server, state);
+ nfs4_schedule_lease_recovery(mds_client);
+ goto wait_on_recovery;
+ /* DS session errors */
case -NFS4ERR_BADSESSION:
case -NFS4ERR_BADSLOT:
case -NFS4ERR_BAD_HIGH_SLOT:
@@ -117,8 +141,15 @@ static int filelayout_async_handle_error(struct rpc_task *task,
*reset = 1;
break;
}
+out:
task->tk_status = 0;
return -EAGAIN;
+wait_on_recovery:
+ rpc_sleep_on(&mds_client->cl_rpcwaitq, task, NULL);
+ if (test_bit(NFS4CLNT_MANAGER_RUNNING, &mds_client->cl_state) == 0)
+ rpc_wake_up_queued_task(&mds_client->cl_rpcwaitq, task);
+ goto out;
+
}
/* NFS_PROTO call done callback routines */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 1dad5c5..a58d02a 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1072,6 +1072,7 @@ void nfs4_schedule_lease_recovery(struct nfs_client *clp)
set_bit(NFS4CLNT_CHECK_LEASE, &clp->cl_state);
nfs4_schedule_state_manager(clp);
}
+EXPORT_SYMBOL_GPL(nfs4_schedule_lease_recovery);
void nfs4_schedule_path_down_recovery(struct nfs_client *clp)
{
@@ -1109,6 +1110,7 @@ void nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4
nfs4_state_mark_reclaim_nograce(clp, state);
nfs4_schedule_state_manager(clp);
}
+EXPORT_SYMBOL_GPL(nfs4_schedule_stateid_recovery);
void nfs_inode_find_state_and_recover(struct inode *inode,
const nfs4_stateid *stateid)
--
1.7.6.4
T24gV2VkLCAyMDEyLTAzLTA3IGF0IDE4OjQ0IC0wNTAwLCBhbmRyb3NAbmV0YXBwLmNvbSB3cm90
ZToNCj4gRnJvbTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gDQo+IEhhbmRs
ZSBEUyBSRUFEIGFuZCBXUklURSBzdGF0ZWlkIGVycm9ycyBieSByZWNvdmVyaW5nIHRoZSBzdGF0
ZWlkIG9uIHRoZSBNRFMuDQo+IA0KPiBORlM0RVJSX09MRF9TVEFURUlEIGlzIGlnbm9yZWQgYXMg
dGhlIGNsaWVudCBhbHdheXMgc2VuZHMgYQ0KPiBzdGF0ZSBzZXF1ZW5jZWlkIG9mIHplcm8gZm9y
IERTIFJFQUQgYW5kIFdSSVRFIHN0YXRlaWRzLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogQW5keSBB
ZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVi
dXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gLS0tDQo+ICBmcy9uZnMvZGVsZWdh
dGlvbi5jICAgICB8ICAgIDEgKw0KPiAgZnMvbmZzL25mczRmaWxlbGF5b3V0LmMgfCAgIDMzICsr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLQ0KPiAgZnMvbmZzL25mczRzdGF0ZS5jICAg
ICAgfCAgICAyICsrDQo+ICAzIGZpbGVzIGNoYW5nZWQsIDM1IGluc2VydGlvbnMoKyksIDEgZGVs
ZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZnMvbmZzL2RlbGVnYXRpb24uYyBiL2ZzL25m
cy9kZWxlZ2F0aW9uLmMNCj4gaW5kZXggODdmNzU0NC4uOTdkNTM1NyAxMDA2NDQNCj4gLS0tIGEv
ZnMvbmZzL2RlbGVnYXRpb24uYw0KPiArKysgYi9mcy9uZnMvZGVsZWdhdGlvbi5jDQo+IEBAIC00
NzQsNiArNDc0LDcgQEAgdm9pZCBuZnNfcmVtb3ZlX2JhZF9kZWxlZ2F0aW9uKHN0cnVjdCBpbm9k
ZSAqaW5vZGUpDQo+ICAJCW5mc19mcmVlX2RlbGVnYXRpb24oZGVsZWdhdGlvbik7DQo+ICAJfQ0K
PiAgfQ0KPiArRVhQT1JUX1NZTUJPTF9HUEwobmZzX3JlbW92ZV9iYWRfZGVsZWdhdGlvbik7DQo+
ICANCj4gIC8qKg0KPiAgICogbmZzX2V4cGlyZV9hbGxfZGVsZWdhdGlvbl90eXBlcw0KPiBkaWZm
IC0tZ2l0IGEvZnMvbmZzL25mczRmaWxlbGF5b3V0LmMgYi9mcy9uZnMvbmZzNGZpbGVsYXlvdXQu
Yw0KPiBpbmRleCA0N2U4ZjM0Li5jZGFlZTYwIDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvbmZzNGZp
bGVsYXlvdXQuYw0KPiArKysgYi9mcy9uZnMvbmZzNGZpbGVsYXlvdXQuYw0KPiBAQCAtMzYsNiAr
MzYsNyBAQA0KPiAgI2luY2x1ZGUgPGxpbnV4L3N1bnJwYy9tZXRyaWNzLmg+DQo+ICANCj4gICNp
bmNsdWRlICJpbnRlcm5hbC5oIg0KPiArI2luY2x1ZGUgImRlbGVnYXRpb24uaCINCj4gICNpbmNs
dWRlICJuZnM0ZmlsZWxheW91dC5oIg0KPiAgDQo+ICAjZGVmaW5lIE5GU0RCR19GQUNJTElUWSAg
ICAgICAgIE5GU0RCR19QTkZTX0xEDQo+IEBAIC04NiwxMiArODcsMzUgQEAgc3RhdGljIGludCBm
aWxlbGF5b3V0X2FzeW5jX2hhbmRsZV9lcnJvcihzdHJ1Y3QgcnBjX3Rhc2sgKnRhc2ssDQo+ICAJ
CQkJCSBzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwLA0KPiAgCQkJCQkgaW50ICpyZXNldCkNCj4gIHsN
Cj4gKwlzdHJ1Y3QgbmZzX3NlcnZlciAqbWRzX3NlcnZlcjsNCj4gKwlzdHJ1Y3QgbmZzX2NsaWVu
dCAqbWRzX2NsaWVudDsNCj4gKw0KPiAgCWlmICh0YXNrLT50a19zdGF0dXMgPj0gMCkNCj4gIAkJ
cmV0dXJuIDA7DQo+IC0NCj4gIAkqcmVzZXQgPSAwOw0KPiArCWlmIChzdGF0ZSAhPSBOVUxMKSB7
DQoNCldoeSBpcyB0aGlzIHRlc3QgbmVjZXNzYXJ5IGluIHRoZSBmaXJzdCBwbGFjZSwgQlRXPyBX
aGVuIGFyZSB5b3UgZXZlcg0KZ29pbmcgdG8gY2FsbCB0aGlzIGZ1bmN0aW9uIHdpdGggc3RhdGUg
PT0gTlVMTD8NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRh
aW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNv
bQ0KDQo=
T24gVGh1LCAyMDEyLTAzLTA4IGF0IDA5OjE1IC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
IEkgYmVsaWV2ZSB0aGF0IENPTU1JVCBjYW4gYmUgY2FsbGVkIHdpdGggc3RhdGUgPT0gTlVMTCAt
IGJ1dCBsb29raW5nDQo+IGF0IHRoZSBDT01NSVQgZXJyb3JzLCBDT01NSVQgc2hvdWxkIG5vdCBy
ZXR1cm4gc3RhdGVpZCBlcnJvcnMuDQoNCkNPTU1JVCB3aWxsIG5ldmVyIGJlIGNhbGxlZCB3aXRo
IHN0YXRlID09IE5VTEwgaGVyZS4gVGhlIG9wZW4gY29udGV4dA0KTVVTVCBhbHdheXMgY2Fycnkg
YW4gb3BlbiBzdGF0ZSBpbiB0aGUgcE5GUyBjYXNlIChwYXJ0aWN1bGFybHkgc2luY2UgeW91DQph
cmUgbm90IGFsbG93ZWQgdG8gdXNlIHRoZSBzcGVjaWFsIHN0YXRlaWRzKS4NCg0KPiBJJ2xsIHJl
bW92ZSB0aGUgc3RhdGVpZCA9PSBOVUxMIGNoZWNrcy4NCg0KVGhhbmtzLg0KDQotLSANClRyb25k
IE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQu
TXlrbGVidXN0QG5ldGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==
T24gV2VkLCAyMDEyLTAzLTA3IGF0IDE4OjQ0IC0wNTAwLCBhbmRyb3NAbmV0YXBwLmNvbSB3cm90
ZToNCj4gRnJvbTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gDQo+IEhhbmRs
ZSBEUyBSRUFEIGFuZCBXUklURSBzdGF0ZWlkIGVycm9ycyBieSByZWNvdmVyaW5nIHRoZSBzdGF0
ZWlkIG9uIHRoZSBNRFMuDQo+IA0KPiBORlM0RVJSX09MRF9TVEFURUlEIGlzIGlnbm9yZWQgYXMg
dGhlIGNsaWVudCBhbHdheXMgc2VuZHMgYQ0KPiBzdGF0ZSBzZXF1ZW5jZWlkIG9mIHplcm8gZm9y
IERTIFJFQUQgYW5kIFdSSVRFIHN0YXRlaWRzLg0KPiANCj4gU2lnbmVkLW9mZi1ieTogQW5keSBB
ZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVi
dXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCj4gLS0tDQoNCkNhbiB5b3UgcGxlYXNl
IG1ha2UgdGhpcyBpbmNyZW1lbnRhbCB3LnIudC4gdGhlIGZpcnN0IHBhdGNoLg0KLS0gDQpUcm9u
ZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25k
Lk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=
I believe that COMMIT can be called with state == NULL - but looking
at the COMMIT errors, COMMIT should not return stateid errors.
I'll remove the stateid == NULL checks.
-->Andy
On Thu, Mar 8, 2012 at 9:09 AM, Myklebust, Trond
<[email protected]> wrote:
> On Wed, 2012-03-07 at 18:44 -0500, [email protected] wrote:
>> From: Andy Adamson <[email protected]>
>>
>> Handle DS READ and WRITE stateid errors by recovering the stateid on the MDS.
>>
>> NFS4ERR_OLD_STATEID is ignored as the client always sends a
>> state sequenceid of zero for DS READ and WRITE stateids.
>>
>> Signed-off-by: Andy Adamson <[email protected]>
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>> ?fs/nfs/delegation.c ? ? | ? ?1 +
>> ?fs/nfs/nfs4filelayout.c | ? 33 ++++++++++++++++++++++++++++++++-
>> ?fs/nfs/nfs4state.c ? ? ?| ? ?2 ++
>> ?3 files changed, 35 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
>> index 87f7544..97d5357 100644
>> --- a/fs/nfs/delegation.c
>> +++ b/fs/nfs/delegation.c
>> @@ -474,6 +474,7 @@ void nfs_remove_bad_delegation(struct inode *inode)
>> ? ? ? ? ? ? ? nfs_free_delegation(delegation);
>> ? ? ? }
>> ?}
>> +EXPORT_SYMBOL_GPL(nfs_remove_bad_delegation);
>>
>> ?/**
>> ? * nfs_expire_all_delegation_types
>> diff --git a/fs/nfs/nfs4filelayout.c b/fs/nfs/nfs4filelayout.c
>> index 47e8f34..cdaee60 100644
>> --- a/fs/nfs/nfs4filelayout.c
>> +++ b/fs/nfs/nfs4filelayout.c
>> @@ -36,6 +36,7 @@
>> ?#include <linux/sunrpc/metrics.h>
>>
>> ?#include "internal.h"
>> +#include "delegation.h"
>> ?#include "nfs4filelayout.h"
>>
>> ?#define NFSDBG_FACILITY ? ? ? ? NFSDBG_PNFS_LD
>> @@ -86,12 +87,35 @@ static int filelayout_async_handle_error(struct rpc_task *task,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct nfs_client *clp,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int *reset)
>> ?{
>> + ? ? struct nfs_server *mds_server;
>> + ? ? struct nfs_client *mds_client;
>> +
>> ? ? ? if (task->tk_status >= 0)
>> ? ? ? ? ? ? ? return 0;
>> -
>> ? ? ? *reset = 0;
>> + ? ? if (state != NULL) {
>
> Why is this test necessary in the first place, BTW? When are you ever
> going to call this function with state == NULL?
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
>