[PATCH 0/4] Address several issues with TEST_STATEID

[PATCH 0/4] Address several issues with TEST_STATEID

[Chuck Lever]

Hi-

While testing the recently introduced PURGE_STATE logic, I found some
issues with the error handling logic under nfs41_open_expired().

After fixing these issues, I am able to get Connectathon basic,
general, and special tests to pass with a server rigged to assert
SEQ4_STATUS_ADMIN_STATE_REVOKED at regular intervals.  The
Connectathon lock tests encounter a deadlock, which I've already
reported, that appears to pre-date PURGE_STATE.

---

Chuck Lever (4):
      NFS: Refactor nfs41_check_expired_stateid()
      NFS: state reclaim clears OPEN state
      NFS: Properly sort TEST_STATEID results
      NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting


 fs/nfs/nfs4proc.c |  144 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 115 insertions(+), 29 deletions(-)

-- 
Chuck Lever

[PATCH 1/4] NFS: Clean up TEST_STATEID and FREE_STATEID proc error reporting

[Chuck Lever]

The TEST_STATEID and FREE_STATEID operations can return
NFS4ERR_BAD_STATEID, NFS4ERR_OLD_STATEID, or NFS4ERR_DEADSESSION.
These error values should not be passed to nfs4_handle_exception(),
since that will recursively kick off state recovery, resulting in a
deadlock.

The reason to continue using nfs4_handle_exception() is to deal with
NFS4ERR_DELAY.  [ Sidebar: It seems to me we are dealing with only
DELAY in enough places now that it might make sense to add a version
of nfs4_handle_exception() that handles only GRACE and DELAY) ].

Moreover, specifically when the TEST_STATEID operation returns
NFS4_OK, res.status can contain one of these errors.
_nfs41_test_stateid() replaces NFS4_OK with the value in res.status,
which is then returned to callers.

But res.status is not passed through nfs4_stat_to_errno(), and thus is
a positive NFS4ERR value.  Currently callers are only interested in
!NFS4_OK, and nfs4_handle_exception() ignores positive values, so this
oversight hasn't bitten us so far.

Bryan agrees the original intent was to return res.status as a
negative NFS4ERR value to callers of nfs41_test_stateid(), as long as
no state ID recovery is attempted.

As a finishing touch, add appropriate documenting comments and some
debugging printk's.  Documenting comments are added to record the
intent of the return value.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/nfs4proc.c |   51 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 0d46fe4..4f1e824 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6384,22 +6384,36 @@ static int _nfs41_test_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 		.rpc_resp = &res,
 	};
 
+	dprintk("NFS call  test_stateid %p\n", stateid);
 	nfs41_init_sequence(&args.seq_args, &res.seq_res, 0);
 	status = nfs4_call_sync_sequence(server->client, server, &msg, &args.seq_args, &res.seq_res, 1);
-
-	if (status == NFS_OK)
-		return res.status;
-	return status;
+	if (status != NFS_OK) {
+		dprintk("NFS reply test_stateid: failed, %d\n", status);
+		return status;
+	}
+	dprintk("NFS reply test_stateid: succeeded, %d\n", -res.status);
+	return -res.status;
 }
 
+/**
+ * nfs41_test_stateid - perform a TEST_STATEID operation
+ *
+ * @server: NFS server that may know about "stateid"
+ * @stateid: state ID to test
+ *
+ * Returns NFS_OK if the server recognizes the state ID as valid.
+ * Otherwise a negative NFS4ERR value is returned if the operation
+ * failed or the state ID is not currently valid.
+ */
 static int nfs41_test_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 {
 	struct nfs4_exception exception = { };
 	int err;
 	do {
-		err = nfs4_handle_exception(server,
-				_nfs41_test_stateid(server, stateid),
-				&exception);
+		err = _nfs41_test_stateid(server, stateid);
+		if (err != -NFS4ERR_DELAY)
+			break;
+		nfs4_handle_exception(server, err, &exception);
 	} while (exception.retry);
 	return err;
 }
