[PATCH 0/3] NFS recover from delegation stateid error

[PATCH 0/3] NFS recover from delegation stateid error

[andros]

From: Andy Adamson <andros@netapp.com>

These patches fix recovery from NFS4ERR_BAD_STATEID, NFS4ERR_ADMIN_REVOKED, and
NFS4ERR_DELEG_REVOKED errors by removing the delegation record, testing the
delegation stateid, and recovering via an OPEN with CLAIM_NULL.

Tested with a pynfs test that removes the pynfs server delegation stateid 
and return NFS4ERR_BAD_STATEID upon a READ.

Andy Adamson (1):
  NFSv4.1 do not clear NFS_DELEAGED_STATE until stateid is tested

Trond Myklebust (2):
  NFSv4.1: Fix the checking of the stateid when returning a delegation
  NFS: Properly handle the case where the delegation is revoked

 fs/nfs/callback_proc.c |    6 +++---
 fs/nfs/delegation.c    |   13 ++++++++++++-
 fs/nfs/delegation.h    |    1 +
 fs/nfs/nfs4_fs.h       |    2 ++
 fs/nfs/nfs4proc.c      |   16 ++++++++++++++--
 fs/nfs/nfs4state.c     |   31 ++++++++++++++++++++++++++++++-
 6 files changed, 62 insertions(+), 7 deletions(-)

-- 
1.7.6.4

[PATCH 1/3] NFSv4.1: Fix the checking of the stateid when returning a delegation

[andros]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

nfs41_validate_delegation_stateid is broken if we supply a stateid with
a non-zero sequence id. Instead of trying to match the sequence id,
the function assumes that we always want to error. While this is
true for a delegation callback, it is not true in general.

Also fix a typo in nfs4_callback_recall.

Reported-by: Andy Adamson <andros@netapp.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/callback_proc.c |    6 +++---
 fs/nfs/delegation.c    |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/callback_proc.c b/fs/nfs/callback_proc.c
index f71978d..63190c1 100644
--- a/fs/nfs/callback_proc.c
+++ b/fs/nfs/callback_proc.c
@@ -86,8 +86,7 @@ __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy,
 		res = 0;
 		break;
 	case -ENOENT:
-		if (res != 0)
-			res = htonl(NFS4ERR_BAD_STATEID);
+		res = htonl(NFS4ERR_BAD_STATEID);
 		break;
 	default:
 		res = htonl(NFS4ERR_RESOURCE);