@@ -6415,19 +6429,34 @@ static int _nfs4_free_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 		.rpc_argp = &args,
 		.rpc_resp = &res,
 	};
+	int status;
 
+	dprintk("NFS call  free_stateid %p\n", stateid);
 	nfs41_init_sequence(&args.seq_args, &res.seq_res, 0);
-	return nfs4_call_sync_sequence(server->client, server, &msg, &args.seq_args, &res.seq_res, 1);
+	status = nfs4_call_sync_sequence(server->client, server, &msg,
+					 &args.seq_args, &res.seq_res, 1);
+	dprintk("NFS reply free_stateid: %d\n", status);
+	return status;
 }
 
+/**
+ * nfs41_free_stateid - perform a FREE_STATEID operation
+ *
+ * @server: NFS server that may know about "stateid"
+ * @stateid: state ID to release
+ *
+ * Returns NFS_OK if the server freed the state ID.  Otherwise a
+ * negative NFS4ERR value is returned if the operation failed.
+ */
 static int nfs41_free_stateid(struct nfs_server *server, nfs4_stateid *stateid)
 {
 	struct nfs4_exception exception = { };
 	int err;
 	do {
-		err = nfs4_handle_exception(server,
-				_nfs4_free_stateid(server, stateid),
-				&exception);
+		err = _nfs4_free_stateid(server, stateid);
+		if (err != -NFS4ERR_DELAY)
+			break;
+		nfs4_handle_exception(server, err, &exception);
 	} while (exception.retry);
 	return err;
 }

[PATCH 2/4] NFS: Properly sort TEST_STATEID results

[Chuck Lever]

The result of a TEST_STATEID operation can indicate a few different
things.  If NFS_OK is returned, then the client can continue using the
state ID under test, and skip recovery.  RFC 5661 says that if and
only if the state ID was revoked, then the client must perform an
explicit FREE_STATEID before trying to re-open.

However, if the server doesn't recognize the state ID at all, then the
client should immediately continue with open recovery.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/nfs4proc.c |   38 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 4f1e824..39d641a 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1733,6 +1733,14 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
 }
 
 #if defined(CONFIG_NFS_V4_1)
+/**
+ * nfs41_check_expired_stateid - does a state ID need recovery?
+ *
+ * @state: NFSv4 open state for a file
+ *
+ * Returns NFS_OK if recovery for this state ID is now finished.
+ * Otherwise a negative NFS4ERR value is returned.
+ */
 static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
 {
 	int status = NFS_OK;
@@ -1740,9 +1748,19 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
 
 	if (state->flags & flags) {
 		status = nfs41_test_stateid(server, stateid);
-		if (status != NFS_OK) {
+		switch (status) {
+		case NFS_OK:
+			/* server recognizes this one, don't re-open */
+			break;
+		case -NFS4ERR_ADMIN_REVOKED:
+		case -NFS4ERR_DELEG_REVOKED:
+			/* state was revoked, free then re-open */
 			nfs41_free_stateid(server, stateid);
 			state->flags &= ~flags;
+			break;
+		default:
+			/* anything else: go ahead and re-open it */
+			break;
 		}
 	}
 	return status;
@@ -4642,6 +4660,14 @@ out:
 }
 
 #if defined(CONFIG_NFS_V4_1)
+/**
+ * nfs41_check_expired_locks - clear lock state IDs
+ *
+ * @state: NFSv4 open state for a file
+ *
+ * Returns NFS_OK if recovery for this state ID is now finished.
+ * Otherwise a negative NFS4ERR value is returned.
+ */
 static int nfs41_check_expired_locks(struct nfs4_state *state)
 {
 	int status, ret = NFS_OK;
@@ -4651,9 +4677,17 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
 	list_for_each_entry(lsp, &state->lock_states, ls_locks) {
 		if (lsp->ls_flags & NFS_LOCK_INITIALIZED) {
 			status = nfs41_test_stateid(server, &lsp->ls_stateid);
-			if (status != NFS_OK) {
+			switch (status) {
+			case NFS_OK:
+				/* server recognizes this one, don't re-lock */
+				break;
+			case -NFS4ERR_ADMIN_REVOKED:
+			case -NFS4ERR_DELEG_REVOKED:
+				/* lock was revoked, free then re-lock */
 				nfs41_free_stateid(server, &lsp->ls_stateid);
 				lsp->ls_flags &= ~NFS_LOCK_INITIALIZED;
+				/*FALLTHROUGH*/
+			default:
 				ret = status;
 			}
 		}

Re: [PATCH 2/4] NFS: Properly sort TEST_STATEID results

[Chuck Lever]

On Jun 1, 2012, at 11:38 AM, Chuck Lever wrote:

> The result of a TEST_STATEID operation can indicate a few different
> things.  If NFS_OK is returned, then the client can continue using the
> state ID under test, and skip recovery.  RFC 5661 says that if and
> only if the state ID was revoked, then the client must perform an
> explicit FREE_STATEID before trying to re-open.
> 
> However, if the server doesn't recognize the state ID at all, then the
> client should immediately continue with open recovery.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> fs/nfs/nfs4proc.c |   38 ++++++++++++++++++++++++++++++++++++--
> 1 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 4f1e824..39d641a 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -1733,6 +1733,14 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
> }
> 
> #if defined(CONFIG_NFS_V4_1)
> +/**
> + * nfs41_check_expired_stateid - does a state ID need recovery?
> + *
> + * @state: NFSv4 open state for a file
> + *
> + * Returns NFS_OK if recovery for this state ID is now finished.
> + * Otherwise a negative NFS4ERR value is returned.
> + */
> static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
> {
> 	int status = NFS_OK;
> @@ -1740,9 +1748,19 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
> 
> 	if (state->flags & flags) {
> 		status = nfs41_test_stateid(server, stateid);
> -		if (status != NFS_OK) {
> +		switch (status) {
> +		case NFS_OK:
> +			/* server recognizes this one, don't re-open */
> +			break;
> +		case -NFS4ERR_ADMIN_REVOKED:
> +		case -NFS4ERR_DELEG_REVOKED:
> +			/* state was revoked, free then re-open */
> 			nfs41_free_stateid(server, stateid);
> 			state->flags &= ~flags;

Actually, I think we want to clear these flags in any case when status != NFS_OK.

> +			break;
> +		default:
> +			/* anything else: go ahead and re-open it */
> +			break;
> 		}
> 	}
> 	return status;
> @@ -4642,6 +4660,14 @@ out:
> }
> 
> #if defined(CONFIG_NFS_V4_1)
> +/**
> + * nfs41_check_expired_locks - clear lock state IDs
> + *
> + * @state: NFSv4 open state for a file
> + *
> + * Returns NFS_OK if recovery for this state ID is now finished.
> + * Otherwise a negative NFS4ERR value is returned.
> + */
> static int nfs41_check_expired_locks(struct nfs4_state *state)
> {
> 	int status, ret = NFS_OK;
> @@ -4651,9 +4677,17 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
> 	list_for_each_entry(lsp, &state->lock_states, ls_locks) {
> 		if (lsp->ls_flags & NFS_LOCK_INITIALIZED) {
> 			status = nfs41_test_stateid(server, &lsp->ls_stateid);
> -			if (status != NFS_OK) {
> +			switch (status) {
> +			case NFS_OK:
> +				/* server recognizes this one, don't re-lock */
> +				break;
> +			case -NFS4ERR_ADMIN_REVOKED:
> +			case -NFS4ERR_DELEG_REVOKED:
> +				/* lock was revoked, free then re-lock */
> 				nfs41_free_stateid(server, &lsp->ls_stateid);
> 				lsp->ls_flags &= ~NFS_LOCK_INITIALIZED;
> +				/*FALLTHROUGH*/
> +			default:
> 				ret = status;
> 			}
> 		}
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH 2/4] NFS: Properly sort TEST_STATEID results

[Myklebust, Trond]

T24gRnJpLCAyMDEyLTA2LTAxIGF0IDExOjM4IC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
VGhlIHJlc3VsdCBvZiBhIFRFU1RfU1RBVEVJRCBvcGVyYXRpb24gY2FuIGluZGljYXRlIGEgZmV3
IGRpZmZlcmVudA0KPiB0aGluZ3MuICBJZiBORlNfT0sgaXMgcmV0dXJuZWQsIHRoZW4gdGhlIGNs
aWVudCBjYW4gY29udGludWUgdXNpbmcgdGhlDQo+IHN0YXRlIElEIHVuZGVyIHRlc3QsIGFuZCBz
a2lwIHJlY292ZXJ5LiAgUkZDIDU2NjEgc2F5cyB0aGF0IGlmIGFuZA0KPiBvbmx5IGlmIHRoZSBz
dGF0ZSBJRCB3YXMgcmV2b2tlZCwgdGhlbiB0aGUgY2xpZW50IG11c3QgcGVyZm9ybSBhbg0KPiBl
eHBsaWNpdCBGUkVFX1NUQVRFSUQgYmVmb3JlIHRyeWluZyB0byByZS1vcGVuLg0KPiANCj4gSG93
ZXZlciwgaWYgdGhlIHNlcnZlciBkb2Vzbid0IHJlY29nbml6ZSB0aGUgc3RhdGUgSUQgYXQgYWxs
LCB0aGVuIHRoZQ0KPiBjbGllbnQgc2hvdWxkIGltbWVkaWF0ZWx5IGNvbnRpbnVlIHdpdGggb3Bl
biByZWNvdmVyeS4NCj4gDQo+IFNpZ25lZC1vZmYtYnk6IENodWNrIExldmVyIDxjaHVjay5sZXZl
ckBvcmFjbGUuY29tPg0KPiAtLS0NCj4gDQo+ICBmcy9uZnMvbmZzNHByb2MuYyB8ICAgMzggKysr
KysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLS0NCj4gIDEgZmlsZXMgY2hhbmdlZCwg
MzYgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCj4gDQo+IGRpZmYgLS1naXQgYS9mcy9u
ZnMvbmZzNHByb2MuYyBiL2ZzL25mcy9uZnM0cHJvYy5jDQo+IGluZGV4IDRmMWU4MjQuLjM5ZDY0
MWEgMTAwNjQ0DQo+IC0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQo+ICsrKyBiL2ZzL25mcy9uZnM0
cHJvYy5jDQo+IEBAIC0xNzMzLDYgKzE3MzMsMTQgQEAgc3RhdGljIGludCBuZnM0X29wZW5fZXhw
aXJlZChzdHJ1Y3QgbmZzNF9zdGF0ZV9vd25lciAqc3AsIHN0cnVjdCBuZnM0X3N0YXRlICpzdGEN
Cj4gIH0NCj4gIA0KPiAgI2lmIGRlZmluZWQoQ09ORklHX05GU19WNF8xKQ0KPiArLyoqDQo+ICsg
KiBuZnM0MV9jaGVja19leHBpcmVkX3N0YXRlaWQgLSBkb2VzIGEgc3RhdGUgSUQgbmVlZCByZWNv
dmVyeT8NCj4gKyAqDQo+ICsgKiBAc3RhdGU6IE5GU3Y0IG9wZW4gc3RhdGUgZm9yIGEgZmlsZQ0K
PiArICoNCj4gKyAqIFJldHVybnMgTkZTX09LIGlmIHJlY292ZXJ5IGZvciB0aGlzIHN0YXRlIElE
IGlzIG5vdyBmaW5pc2hlZC4NCj4gKyAqIE90aGVyd2lzZSBhIG5lZ2F0aXZlIE5GUzRFUlIgdmFs
dWUgaXMgcmV0dXJuZWQuDQo+ICsgKi8NCj4gIHN0YXRpYyBpbnQgbmZzNDFfY2hlY2tfZXhwaXJl
ZF9zdGF0ZWlkKHN0cnVjdCBuZnM0X3N0YXRlICpzdGF0ZSwgbmZzNF9zdGF0ZWlkICpzdGF0ZWlk
LCB1bnNpZ25lZCBpbnQgZmxhZ3MpDQo+ICB7DQo+ICAJaW50IHN0YXR1cyA9IE5GU19PSzsNCj4g
QEAgLTE3NDAsOSArMTc0OCwxOSBAQCBzdGF0aWMgaW50IG5mczQxX2NoZWNrX2V4cGlyZWRfc3Rh
dGVpZChzdHJ1Y3QgbmZzNF9zdGF0ZSAqc3RhdGUsIG5mczRfc3RhdGVpZCAqcw0KPiAgDQo+ICAJ
aWYgKHN0YXRlLT5mbGFncyAmIGZsYWdzKSB7DQo+ICAJCXN0YXR1cyA9IG5mczQxX3Rlc3Rfc3Rh
dGVpZChzZXJ2ZXIsIHN0YXRlaWQpOw0KPiAtCQlpZiAoc3RhdHVzICE9IE5GU19PSykgew0KPiAr
CQlzd2l0Y2ggKHN0YXR1cykgew0KPiArCQljYXNlIE5GU19PSzoNCj4gKwkJCS8qIHNlcnZlciBy
ZWNvZ25pemVzIHRoaXMgb25lLCBkb24ndCByZS1vcGVuICovDQo+ICsJCQlicmVhazsNCj4gKwkJ
Y2FzZSAtTkZTNEVSUl9BRE1JTl9SRVZPS0VEOg0KPiArCQljYXNlIC1ORlM0RVJSX0RFTEVHX1JF
Vk9LRUQ6DQo+ICsJCQkvKiBzdGF0ZSB3YXMgcmV2b2tlZCwgZnJlZSB0aGVuIHJlLW9wZW4gKi8N
Cj4gIAkJCW5mczQxX2ZyZWVfc3RhdGVpZChzZXJ2ZXIsIHN0YXRlaWQpOw0KPiAgCQkJc3RhdGUt
PmZsYWdzICY9IH5mbGFnczsNCj4gKwkJCWJyZWFrOw0KPiArCQlkZWZhdWx0Og0KDQpQbGVhc2Ug
ZG9uJ3QgYWRkIGVtcHR5ICJkZWZhdWx0OiIgY2FzZXMuIA0KDQo+ICsJCQkvKiBhbnl0aGluZyBl
bHNlOiBnbyBhaGVhZCBhbmQgcmUtb3BlbiBpdCAqLw0KPiArCQkJYnJlYWs7DQoNClRoYXQgbGFz
dCAiYnJlYWsiIGlzIHVubmVjZXNzYXJ5IHRvbyBpbiBzdGFuZGFyZCBDOTkuDQoNCj4gIAkJfQ0K
PiAgCX0NCj4gIAlyZXR1cm4gc3RhdHVzOw0KPiBAQCAtNDY0Miw2ICs0NjYwLDE0IEBAIG91dDoN
Cj4gIH0NCj4gIA0KPiAgI2lmIGRlZmluZWQoQ09ORklHX05GU19WNF8xKQ0KPiArLyoqDQo+ICsg
KiBuZnM0MV9jaGVja19leHBpcmVkX2xvY2tzIC0gY2xlYXIgbG9jayBzdGF0ZSBJRHMNCj4gKyAq
DQo+ICsgKiBAc3RhdGU6IE5GU3Y0IG9wZW4gc3RhdGUgZm9yIGEgZmlsZQ0KPiArICoNCj4gKyAq
IFJldHVybnMgTkZTX09LIGlmIHJlY292ZXJ5IGZvciB0aGlzIHN0YXRlIElEIGlzIG5vdyBmaW5p
c2hlZC4NCj4gKyAqIE90aGVyd2lzZSBhIG5lZ2F0aXZlIE5GUzRFUlIgdmFsdWUgaXMgcmV0dXJu
ZWQuDQo+ICsgKi8NCj4gIHN0YXRpYyBpbnQgbmZzNDFfY2hlY2tfZXhwaXJlZF9sb2NrcyhzdHJ1
Y3QgbmZzNF9zdGF0ZSAqc3RhdGUpDQo+ICB7DQo+ICAJaW50IHN0YXR1cywgcmV0ID0gTkZTX09L
Ow0KPiBAQCAtNDY1MSw5ICs0Njc3LDE3IEBAIHN0YXRpYyBpbnQgbmZzNDFfY2hlY2tfZXhwaXJl
ZF9sb2NrcyhzdHJ1Y3QgbmZzNF9zdGF0ZSAqc3RhdGUpDQo+ICAJbGlzdF9mb3JfZWFjaF9lbnRy
eShsc3AsICZzdGF0ZS0+bG9ja19zdGF0ZXMsIGxzX2xvY2tzKSB7DQo+ICAJCWlmIChsc3AtPmxz
X2ZsYWdzICYgTkZTX0xPQ0tfSU5JVElBTElaRUQpIHsNCj4gIAkJCXN0YXR1cyA9IG5mczQxX3Rl
c3Rfc3RhdGVpZChzZXJ2ZXIsICZsc3AtPmxzX3N0YXRlaWQpOw0KPiAtCQkJaWYgKHN0YXR1cyAh
PSBORlNfT0spIHsNCj4gKwkJCXN3aXRjaCAoc3RhdHVzKSB7DQo+ICsJCQljYXNlIE5GU19PSzoN
Cj4gKwkJCQkvKiBzZXJ2ZXIgcmVjb2duaXplcyB0aGlzIG9uZSwgZG9uJ3QgcmUtbG9jayAqLw0K
PiArCQkJCWJyZWFrOw0KPiArCQkJY2FzZSAtTkZTNEVSUl9BRE1JTl9SRVZPS0VEOg0KPiArCQkJ
Y2FzZSAtTkZTNEVSUl9ERUxFR19SRVZPS0VEOg0KPiArCQkJCS8qIGxvY2sgd2FzIHJldm9rZWQs
IGZyZWUgdGhlbiByZS1sb2NrICovDQo+ICAJCQkJbmZzNDFfZnJlZV9zdGF0ZWlkKHNlcnZlciwg
JmxzcC0+bHNfc3RhdGVpZCk7DQo+ICAJCQkJbHNwLT5sc19mbGFncyAmPSB+TkZTX0xPQ0tfSU5J
VElBTElaRUQ7DQo+ICsJCQkJLypGQUxMVEhST1VHSCovDQoNCldoeSBkbyB3ZSBuZWVkIHRoaXMg
Y29tbWVudD8gSXQgc2hvdWxkIGJlIG9idmlvdXMuDQoNCj4gKwkJCWRlZmF1bHQ6DQo+ICAJCQkJ
cmV0ID0gc3RhdHVzOw0KPiAgCQkJfQ0KPiAgCQl9DQo+IA0KPiAtLQ0KPiBUbyB1bnN1YnNjcmli
ZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtbmZzIiBp
bg0KPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZw0K
PiBNb3JlIG1ham9yZG9tbyBpbmZvIGF0ICBodHRwOi8vdmdlci5rZXJuZWwub3JnL21ham9yZG9t
by1pbmZvLmh0bWwNCg0KLS0gDQpUcm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFp
bnRhaW5lcg0KDQpOZXRBcHANClRyb25kLk15a2xlYnVzdEBuZXRhcHAuY29tDQp3d3cubmV0YXBw
LmNvbQ0KDQo=

Re: [PATCH 2/4] NFS: Properly sort TEST_STATEID results

[Chuck Lever]

On Jun 1, 2012, at 12:01 PM, Myklebust, Trond wrote:

> On Fri, 2012-06-01 at 11:38 -0400, Chuck Lever wrote:
>> 
>> @@ -1740,9 +1748,19 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
>> 
>> 	if (state->flags & flags) {
>> 		status = nfs41_test_stateid(server, stateid);
>> -		if (status != NFS_OK) {
>> +		switch (status) {
>> +		case NFS_OK:
>> +			/* server recognizes this one, don't re-open */
>> +			break;
>> +		case -NFS4ERR_ADMIN_REVOKED:
>> +		case -NFS4ERR_DELEG_REVOKED:
>> +			/* state was revoked, free then re-open */
>> 			nfs41_free_stateid(server, stateid);
>> 			state->flags &= ~flags;
>> +			break;
>> +		default:
> 
> Please don't add empty "default:" cases. 
> 
>> +			/* anything else: go ahead and re-open it */
>> +			break;
> 
> That last "break" is unnecessary too in standard C99.

OK, all fixed for next version.  I'll wait for additional comments before reposting.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

[PATCH 3/4] NFS: state reclaim clears OPEN state

[Chuck Lever]

Before beginning state recovery, the state manager zaps open and lock
state for each open file, thus the "state->flags & flags" test always
fails during reclaim.  But open recovery is still needed for these
files.

To force a call to nfs4_open_expired(), change the default return
value for nfs41_check_expired_stateid() to force open recovery and
nfs41_check_locks() to force lock recovery if the requested flags are
clear.  Fix suggested by Bryan Schumaker.

The presence of a delegation state ID must not prevent normal open
recovery.  The delegation state ID must be cleared if it was revoked,
but once cleared I don't think it's presence or absence has any
bearing on whether open recovery is still needed.

The logic in nfs41_lock_expired() is cleaned up to match the form of
nfs41_open_expired(), but no change in behavior is expected there.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/nfs4proc.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 39d641a..676d341 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1743,8 +1743,8 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
  */
 static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
 {
-	int status = NFS_OK;
 	struct nfs_server *server = NFS_SERVER(state->inode);
+	int status = -NFS4ERR_BAD_STATEID;
 
 	if (state->flags & flags) {
 		status = nfs41_test_stateid(server, stateid);
@@ -1768,16 +1768,16 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
 
 static int nfs41_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *state)
 {
-	int deleg_status, open_status;
 	int deleg_flags = 1 << NFS_DELEGATED_STATE;
 	int open_flags = (1 << NFS_O_RDONLY_STATE) | (1 << NFS_O_WRONLY_STATE) | (1 << NFS_O_RDWR_STATE);
+	int status;
 
-	deleg_status = nfs41_check_expired_stateid(state, &state->stateid, deleg_flags);
-	open_status = nfs41_check_expired_stateid(state,  &state->open_stateid, open_flags);
+	nfs41_check_expired_stateid(state, &state->stateid, deleg_flags);
+	status = nfs41_check_expired_stateid(state,  &state->open_stateid, open_flags);
 
-	if ((deleg_status == NFS_OK) && (open_status == NFS_OK))
-		return NFS_OK;
-	return nfs4_open_expired(sp, state);
+	if (status != NFS_OK)
+		status = nfs4_open_expired(sp, state);
+	return status;
 }
 #endif
 
@@ -4670,7 +4670,7 @@ out:
  */
 static int nfs41_check_expired_locks(struct nfs4_state *state)
 {
-	int status, ret = NFS_OK;
+	int status, ret = -NFS4ERR_BAD_STATEID;
 	struct nfs4_lock_state *lsp;
 	struct nfs_server *server = NFS_SERVER(state->inode);
 
@@ -4702,9 +4702,9 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
 
 	if (test_bit(LK_STATE_IN_USE, &state->flags))
 		status = nfs41_check_expired_locks(state);
-	if (status == NFS_OK)
-		return status;
-	return nfs4_lock_expired(state, request);
+	if (status != NFS_OK)
+		status = nfs4_lock_expired(state, request);
+	return status;
 }
 #endif
 

[PATCH 4/4] NFS: Refactor nfs41_check_expired_stateid()

[Chuck Lever]

Clean up: The logic for dealing with a delegation state ID and an open
state ID is now sufficiently distinct that I've refactored
nfs41_check_expired_stateid() into two separate functions.

Instead of open-coded flag manipulation, use test_bit() and
clear_bit() just like all other accessors of the state->flag field.

This also eliminates a bunch of unnecessary integer type conversions.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/nfs4proc.c |   41 ++++++++++++++++++++++++++++++++---------
 1 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 676d341..abb6e6b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -1734,19 +1734,42 @@ static int nfs4_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *sta
 
 #if defined(CONFIG_NFS_V4_1)
 /**
- * nfs41_check_expired_stateid - does a state ID need recovery?
+ * nfs41_clear_delegation_stateid - possibly clear revoked delegation state ID
  *
- * @state: NFSv4 open state for a file
+ * @state: NFSv4 open state for an inode
+ */
+static void nfs41_clear_delegation_stateid(struct nfs4_state *state)
+{
+	struct nfs_server *server = NFS_SERVER(state->inode);
+	nfs4_stateid *stateid = &state->stateid;
+
+	if (test_bit(NFS_DELEGATED_STATE, &state->flags) != 0)
+		switch (nfs41_test_stateid(server, stateid)) {
+		case -NFS4ERR_ADMIN_REVOKED:
+		case -NFS4ERR_DELEG_REVOKED:
+			nfs41_free_stateid(server, stateid);
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+			break;
+		}
+}
+
+/**
+ * nfs41_check_open_stateid - clear open state ID
+ *
+ * @state: NFSv4 open state for an inode
  *
  * Returns NFS_OK if recovery for this state ID is now finished.
  * Otherwise a negative NFS4ERR value is returned.
  */
-static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *stateid, unsigned int flags)
+static int nfs41_check_open_stateid(struct nfs4_state *state)
 {
 	struct nfs_server *server = NFS_SERVER(state->inode);
+	nfs4_stateid *stateid = &state->stateid;
 	int status = -NFS4ERR_BAD_STATEID;
 
-	if (state->flags & flags) {
+	if ((test_bit(NFS_O_RDONLY_STATE, &state->flags) != 0) ||
+	    (test_bit(NFS_O_WRONLY_STATE, &state->flags) != 0) ||
+	    (test_bit(NFS_O_RDWR_STATE, &state->flags) != 0)) {
 		status = nfs41_test_stateid(server, stateid);
 		switch (status) {
 		case NFS_OK:
@@ -1756,7 +1779,9 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
 		case -NFS4ERR_DELEG_REVOKED:
 			/* state was revoked, free then re-open */
 			nfs41_free_stateid(server, stateid);
-			state->flags &= ~flags;
+			clear_bit(NFS_O_RDONLY_STATE, &state->flags);
+			clear_bit(NFS_O_WRONLY_STATE, &state->flags);
+			clear_bit(NFS_O_RDWR_STATE, &state->flags);
 			break;
 		default:
 			/* anything else: go ahead and re-open it */
@@ -1768,12 +1793,10 @@ static int nfs41_check_expired_stateid(struct nfs4_state *state, nfs4_stateid *s
 
 static int nfs41_open_expired(struct nfs4_state_owner *sp, struct nfs4_state *state)
 {
-	int deleg_flags = 1 << NFS_DELEGATED_STATE;
-	int open_flags = (1 << NFS_O_RDONLY_STATE) | (1 << NFS_O_WRONLY_STATE) | (1 << NFS_O_RDWR_STATE);
 	int status;
 
-	nfs41_check_expired_stateid(state, &state->stateid, deleg_flags);
-	status = nfs41_check_expired_stateid(state,  &state->open_stateid, open_flags);
+	nfs41_clear_delegation_stateid(state);
+	status = nfs41_check_open_stateid(state);
 
 	if (status != NFS_OK)
 		status = nfs4_open_expired(sp, state);