@@ -328,7 +327,8 @@ int nfs41_validate_delegation_stateid(struct nfs_delegation *delegation, const n
 	if (delegation == NULL)
 		return 0;
 
-	if (stateid->stateid.seqid != 0)
+	if (stateid->stateid.seqid != 0 &&
+	    stateid->stateid.seqid != delegation->stateid.stateid.seqid)
 		return 0;
 	if (memcmp(&delegation->stateid.stateid.other,
 		   &stateid->stateid.other,
diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 7f26540..1596098 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -531,7 +531,7 @@ void nfs_expire_unreferenced_delegations(struct nfs_client *clp)
 /**
  * nfs_async_inode_return_delegation - asynchronously return a delegation
  * @inode: inode to process
- * @stateid: state ID information from CB_RECALL arguments
+ * @stateid: state ID information
  *
  * Returns zero on success, or a negative errno value.
  */
-- 
1.7.6.4

Re: [PATCH 1/3] NFSv4.1: Fix the checking of the stateid when returning a delegation

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTAzLTA2IGF0IDA5OjQ2IC0wNTAwLCBhbmRyb3NAbmV0YXBwLmNvbSB3cm90
ZToNCj4gRnJvbTogVHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4N
Cj4gDQo+IG5mczQxX3ZhbGlkYXRlX2RlbGVnYXRpb25fc3RhdGVpZCBpcyBicm9rZW4gaWYgd2Ug
c3VwcGx5IGEgc3RhdGVpZCB3aXRoDQo+IGEgbm9uLXplcm8gc2VxdWVuY2UgaWQuIEluc3RlYWQg
b2YgdHJ5aW5nIHRvIG1hdGNoIHRoZSBzZXF1ZW5jZSBpZCwNCj4gdGhlIGZ1bmN0aW9uIGFzc3Vt
ZXMgdGhhdCB3ZSBhbHdheXMgd2FudCB0byBlcnJvci4gV2hpbGUgdGhpcyBpcw0KPiB0cnVlIGZv
ciBhIGRlbGVnYXRpb24gY2FsbGJhY2ssIGl0IGlzIG5vdCB0cnVlIGluIGdlbmVyYWwuDQo+IA0K
PiBBbHNvIGZpeCBhIHR5cG8gaW4gbmZzNF9jYWxsYmFja19yZWNhbGwuDQo+IA0KPiBSZXBvcnRl
ZC1ieTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gU2lnbmVkLW9mZi1ieTog
VHJvbmQgTXlrbGVidXN0IDxUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbT4NCg0KRGlkIHlvdSBy
ZWFsbHkgaW50ZW5kIHRvIENjOiBzdGFibGUgb24gdGhlc2UgYmVmb3JlIHdlJ3ZlIGNvbW1pdHRl
ZCB0aGVtDQp1cHN0cmVhbT8NCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGll
bnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cu
bmV0YXBwLmNvbQ0KDQo=

Re: [PATCH 1/3] NFSv4.1: Fix the checking of the stateid when returning a delegation

[Adamson, Andy]

On Mar 6, 2012, at 9:57 AM, Myklebust, Trond wrote:

> On Tue, 2012-03-06 at 09:46 -0500, andros@netapp.com wrote:
>> From: Trond Myklebust <Trond.Myklebust@netapp.com>
>> 
>> nfs41_validate_delegation_stateid is broken if we supply a stateid with
>> a non-zero sequence id. Instead of trying to match the sequence id,
>> the function assumes that we always want to error. While this is
>> true for a delegation callback, it is not true in general.
>> 
>> Also fix a typo in nfs4_callback_recall.
>> 
>> Reported-by: Andy Adamson <andros@netapp.com>
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> 
> Did you really intend to Cc: stable on these before we've committed them
> upstream?

I guess I misunderstood when to add the Cc: stable.

-->Andy

> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> 

Re: [PATCH 1/3] NFSv4.1: Fix the checking of the stateid when returning a delegation

[J. Bruce Fields]

On Tue, Mar 06, 2012 at 03:13:37PM +0000, Adamson, Andy wrote:
> 
> On Mar 6, 2012, at 9:57 AM, Myklebust, Trond wrote:
> 
> > On Tue, 2012-03-06 at 09:46 -0500, andros@netapp.com wrote:
> >> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> >> 
> >> nfs41_validate_delegation_stateid is broken if we supply a stateid with
> >> a non-zero sequence id. Instead of trying to match the sequence id,
> >> the function assumes that we always want to error. While this is
> >> true for a delegation callback, it is not true in general.
> >> 
> >> Also fix a typo in nfs4_callback_recall.
> >> 
> >> Reported-by: Andy Adamson <andros@netapp.com>
> >> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> > 
> > Did you really intend to Cc: stable on these before we've committed them
> > upstream?
> 
> I guess I misunderstood when to add the Cc: stable.

Me too--I thought the stable folks would wait till they see a commit?

Though adding a Cc: line with the Signed-off-by's and leaving them out
of the email header would seem simpler.

--b.

[PATCH 2/3] NFS: Properly handle the case where the delegation is revoked

[andros]

From: Trond Myklebust <Trond.Myklebust@netapp.com>

If we know that the delegation stateid is bad or revoked, we need to
remove that delegation as soon as possible, and then mark all the
stateids that relied on that delegation for recovery. We cannot use
the delegation as part of the recovery process.

Also note that NFSv4.1 uses a different error code (NFS4ERR_DELEG_REVOKED)
to indicate that the delegation was revoked.

Finally, ensure that setlk() and setattr() can both recover safely from
a revoked delegation.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/delegation.c |   11 +++++++++++
 fs/nfs/delegation.h |    1 +
 fs/nfs/nfs4_fs.h    |    2 ++
 fs/nfs/nfs4proc.c   |   16 ++++++++++++++--
 fs/nfs/nfs4state.c  |   26 ++++++++++++++++++++++++++
 5 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 1596098..c14512c 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -466,6 +466,17 @@ static void nfs_delegation_run_state_manager(struct nfs_client *clp)
 		nfs4_schedule_state_manager(clp);
 }
 
+void nfs_remove_bad_delegation(struct inode *inode)
+{
+	struct nfs_delegation *delegation;
+
+	delegation = nfs_detach_delegation(NFS_I(inode), NFS_SERVER(inode));
+	if (delegation) {
+		nfs_inode_find_state_and_recover(inode, &delegation->stateid);
+		nfs_free_delegation(delegation);
+	}
+}
+
 /**
  * nfs_expire_all_delegation_types
  * @clp: client to process
diff --git a/fs/nfs/delegation.h b/fs/nfs/delegation.h
index d9322e4..691a796 100644
--- a/fs/nfs/delegation.h
+++ b/fs/nfs/delegation.h
@@ -45,6 +45,7 @@ void nfs_expire_unreferenced_delegations(struct nfs_client *clp);
 void nfs_handle_cb_pathdown(struct nfs_client *clp);
 int nfs_client_return_marked_delegations(struct nfs_client *clp);
 int nfs_delegations_present(struct nfs_client *clp);
+void nfs_remove_bad_delegation(struct inode *inode);
 
 void nfs_delegation_mark_reclaim(struct nfs_client *clp);
 void nfs_delegation_reap_unclaimed(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
index b133b50..6b53cca 100644
--- a/fs/nfs/nfs4_fs.h
+++ b/fs/nfs/nfs4_fs.h
@@ -318,6 +318,8 @@ extern void nfs4_put_open_state(struct nfs4_state *);
 extern void nfs4_close_state(struct nfs4_state *, fmode_t);
 extern void nfs4_close_sync(struct nfs4_state *, fmode_t);
 extern void nfs4_state_set_mode_locked(struct nfs4_state *, fmode_t);
+extern void nfs_inode_find_state_and_recover(struct inode *inode,
+		const nfs4_stateid *stateid);
 extern void nfs4_schedule_lease_recovery(struct nfs_client *);
 extern void nfs4_schedule_state_manager(struct nfs_client *);
 extern void nfs4_schedule_path_down_recovery(struct nfs_client *clp);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0d5134a..2722388 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -267,8 +267,11 @@ static int nfs4_handle_exception(struct nfs_server *server, int errorcode, struc
 	switch(errorcode) {
 		case 0:
 			return 0;
+		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;
@@ -1329,6 +1332,7 @@ int nfs4_open_delegation_recall(struct nfs_open_context *ctx, struct nfs4_state
 				 * The show must go on: exit, but mark the
 				 * stateid as needing recovery.
 				 */
+			case -NFS4ERR_DELEG_REVOKED:
 			case -NFS4ERR_ADMIN_REVOKED:
 			case -NFS4ERR_BAD_STATEID:
 				nfs4_schedule_stateid_recovery(server, state);
@@ -1929,7 +1933,9 @@ static int nfs4_do_setattr(struct inode *inode, struct rpc_cred *cred,
 			   struct nfs4_state *state)
 {
 	struct nfs_server *server = NFS_SERVER(inode);
-	struct nfs4_exception exception = { };
+	struct nfs4_exception exception = {
+		.state = state,
+	};
 	int err;
 	do {
 		err = nfs4_handle_exception(server,
@@ -3753,8 +3759,11 @@ nfs4_async_handle_error(struct rpc_task *task, const struct nfs_server *server,
 	if (task->tk_status >= 0)
 		return 0;
 	switch(task->tk_status) {
+		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;
@@ -4595,7 +4604,9 @@ out:
 
 static int nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
 {
-	struct nfs4_exception exception = { };
+	struct nfs4_exception exception = {
+		.state = state,
+	};
 	int err;
 
 	do {
@@ -4688,6 +4699,7 @@ int nfs4_lock_delegation_recall(struct nfs4_state *state, struct file_lock *fl)
 				 * The show must go on: exit, but mark the
 				 * stateid as needing recovery.
 				 */
+			case -NFS4ERR_DELEG_REVOKED:
 			case -NFS4ERR_ADMIN_REVOKED:
 			case -NFS4ERR_BAD_STATEID:
 			case -NFS4ERR_OPENMODE:
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 653bfd7..7bd9822 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1103,6 +1103,32 @@ void nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4
 	nfs4_schedule_state_manager(clp);
 }
 
+void nfs_inode_find_state_and_recover(struct inode *inode,
+		const nfs4_stateid *stateid)
+{
+	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
+	struct nfs_inode *nfsi = NFS_I(inode);
+	struct nfs_open_context *ctx;
+	struct nfs4_state *state;
+	bool found = false;
+
+	spin_lock(&inode->i_lock);
+	list_for_each_entry(ctx, &nfsi->open_files, list) {
+		state = ctx->state;
+		if (state == NULL)
+			continue;
+		if (!test_bit(NFS_DELEGATED_STATE, &state->flags))
+			continue;
+		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
+			continue;
+		nfs4_state_mark_reclaim_nograce(clp, state);
+		found = true;
+	}
+	spin_unlock(&inode->i_lock);
+	if (found)
+		nfs4_schedule_state_manager(clp);
+}
+
 static int nfs4_reclaim_locks(struct nfs4_state *state, const struct nfs4_state_recovery_ops *ops)
 {
 	struct inode *inode = state->inode;
-- 
1.7.6.4

[PATCH 3/3] NFSv4.1 do not clear NFS_DELEAGED_STATE until stateid is tested

[andros]

From: Andy Adamson <andros@netapp.com>

nfs41_open_expired() will test the delegation stateid based on
NFS_DELEGATED_STATE being set. If the stateid is bad, nfs4_open_recover
will clear the NFS_DELEGATED_STATE bit recovering the delegation.

Signed-off-by: Andy Adamson <andros@netapp.com>
Cc: stable@vger.kernel.org
---
 fs/nfs/nfs4state.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 7bd9822..44fcd60 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -696,7 +696,10 @@ static void __nfs4_close(struct nfs4_state *state,
 			call_close |= test_bit(NFS_O_RDWR_STATE, &state->flags);
 		}
 		if (newstate == 0)
+{
+printk("%s CLEAR NFS_DELEGATED_STATE state %p\n", __func__, state);
 			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+}
 	}
 	nfs4_state_set_mode_locked(state, newstate);
 	spin_unlock(&owner->so_lock);
@@ -1097,7 +1100,7 @@ void nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4
 {
 	struct nfs_client *clp = server->nfs_client;
 
-	if (test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
+	if (test_bit(NFS_DELEGATED_STATE, &state->flags))
 		nfs_async_inode_return_delegation(state->inode, &state->stateid);
 	nfs4_state_mark_reclaim_nograce(clp, state);
 	nfs4_schedule_state_manager(clp);
-- 
1.7.6.4

Re: [PATCH 3/3] NFSv4.1 do not clear NFS_DELEAGED_STATE until stateid is tested

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTAzLTA2IGF0IDA5OjQ2IC0wNTAwLCBhbmRyb3NAbmV0YXBwLmNvbSB3cm90
ZToNCj4gRnJvbTogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNvbT4NCj4gDQo+IG5mczQx
X29wZW5fZXhwaXJlZCgpIHdpbGwgdGVzdCB0aGUgZGVsZWdhdGlvbiBzdGF0ZWlkIGJhc2VkIG9u
DQo+IE5GU19ERUxFR0FURURfU1RBVEUgYmVpbmcgc2V0LiBJZiB0aGUgc3RhdGVpZCBpcyBiYWQs
IG5mczRfb3Blbl9yZWNvdmVyDQo+IHdpbGwgY2xlYXIgdGhlIE5GU19ERUxFR0FURURfU1RBVEUg
Yml0IHJlY292ZXJpbmcgdGhlIGRlbGVnYXRpb24uDQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBBbmR5
IEFkYW1zb24gPGFuZHJvc0BuZXRhcHAuY29tPg0KPiBDYzogc3RhYmxlQHZnZXIua2VybmVsLm9y
Zw0KPiAtLS0NCj4gIGZzL25mcy9uZnM0c3RhdGUuYyB8ICAgIDUgKysrKy0NCj4gIDEgZmlsZXMg
Y2hhbmdlZCwgNCBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9ucygtKQ0KPiANCj4gZGlmZiAtLWdp
dCBhL2ZzL25mcy9uZnM0c3RhdGUuYyBiL2ZzL25mcy9uZnM0c3RhdGUuYw0KPiBpbmRleCA3YmQ5
ODIyLi40NGZjZDYwIDEwMDY0NA0KPiAtLS0gYS9mcy9uZnMvbmZzNHN0YXRlLmMNCj4gKysrIGIv
ZnMvbmZzL25mczRzdGF0ZS5jDQo+IEBAIC02OTYsNyArNjk2LDEwIEBAIHN0YXRpYyB2b2lkIF9f
bmZzNF9jbG9zZShzdHJ1Y3QgbmZzNF9zdGF0ZSAqc3RhdGUsDQo+ICAJCQljYWxsX2Nsb3NlIHw9
IHRlc3RfYml0KE5GU19PX1JEV1JfU1RBVEUsICZzdGF0ZS0+ZmxhZ3MpOw0KPiAgCQl9DQo+ICAJ
CWlmIChuZXdzdGF0ZSA9PSAwKQ0KPiArew0KPiArcHJpbnRrKCIlcyBDTEVBUiBORlNfREVMRUdB
VEVEX1NUQVRFIHN0YXRlICVwXG4iLCBfX2Z1bmNfXywgc3RhdGUpOw0KPiAgCQkJY2xlYXJfYml0
KE5GU19ERUxFR0FURURfU1RBVEUsICZzdGF0ZS0+ZmxhZ3MpOw0KPiArfVwNCg0KRXd3dy4uLi4u
DQoNCj4gIAl9DQo+ICAJbmZzNF9zdGF0ZV9zZXRfbW9kZV9sb2NrZWQoc3RhdGUsIG5ld3N0YXRl
KTsNCj4gIAlzcGluX3VubG9jaygmb3duZXItPnNvX2xvY2spOw0KPiBAQCAtMTA5Nyw3ICsxMTAw
LDcgQEAgdm9pZCBuZnM0X3NjaGVkdWxlX3N0YXRlaWRfcmVjb3ZlcnkoY29uc3Qgc3RydWN0IG5m
c19zZXJ2ZXIgKnNlcnZlciwgc3RydWN0IG5mczQNCj4gIHsNCj4gIAlzdHJ1Y3QgbmZzX2NsaWVu
dCAqY2xwID0gc2VydmVyLT5uZnNfY2xpZW50Ow0KPiAgDQo+IC0JaWYgKHRlc3RfYW5kX2NsZWFy
X2JpdChORlNfREVMRUdBVEVEX1NUQVRFLCAmc3RhdGUtPmZsYWdzKSkNCj4gKwlpZiAodGVzdF9i
aXQoTkZTX0RFTEVHQVRFRF9TVEFURSwgJnN0YXRlLT5mbGFncykpDQo+ICAJCW5mc19hc3luY19p
bm9kZV9yZXR1cm5fZGVsZWdhdGlvbihzdGF0ZS0+aW5vZGUsICZzdGF0ZS0+c3RhdGVpZCk7DQo+
ICAJbmZzNF9zdGF0ZV9tYXJrX3JlY2xhaW1fbm9ncmFjZShjbHAsIHN0YXRlKTsNCj4gIAluZnM0
X3NjaGVkdWxlX3N0YXRlX21hbmFnZXIoY2xwKTsNCg0KQWN0dWFsbHksIHRoZSBsYXRlc3QgaW5j
YXJuYXRpb24gb2YgdGhlIHBhdGNoIDIvMyByZW1vdmVzIHRoZQ0KYXN5bmNfaW5vZGVfcmV0dXJu
X2RlbGVnYXRpb24gZnJvbSBuZnM0X3NjaGVkdWxlX3N0YXRlaWRfcmVjb3ZlcnkuIFdlDQpkb24n
dCBuZWVkIHRvIHJldHVybiB0aGUgZGVsZWdhdGlvbiBpZiBhbGwgdGhlIHN0YXRlaWRzIGhhdmUg
YmVlbiBtYXJrZWQNCmZvciByZXR1cm4sIGFuZCB3ZSd2ZSB0aHJvd24gb3V0IHRoZSBkZWxlZ2F0
aW9uIGl0c2VsZi4NCg0KTGV0J3Mgbm90IGh1cnJ5IHRoaXMgaW50byB0aGUgc3RhYmxlIHRyZWUg
eWV0Li4uDQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWlu
ZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5jb20N
Cg0K

Re: [PATCH 3/3] NFSv4.1 do not clear NFS_DELEAGED_STATE until stateid is tested

[Andy Adamson]

On Tue, Mar 6, 2012 at 9:53 AM, Myklebust, Trond
<Trond.Myklebust@netapp.com> wrote:
> On Tue, 2012-03-06 at 09:46 -0500, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> nfs41_open_expired() will test the delegation stateid based on
>> NFS_DELEGATED_STATE being set. If the stateid is bad, nfs4_open_recover
>> will clear the NFS_DELEGATED_STATE bit recovering the delegation.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  fs/nfs/nfs4state.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 7bd9822..44fcd60 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -696,7 +696,10 @@ static void __nfs4_close(struct nfs4_state *state,
>>                       call_close |= test_bit(NFS_O_RDWR_STATE, &state->flags);
>>               }
>>               if (newstate == 0)
>> +{
>> +printk("%s CLEAR NFS_DELEGATED_STATE state %p\n", __func__, state);
>>                       clear_bit(NFS_DELEGATED_STATE, &state->flags);
>> +}\
>
> Ewww.....


yep, still had stupid development messages -- resent immediately!

-->Andy
>
>>       }
>>       nfs4_state_set_mode_locked(state, newstate);
>>       spin_unlock(&owner->so_lock);
>> @@ -1097,7 +1100,7 @@ void nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4
>>  {
>>       struct nfs_client *clp = server->nfs_client;
>>
>> -     if (test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
>> +     if (test_bit(NFS_DELEGATED_STATE, &state->flags))
>>               nfs_async_inode_return_delegation(state->inode, &state->stateid);
>>       nfs4_state_mark_reclaim_nograce(clp, state);
>>       nfs4_schedule_state_manager(clp);
>
> Actually, the latest incarnation of the patch 2/3 removes the
> async_inode_return_delegation from nfs4_schedule_stateid_recovery. We
> don't need to return the delegation if all the stateids have been marked
> for return, and we've thrown out the delegation itself.
>
> Let's not hurry this into the stable tree yet...
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
>

Re: [PATCH 3/3] NFSv4.1 do not clear NFS_DELEAGED_STATE until stateid is tested

[Andy Adamson]

On Tue, Mar 6, 2012 at 9:53 AM, Myklebust, Trond
<Trond.Myklebust@netapp.com> wrote:
> On Tue, 2012-03-06 at 09:46 -0500, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> nfs41_open_expired() will test the delegation stateid based on
>> NFS_DELEGATED_STATE being set. If the stateid is bad, nfs4_open_recover
>> will clear the NFS_DELEGATED_STATE bit recovering the delegation.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  fs/nfs/nfs4state.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 7bd9822..44fcd60 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -696,7 +696,10 @@ static void __nfs4_close(struct nfs4_state *state,
>>                       call_close |= test_bit(NFS_O_RDWR_STATE, &state->flags);
>>               }
>>               if (newstate == 0)
>> +{
>> +printk("%s CLEAR NFS_DELEGATED_STATE state %p\n", __func__, state);
>>                       clear_bit(NFS_DELEGATED_STATE, &state->flags);
>> +}\
>
> Ewww.....
>
>>       }
>>       nfs4_state_set_mode_locked(state, newstate);
>>       spin_unlock(&owner->so_lock);
>> @@ -1097,7 +1100,7 @@ void nfs4_schedule_stateid_recovery(const struct nfs_server *server, struct nfs4
>>  {
>>       struct nfs_client *clp = server->nfs_client;
>>
>> -     if (test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
>> +     if (test_bit(NFS_DELEGATED_STATE, &state->flags))
>>               nfs_async_inode_return_delegation(state->inode, &state->stateid);
>>       nfs4_state_mark_reclaim_nograce(clp, state);
>>       nfs4_schedule_state_manager(clp);
>
> Actually, the latest incarnation of the patch 2/3 removes the
> async_inode_return_delegation from nfs4_schedule_stateid_recovery.

OK - but I think we still need to postpone clearing the
NFS_DELEGATED_STATE bit until after calling nfs41_open_expired.
Otherwise nfs41_check_expired_stateid will not test the delegation
stateid, and nfs4_open_recover will not be called, as no stateid is
deemed bad, and the OPEN with CLAIM_NULL to (potentially) recover the
delegation will not be sent. In this case, the old bad delegation
stateid will be resent.

> We
> don't need to return the delegation if all the stateids have been marked
> for return, and we've thrown out the delegation itself.
>
> Let's not hurry this into the stable tree yet...

OK.

-->Andy
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
>

Re: [PATCH 3/3] NFSv4.1 do not clear NFS_DELEAGED_STATE until stateid is tested

[Myklebust, Trond]

T24gVHVlLCAyMDEyLTAzLTA2IGF0IDEwOjEyIC0wNTAwLCBBbmR5IEFkYW1zb24gd3JvdGU6DQo+
IE9uIFR1ZSwgTWFyIDYsIDIwMTIgYXQgOTo1MyBBTSwgTXlrbGVidXN0LCBUcm9uZA0KPiA8VHJv
bmQuTXlrbGVidXN0QG5ldGFwcC5jb20+IHdyb3RlOg0KPiA+IE9uIFR1ZSwgMjAxMi0wMy0wNiBh
dCAwOTo0NiAtMDUwMCwgYW5kcm9zQG5ldGFwcC5jb20gd3JvdGU6DQo+ID4+IEZyb206IEFuZHkg
QWRhbXNvbiA8YW5kcm9zQG5ldGFwcC5jb20+DQo+ID4+DQo+ID4+IG5mczQxX29wZW5fZXhwaXJl
ZCgpIHdpbGwgdGVzdCB0aGUgZGVsZWdhdGlvbiBzdGF0ZWlkIGJhc2VkIG9uDQo+ID4+IE5GU19E
RUxFR0FURURfU1RBVEUgYmVpbmcgc2V0LiBJZiB0aGUgc3RhdGVpZCBpcyBiYWQsIG5mczRfb3Bl
bl9yZWNvdmVyDQo+ID4+IHdpbGwgY2xlYXIgdGhlIE5GU19ERUxFR0FURURfU1RBVEUgYml0IHJl
Y292ZXJpbmcgdGhlIGRlbGVnYXRpb24uDQo+ID4+DQo+ID4+IFNpZ25lZC1vZmYtYnk6IEFuZHkg
QWRhbXNvbiA8YW5kcm9zQG5ldGFwcC5jb20+DQo+ID4+IENjOiBzdGFibGVAdmdlci5rZXJuZWwu
b3JnDQo+ID4+IC0tLQ0KPiA+PiAgZnMvbmZzL25mczRzdGF0ZS5jIHwgICAgNSArKysrLQ0KPiA+
PiAgMSBmaWxlcyBjaGFuZ2VkLCA0IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb25zKC0pDQo+ID4+
DQo+ID4+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHN0YXRlLmMgYi9mcy9uZnMvbmZzNHN0YXRl
LmMNCj4gPj4gaW5kZXggN2JkOTgyMi4uNDRmY2Q2MCAxMDA2NDQNCj4gPj4gLS0tIGEvZnMvbmZz
L25mczRzdGF0ZS5jDQo+ID4+ICsrKyBiL2ZzL25mcy9uZnM0c3RhdGUuYw0KPiA+PiBAQCAtNjk2
LDcgKzY5NiwxMCBAQCBzdGF0aWMgdm9pZCBfX25mczRfY2xvc2Uoc3RydWN0IG5mczRfc3RhdGUg
KnN0YXRlLA0KPiA+PiAgICAgICAgICAgICAgICAgICAgICAgY2FsbF9jbG9zZSB8PSB0ZXN0X2Jp
dChORlNfT19SRFdSX1NUQVRFLCAmc3RhdGUtPmZsYWdzKTsNCj4gPj4gICAgICAgICAgICAgICB9
DQo+ID4+ICAgICAgICAgICAgICAgaWYgKG5ld3N0YXRlID09IDApDQo+ID4+ICt7DQo+ID4+ICtw
cmludGsoIiVzIENMRUFSIE5GU19ERUxFR0FURURfU1RBVEUgc3RhdGUgJXBcbiIsIF9fZnVuY19f
LCBzdGF0ZSk7DQo+ID4+ICAgICAgICAgICAgICAgICAgICAgICBjbGVhcl9iaXQoTkZTX0RFTEVH
QVRFRF9TVEFURSwgJnN0YXRlLT5mbGFncyk7DQo+ID4+ICt9XA0KPiA+DQo+ID4gRXd3dy4uLi4u
DQo+ID4NCj4gPj4gICAgICAgfQ0KPiA+PiAgICAgICBuZnM0X3N0YXRlX3NldF9tb2RlX2xvY2tl
ZChzdGF0ZSwgbmV3c3RhdGUpOw0KPiA+PiAgICAgICBzcGluX3VubG9jaygmb3duZXItPnNvX2xv
Y2spOw0KPiA+PiBAQCAtMTA5Nyw3ICsxMTAwLDcgQEAgdm9pZCBuZnM0X3NjaGVkdWxlX3N0YXRl
aWRfcmVjb3ZlcnkoY29uc3Qgc3RydWN0IG5mc19zZXJ2ZXIgKnNlcnZlciwgc3RydWN0IG5mczQN
Cj4gPj4gIHsNCj4gPj4gICAgICAgc3RydWN0IG5mc19jbGllbnQgKmNscCA9IHNlcnZlci0+bmZz
X2NsaWVudDsNCj4gPj4NCj4gPj4gLSAgICAgaWYgKHRlc3RfYW5kX2NsZWFyX2JpdChORlNfREVM
RUdBVEVEX1NUQVRFLCAmc3RhdGUtPmZsYWdzKSkNCj4gPj4gKyAgICAgaWYgKHRlc3RfYml0KE5G
U19ERUxFR0FURURfU1RBVEUsICZzdGF0ZS0+ZmxhZ3MpKQ0KPiA+PiAgICAgICAgICAgICAgIG5m
c19hc3luY19pbm9kZV9yZXR1cm5fZGVsZWdhdGlvbihzdGF0ZS0+aW5vZGUsICZzdGF0ZS0+c3Rh
dGVpZCk7DQo+ID4+ICAgICAgIG5mczRfc3RhdGVfbWFya19yZWNsYWltX25vZ3JhY2UoY2xwLCBz
dGF0ZSk7DQo+ID4+ICAgICAgIG5mczRfc2NoZWR1bGVfc3RhdGVfbWFuYWdlcihjbHApOw0KPiA+
DQo+ID4gQWN0dWFsbHksIHRoZSBsYXRlc3QgaW5jYXJuYXRpb24gb2YgdGhlIHBhdGNoIDIvMyBy
ZW1vdmVzIHRoZQ0KPiA+IGFzeW5jX2lub2RlX3JldHVybl9kZWxlZ2F0aW9uIGZyb20gbmZzNF9z
Y2hlZHVsZV9zdGF0ZWlkX3JlY292ZXJ5Lg0KPiANCj4gT0sgLSBidXQgSSB0aGluayB3ZSBzdGls
bCBuZWVkIHRvIHBvc3Rwb25lIGNsZWFyaW5nIHRoZQ0KPiBORlNfREVMRUdBVEVEX1NUQVRFIGJp
dCB1bnRpbCBhZnRlciBjYWxsaW5nIG5mczQxX29wZW5fZXhwaXJlZC4NCj4gT3RoZXJ3aXNlIG5m
czQxX2NoZWNrX2V4cGlyZWRfc3RhdGVpZCB3aWxsIG5vdCB0ZXN0IHRoZSBkZWxlZ2F0aW9uDQo+
IHN0YXRlaWQsIGFuZCBuZnM0X29wZW5fcmVjb3ZlciB3aWxsIG5vdCBiZSBjYWxsZWQsIGFzIG5v
IHN0YXRlaWQgaXMNCj4gZGVlbWVkIGJhZCwgYW5kIHRoZSBPUEVOIHdpdGggQ0xBSU1fTlVMTCB0
byAocG90ZW50aWFsbHkpIHJlY292ZXIgdGhlDQo+IGRlbGVnYXRpb24gd2lsbCBub3QgYmUgc2Vu
dC4gSW4gdGhpcyBjYXNlLCB0aGUgb2xkIGJhZCBkZWxlZ2F0aW9uDQo+IHN0YXRlaWQgd2lsbCBi
ZSByZXNlbnQuDQoNCkFncmVlZC4gUGxlYXNlIHNlZSB0aGUgJ3YyJyBwYXRjaCB0aGF0IEkgc2Vu
dCB5b3U6IGl0IHJlbW92ZXMgX2JvdGhfDQpsaW5lcyBhYm92ZSAodGhlIHRlc3QvY2xlYXJpbmcg
b2YgdGhlIGJpdCBhbmQgdGhlIGRlbGVnYXRpb24gcmV0dXJuKS4NCg0KLS0gDQpUcm9uZCBNeWts
ZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xl
YnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBwLmNvbQ0KDQo=