Skip to contentSkip to conversation

Fix bug that client use a invaild delegation

PATCH In review Bian Naimeng 3 participants 22 messages Aug 4, 2010 – Dec 2, 2010 1 message not in the archive

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Bian Naimeng

author
#0 lore raw

cover Fix bug that client use a invaild delegation

Hi all

I find a bug at RHEL6Beta2. If client return nfsi->delegation, some open_stateid
still have the NFS_DELEGATED_STATE bit, and another open process which has some owner will not clear this bit, it will not use the new open stateid to fill state->stateid, just fill state->open_stateid.
Then other IO operation will use the old delegation, it will get NFS4ERR_BAD_STATEID.

Thought my test program just FAIL at RHEL6Beta2, latest kernel is OK,
but i thinks the latest kernel maybe have the some bug.

---

Bian Naimeng (2):
[PATCH 1/2] Make lock inode before lock nfs4_state_owner [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

 fs/nfs/nfs4proc.c  |   15 ++++++++++++++-
 fs/nfs/nfs4state.c |   16 +++++++++-------
 2 files changed, 23 insertions(+), 8 deletions(-)
Signature · 2 lines
Regards
Bian Naimeng
author
#1 lore raw

PATCH 1/2 Make lock inode before lock nfs4_state_owner

We should lock inode before lock nfs4_state_owner, sometimes, we want scan
nfsi->open_states, and modify once state, so we need lock state->owner.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/nfs4state.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)
fs/nfs/nfs4state.c +9 -7
index 34acf59..fe34c41 100644
@@ -498,8 +498,8 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
 	if (state)
 		goto out;
 	new = nfs4_alloc_open_state();
-	spin_lock(&owner->so_lock);
 	spin_lock(&inode->i_lock);
+	spin_lock(&owner->so_lock);
 	state = __nfs4_find_state_byowner(inode, owner);
 	if (state == NULL && new != NULL) {
 		state = new;

@@ -507,14 +507,14 @@ nfs4_get_open_state(struct inode *inode, struct nfs4_state_owner *owner)
 		atomic_inc(&owner->so_count);
 		list_add(&state->inode_states, &nfsi->open_states);
 		state->inode = igrab(inode);
-		spin_unlock(&inode->i_lock);
 		/* Note: The reclaim code dictates that we add stateless
 		 * and read-only stateids to the end of the list */
 		list_add_tail(&state->open_states, &owner->so_states);
 		spin_unlock(&owner->so_lock);
-	} else {
 		spin_unlock(&inode->i_lock);
+	} else {
 		spin_unlock(&owner->so_lock);
+		spin_unlock(&inode->i_lock);
 		if (new)
 			nfs4_free_open_state(new);
 	}

@@ -527,13 +527,15 @@ void nfs4_put_open_state(struct nfs4_state *state)
 	struct inode *inode = state->inode;
 	struct nfs4_state_owner *owner = state->owner;
 
-	if (!atomic_dec_and_lock(&state->count, &owner->so_lock))
-		return;
 	spin_lock(&inode->i_lock);
-	list_del(&state->inode_states);
+	if (!atomic_dec_and_lock(&state->count, &owner->so_lock)) {
+		spin_unlock(&inode->i_lock);
+		return;
+	}
 	list_del(&state->open_states);
-	spin_unlock(&inode->i_lock);
 	spin_unlock(&owner->so_lock);
+	list_del(&state->inode_states);
+	spin_unlock(&inode->i_lock);
 	iput(inode);
 	nfs4_free_open_state(state);
 	nfs4_put_state_owner(owner);
Signature · 7 lines
1.6.5.2



-- 
Regards
Bian Naimeng
author
#2 lore raw

PATCH 2/2 We should clear NFS_DELEGATED_STATE after return delegation

In reply to Bian Naimeng

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/nfs4proc.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
fs/nfs/nfs4proc.c +14 -1
index 70015dd..76cdef4 100644
@@ -933,16 +933,29 @@ no_delegation:
 
 static void nfs4_return_incompatible_delegation(struct inode *inode, fmode_t fmode)
 {
+	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
+	struct nfs4_state *state;
 
 	rcu_read_lock();
-	delegation = rcu_dereference(NFS_I(inode)->delegation);
+	delegation = rcu_dereference(nfsi->delegation);
 	if (delegation == NULL || (delegation->type & fmode) == fmode) {
 		rcu_read_unlock();
 		return;
 	}
 	rcu_read_unlock();
 	nfs_inode_return_delegation(inode);
+
+	spin_lock(&inode->i_lock);
+	list_for_each_entry(state, &nfsi->open_states, inode_states) {
+		if (state->owner == NULL)
+			continue;
+
+		spin_lock(&state->owner->so_lock);
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		spin_unlock(&state->owner->so_lock);
+	}
+	spin_unlock(&inode->i_lock);
 }
 
 static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)
Signature · 1 line
1.6.5.2
#3 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 33 lines · Bian Naimeng

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/nfs4proc.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
fs/nfs/nfs4proc.c +7 -1
index 70015dd..76cdef4 100644
@@ -933,16 +933,29 @@ no_delegation:
 
 static void nfs4_return_incompatible_delegation(struct inode *inode, fmode_t fmode)
 {
+	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_delegation *delegation;
+	struct nfs4_state *state;
 
 	rcu_read_lock();
-	delegation = rcu_dereference(NFS_I(inode)->delegation);
+	delegation = rcu_dereference(nfsi->delegation);
 	if (delegation == NULL || (delegation->type & fmode) == fmode) {
 		rcu_read_unlock();
 		return;
 	}
 	rcu_read_unlock();
 	nfs_inode_return_delegation(inode);
+
+	spin_lock(&inode->i_lock);
+	list_for_each_entry(state, &nfsi->open_states, inode_states) {
+		if (state->owner == NULL)

fs/nfs/nfs4proc.c no_delegation: · line 961

Quoted 10 lines
fs/nfs/nfs4proc.c +7 -0
+			continue;
+
+		spin_lock(&state->owner->so_lock);
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		spin_unlock(&state->owner->so_lock);
+	}
+	spin_unlock(&inode->i_lock);
 }
 
 static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)

It is way too late to clear the NFS_DELEGATED_STATE flag _after_ we've returned the delegation. We should be doing it as part of nfs_delegation_claim_opens().

Why isn't the following patch sufficient?

Cheers
Trond

----------------------------------------------------------------------------------
NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens

From: Trond Myklebust <[email protected]>

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/delegation.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfs/delegation.c +2 -2
index 3016345..56d5d1a 100644
@@ -102,10 +102,10 @@ again:
 		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;
+		if (!test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
+			continue;
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);
author
#4 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 13 lines · Bian Naimeng
Quoted 12 lines

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/nfs4proc.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
fs/nfs/nfs4proc.c +0 -0
index 70015dd..76cdef4 100644

... snip ...

Quoted 32 lines
Quoted 2 lines

static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)

It is way too late to clear the NFS_DELEGATED_STATE flag _after_ we've returned the delegation. We should be doing it as part of nfs_delegation_claim_opens().

Why isn't the following patch sufficient?

Cheers
Trond

----------------------------------------------------------------------------------
NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens

From: Trond Myklebust <[email protected]>

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/delegation.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfs/delegation.c +0 -0
index 3016345..56d5d1a 100644
@@ -102,10 +102,10 @@ again:
 		state = ctx->state;
 		if (state == NULL)

fs/nfs/delegation.c again: · line 111

Quoted 10 lines
fs/nfs/delegation.c +2 -2
 			continue;
-		if (!test_bit(NFS_DELEGATED_STATE, &state->flags))
-			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
+		if (!test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
+			continue;
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);

Thanks Trond.
But why we must remove test_and_clear_bit behind memcmp?

Always test_bit and memcmp have same result, and I think test_and_clear_bit is fast than memcmp, so i suggest we should call test_and_clear_bit first. right?

Regards
Bian Naimeng

----------------------------------------------------------------------------------

NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens

Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/nfs/delegation.c +1 -1
index 3016345..cee5755 100644
@@ -102,7 +102,7 @@ again:
 		state = ctx->state;
 		if (state == NULL)
 			continue;
-		if (!test_bit(NFS_DELEGATED_STATE, &state->flags))
+		if (!test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
 			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
Signature · 6 lines
1.6.5.2


-- 
Regards
Bian Naimeng
#5 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 62 lines · Bian Naimeng
Quoted 13 lines · Bian Naimeng
Quoted 12 lines

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/nfs4proc.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
fs/nfs/nfs4proc.c +0 -0
index 70015dd..76cdef4 100644

... snip ...

Quoted 32 lines
Quoted 2 lines

static struct nfs4_state *nfs4_try_open_cached(struct nfs4_opendata *opendata)

It is way too late to clear the NFS_DELEGATED_STATE flag _after_ we've returned the delegation. We should be doing it as part of nfs_delegation_claim_opens().

Why isn't the following patch sufficient?

Cheers
Trond

----------------------------------------------------------------------------------
NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens

From: Trond Myklebust <[email protected]>

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/delegation.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfs/delegation.c +0 -0
index 3016345..56d5d1a 100644
@@ -102,10 +102,10 @@ again:
 		state = ctx->state;
 		if (state == NULL)

fs/nfs/delegation.c again: · line 111

Quoted 10 lines
fs/nfs/delegation.c +2 -2
 			continue;
-		if (!test_bit(NFS_DELEGATED_STATE, &state->flags))
-			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
+		if (!test_and_clear_bit(NFS_DELEGATED_STATE, &state->flags))
+			continue;
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);

Thanks Trond.
But why we must remove test_and_clear_bit behind memcmp?

Quoted 2 lines

Always test_bit and memcmp have same result, and I think test_and_clear_bit is fast than memcmp, so i suggest we should call test_and_clear_bit first. right?

We can't clear the bit before the memcmp() test. What we could do is keep the current test_bit() and then do a clear_bit after the memcmp().

Cheers
Trond

author
#6 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 4 lines · Trond Myklebust
Quoted 3 lines · Bian Naimeng
Quoted 3 lines · Bian Naimeng
Quoted 2 lines

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

    ... snip ...
Quoted 9 lines
Quoted 5 lines

Thanks Trond.
But why we must remove test_and_clear_bit behind memcmp?

Always test_bit and memcmp have same result, and I think test_and_clear_bit is fast than memcmp, so i suggest we should call test_and_clear_bit first. right?

We can't clear the bit before the memcmp() test. What we could do is keep the current test_bit() and then do a clear_bit after the memcmp().

When i apply the patch, my test still fail.

The ctx->state will set set after open success, but this ctx add to the
the nfsi->open_files when we call nfs_open that is so early. So maybe there are some states can be found at nfsi->open_states, but not at ctx->state of nfsi->open_files, so we will miss clear NFS_DELEGATED_STATE for these state.

Regards
Bian Naimeng

--------------------------------------------------

NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
fs/nfs/delegation.c +11 -4
index 3016345..94a63e3 100644
@@ -94,7 +94,7 @@ static int nfs_delegation_claim_opens(struct inode *inode, const nfs4_stateid *s
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_open_context *ctx;
 	struct nfs4_state *state;
-	int err;
+	int err = 0;
 
 again:
 	spin_lock(&inode->i_lock);

@@ -112,12 +112,19 @@ again:
 		if (err >= 0)
 			err = nfs_delegation_claim_locks(ctx, state);
 		put_nfs_open_context(ctx);
-		if (err != 0)
-			return err;
+		if (err != 0) {
+			spin_lock(&inode->i_lock);
+			break;
+		}
 		goto again;
 	}
+
+	list_for_each_entry(state, &nfsi->open_states, inode_states) {
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
+	}
+
 	spin_unlock(&inode->i_lock);
-	return 0;
+	return err;
 }
 
 /*
Signature · 7 lines
1.6.5.2



-- 
Regards
Bian Naimeng
#7 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 65 lines · Bian Naimeng
Quoted 4 lines · Trond Myklebust
Quoted 3 lines · Bian Naimeng
Quoted 3 lines · Bian Naimeng
Quoted 2 lines

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

    ... snip ...
Quoted 9 lines
Quoted 5 lines

Thanks Trond.
But why we must remove test_and_clear_bit behind memcmp?

Always test_bit and memcmp have same result, and I think test_and_clear_bit is fast than memcmp, so i suggest we should call test_and_clear_bit first. right?

We can't clear the bit before the memcmp() test. What we could do is keep the current test_bit() and then do a clear_bit after the memcmp().

When i apply the patch, my test still fail.

The ctx->state will set set after open success, but this ctx add to the
the nfsi->open_files when we call nfs_open that is so early. So maybe there are some states can be found at nfsi->open_states, but not at ctx->state of nfsi->open_files, so we will miss clear NFS_DELEGATED_STATE for these state.

Regards
Bian Naimeng

--------------------------------------------------

NFSv4: Remember to clear NFS_DELEGATED_STATE in nfs_delegation_claim_opens

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
fs/nfs/delegation.c +5 -3
index 3016345..94a63e3 100644
@@ -94,7 +94,7 @@ static int nfs_delegation_claim_opens(struct inode *inode, const nfs4_stateid *s
 	struct nfs_inode *nfsi = NFS_I(inode);
 	struct nfs_open_context *ctx;
 	struct nfs4_state *state;
-	int err;
+	int err = 0;
 
 again:
 	spin_lock(&inode->i_lock);

@@ -112,12 +112,19 @@ again:
 		if (err >= 0)
 			err = nfs_delegation_claim_locks(ctx, state);
 		put_nfs_open_context(ctx);
-		if (err != 0)
-			return err;
+		if (err != 0) {
+			spin_lock(&inode->i_lock);
+			break;
+		}
 		goto again;
 	}

fs/nfs/delegation.c again: · line 129

Quoted 10 lines
fs/nfs/delegation.c +6 -1
+
+	list_for_each_entry(state, &nfsi->open_states, inode_states) {
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
+	}
+
 	spin_unlock(&inode->i_lock);
-	return 0;
+	return err;
 }

I still don't see why this should be necessary. In the case of a server reboot or network partition, the state recovery thread ought to be taking care of this for us.

Cheers
Trond

author
#8 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

... snip ...

Quoted 6 lines
Quoted 6 lines
fs/nfs/delegation.c +5 -0
+
+	list_for_each_entry(state, &nfsi->open_states, inode_states) {
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
+	}
+
 	spin_unlock(&inode->i_lock);
Quoted 9 lines

fs/nfs/delegation.c again: · line 128

Quoted 4 lines
fs/nfs/delegation.c +1 -1
-	return 0;
+	return err;
 }

I still don't see why this should be necessary. In the case of a server reboot or network partition, the state recovery thread ought to be taking care of this for us.

A open state can be found at nfsi->open_states and owner->so_states always, but
it not be found at nfsi->open_files until we call nfs4_intent_set_file.

At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at nfsi->open_files, but it still at nfsi->open_states, so we do not clear NFS_DELEGATED_STATE bit for it.

Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
as a delegation, some error will occur.

Signature · 2 lines
Regards
Bian Naimeng
Quoted 2 lines

Cheers
Trond

#9 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 23 lines · Bian Naimeng
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

... snip ...

Quoted 6 lines
Quoted 6 lines
fs/nfs/delegation.c +5 -0
+
+	list_for_each_entry(state, &nfsi->open_states, inode_states) {
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
+	}
+
 	spin_unlock(&inode->i_lock);
Quoted 9 lines

fs/nfs/delegation.c again: · line 128

Quoted 4 lines
fs/nfs/delegation.c +1 -1
-	return 0;
+	return err;
 }

I still don't see why this should be necessary. In the case of a server reboot or network partition, the state recovery thread ought to be taking care of this for us.

A open state can be found at nfsi->open_states and owner->so_states always, but
it not be found at nfsi->open_files until we call nfs4_intent_set_file.

Quoted 8 lines

At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at nfsi->open_files, but it still at nfsi->open_states, so we do not clear NFS_DELEGATED_STATE bit for it.

Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
as a delegation, some error will occur.

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Cheers
Trond

author
#10 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 12 lines
Quoted 12 lines
Quoted 4 lines

I still don't see why this should be necessary. In the case of a server reboot or network partition, the state recovery thread ought to be taking care of this for us.

A open state can be found at nfsi->open_states and owner->so_states always, but
it not be found at nfsi->open_files until we call nfs4_intent_set_file.

At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at nfsi->open_files, but it still at nfsi->open_states, so we do not clear NFS_DELEGATED_STATE bit for it.

Quoted 8 lines
Quoted 3 lines

Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
as a delegation, some error will occur.

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Hi trond, i am not sure my opinion is right.

If the state not at nfsi->open_states, i think maybe that means it's not a open stateid, it just a old state we want reuse it. After we update it, it become a open stateid.

Signature · 2 lines
Regards
Bian Naimeng
author
#11 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 20 lines
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines

I still don't see why this should be necessary. In the case of a server reboot or network partition, the state recovery thread ought to be taking care of this for us.

A open state can be found at nfsi->open_states and owner->so_states always, but
it not be found at nfsi->open_files until we call nfs4_intent_set_file.

At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at nfsi->open_files, but it still at nfsi->open_states, so we do not clear NFS_DELEGATED_STATE bit for it.

Quoted 7 lines
Quoted 3 lines

Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
as a delegation, some error will occur.

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Quoted 5 lines

Hi trond, i am not sure my opinion is right.

If the state not at nfsi->open_states, i think maybe that means it's not a open stateid, it just a old state we want reuse it. After we update it, it become a open stateid.

Hi trond, have you got a idea to solve this race?

Signature · 2 lines
Regards
Bian Naimeng
author
#12 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

In reply to Trond Myklebust
Quoted 2 lines · Bian Naimeng
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

... snip ...

Quoted 8 lines
Quoted 8 lines

A open state can be found at nfsi->open_states and owner->so_states always, but
it not be found at nfsi->open_files until we call nfs4_intent_set_file.

At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at nfsi->open_files, but it still at nfsi->open_states, so we do not clear NFS_DELEGATED_STATE bit for it.

Quoted 8 lines
Quoted 3 lines

Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
as a delegation, some error will occur.

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Hi trond, what do you think about this one?

Best Regards
Bian

-------------------------------------------------------------------------
If there are not any delegation at this inode, we should clear stateid's NFS_DELEGATED_STATE when update it.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |    1 +
 fs/nfs/nfs4proc.c   |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)
fs/nfs/delegation.c +1 -0
index 83bb16f..3a7a19d 100644
@@ -109,6 +109,7 @@ again:
 			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);
fs/nfs/nfs4proc.c +11 -1
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
 
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||
Signature · 1 line
1.6.5.2
#13 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 17 lines · Bian Naimeng
Quoted 2 lines · Bian Naimeng
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

... snip ...

Quoted 12 lines
Quoted 11 lines

A open state can be found at nfsi->open_states and owner->so_states always, but
it not be found at nfsi->open_files until we call nfs4_intent_set_file.

At _nfs4_do_open, we will invoke nfs4_return_incompatible_delegation to return a
delegation, a open stateid which set NFS_DELEGATED_STATE bit may not be found at nfsi->open_files, but it still at nfsi->open_states, so we do not clear NFS_DELEGATED_STATE bit for it.

Then _nfs4_do_open will find it by nfs4_get_open_state, and we still use it
as a delegation, some error will occur.

Quoted 6 lines
Quoted 4 lines

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Hi trond, what do you think about this one?

Hi Bian,

See comments/questions below.

Quoted 31 lines

Best Regards
Bian

-------------------------------------------------------------------------
If there are not any delegation at this inode, we should clear stateid's NFS_DELEGATED_STATE when update it.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |    1 +
 fs/nfs/nfs4proc.c   |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)
fs/nfs/delegation.c +1 -0
index 83bb16f..3a7a19d 100644
@@ -109,6 +109,7 @@ again:
 			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);
fs/nfs/nfs4proc.c +0 -0
index 36400d3..c0c0320 100644

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 905

Quoted 7 lines
fs/nfs/nfs4proc.c +3 -1
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
 
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {

Any reason why we can't ditch the 'test_bit'?

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 909

Quoted 4 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 917

Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||

Cheers
Trond

author
#14 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 10 lines
Quoted 5 lines
Quoted 4 lines

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Hi trond, what do you think about this one?

Hi Bian,

See comments/questions below.

Thanks for your answer.

Quoted 6 lines
Quoted 6 lines
fs/nfs/nfs4proc.c +0 -0
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 9 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 905

Quoted 6 lines
fs/nfs/nfs4proc.c +3 -1
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {

Any reason why we can't ditch the 'test_bit'?

Because i am not sure it's ok that we just clear_bit at here. If you think it's ok, i'd be fine with removing this test_bit.

Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 909

Quoted 4 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

Yes, i will add a checking. Thanks very much.

Quoted 8 lines
Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||
Signature · 2 lines
Regards
Bian Naimeng
#15 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 29 lines · Bian Naimeng
Quoted 10 lines
Quoted 5 lines
Quoted 4 lines

OK. I see agree that is a race, but AFAICS, your fix means that we end up with an nfs_state structure that has cleared NFS_DELEGATED_STATE, but that did not recover its open stateid.

Hi trond, what do you think about this one?

Hi Bian,

See comments/questions below.

Thanks for your answer.

Quoted 6 lines
Quoted 6 lines
fs/nfs/nfs4proc.c +0 -0
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 9 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 905

Quoted 6 lines
fs/nfs/nfs4proc.c +3 -1
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {

Any reason why we can't ditch the 'test_bit'?

Quoted 2 lines

Because i am not sure it's ok that we just clear_bit at here. If you think it's ok, i'd be fine with removing this test_bit.

How is it different from just clearing the bit?

Quoted 11 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 909

Quoted 4 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

Yes, i will add a checking. Thanks very much.

Quoted 8 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 917

Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||

Cheers
Trond

author
#16 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 905

Quoted 12 lines
fs/nfs/nfs4proc.c +3 -1
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
 
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {

Any reason why we can't ditch the 'test_bit'?

Quoted 4 lines
Quoted 2 lines

Because i am not sure it's ok that we just clear_bit at here. If you think it's ok, i'd be fine with removing this test_bit.

How is it different from just clearing the bit?

Yes, I know there are no difference.

I mean maybe we need do more things if state is delegation, not just clearing the bit, this the reason why i add the FIXME.

OK, i will send a new one after remove the test_bit.

Quoted 22 lines
Quoted 10 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 909

Quoted 5 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

Yes, i will add a checking. Thanks very much.

Quoted 8 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 917

Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||

Cheers
Trond

Signature · 2 lines
Regards
Bian Naimeng
author
#17 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

In reply to Trond Myklebust

If there are not any delegation at this inode, we should clear stateid's NFS_DELEGATED_STATE when update it.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |    1 +
 fs/nfs/nfs4proc.c   |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)
fs/nfs/delegation.c +1 -0
index b9c3c43..62f296e 100644
@@ -106,6 +106,7 @@ again:
 			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);
fs/nfs/nfs4proc.c +11 -1
index 089da5b..f7e45b4 100644
@@ -919,8 +919,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
 
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL && open_stateid != NULL) {
+			/*
+			 * FIXME: If the state has NFS_DELEGATED_STATE bit
+			 * we catch a race. Maybe should recover its open
+			 * stateid, now we just clear the NFS_DELEGATED_STATE
+			 * bit.
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||
Signature · 1 line
1.7.0
Quoted 42 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
fs/nfs/nfs4proc.c +0 -0
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 905

Quoted 6 lines
fs/nfs/nfs4proc.c +3 -1
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {

Any reason why we can't ditch the 'test_bit'?

Quoted 2 lines

Because i am not sure it's ok that we just clear_bit at here. If you think it's ok, i'd be fine with removing this test_bit.

How is it different from just clearing the bit?

Quoted 9 lines
Quoted 7 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 909

Quoted 4 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

Yes, i will add a checking. Thanks very much.

Quoted 8 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 917

Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||

Cheers
Trond

Signature · 2 lines
Regards
Bian Naimeng
#18 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 27 lines · Bian Naimeng

If there are not any delegation at this inode, we should clear stateid's NFS_DELEGATED_STATE when update it.

Signed-off-by Bian Naimeng <[email protected]>
---
 fs/nfs/delegation.c |    1 +
 fs/nfs/nfs4proc.c   |   12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletions(-)
fs/nfs/delegation.c +1 -0
index b9c3c43..62f296e 100644
@@ -106,6 +106,7 @@ again:
 			continue;
 		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
 			continue;
+		clear_bit(NFS_DELEGATED_STATE, &state->flags);
 		get_nfs_open_context(ctx);
 		spin_unlock(&inode->i_lock);
 		err = nfs4_open_delegation_recall(ctx, state, stateid);
fs/nfs/nfs4proc.c +0 -0
index 089da5b..f7e45b4 100644

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 923

Quoted 6 lines
fs/nfs/nfs4proc.c +2 -1
@@ -919,8 +919,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
 
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL && open_stateid != NULL) {

Well... What I really meant was that we should make sure that we don't get into this situation.

I think the clear_bit() should be unconditional if delegation == NULL, but if the (delegation == NULL && open_stateid == NULL) _can_ occur, then we should probably mark the nfs4_state for recovery using nfs4_state_mark_reclaim_nograce(), and then fire of a recovery thread.

Quoted 60 lines
fs/nfs/nfs4proc.c +9 -0
+			/*
+			 * FIXME: If the state has NFS_DELEGATED_STATE bit
+			 * we catch a race. Maybe should recover its open
+			 * stateid, now we just clear the NFS_DELEGATED_STATE
+			 * bit.
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||
Signature · 1 line
1.7.0
Quoted 42 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
fs/nfs/nfs4proc.c +0 -0
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat

Quoted 6 lines
fs/nfs/nfs4proc.c +3 -1
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL &&
+		    test_bit(NFS_DELEGATED_STATE, &state->flags)) {

Any reason why we can't ditch the 'test_bit'?

Quoted 2 lines

Because i am not sure it's ok that we just clear_bit at here. If you think it's ok, i'd be fine with removing this test_bit.

How is it different from just clearing the bit?

Quoted 9 lines
Quoted 7 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 928

Quoted 4 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit, 
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

Yes, i will add a checking. Thanks very much.

Quoted 8 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 936

Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||

Cheers
Trond

author
#19 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 5 lines
Quoted 5 lines

index 089da5b..f7e45b4 100644

fs/nfs/nfs4proc.c +0 -0
@@ -919,8 +919,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 9 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 923

Quoted 5 lines
fs/nfs/nfs4proc.c +2 -1
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL && open_stateid != NULL) {

Well... What I really meant was that we should make sure that we don't get into this situation.

Thanks very much for your explainning.

Quoted 1 line

I think the clear_bit() should be unconditional if delegation == NULL,

en..., i have a question.

If the (deleg_cur == NULL && delegation == NULL) occured, that means there are not any delegation at this nfs_inode, i think this state do not need a NFS_DELEGATED_STATE bit anymore, is it right?

Quoted 4 lines

but if the (delegation == NULL && open_stateid == NULL) _can_ occur, then we should probably mark the nfs4_state for recovery using nfs4_state_mark_reclaim_nograce(), and then fire of a recovery thread.

It looks like that (delegation == NULL && open_stateid == NULL) can not occur at our kernel.

And..., would you tell me why we must start recovery with using nfs4_state_mark_reclaim_nograce, are there any hint tell us that this state has expired ?

Signature · 2 lines
Regards
Bian Naimeng
Quoted 15 lines
Quoted 15 lines
fs/nfs/nfs4proc.c +9 -0
+			/*
+			 * FIXME: If the state has NFS_DELEGATED_STATE bit
+			 * we catch a race. Maybe should recover its open
+			 * stateid, now we just clear the NFS_DELEGATED_STATE
+			 * bit.
+			 */
+			clear_bit(NFS_DELEGATED_STATE, &state->flags);
+		}
 		goto no_delegation;
+	}
 
 	spin_lock(&deleg_cur->lock);
 	if (nfsi->delegation != deleg_cur ||
Signature · 1 line
1.7.0
Signature · 2 lines
Regards
Bian Naimeng
author
#20 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

Quoted 29 lines · Bian Naimeng
Quoted 5 lines
Quoted 5 lines

index 089da5b..f7e45b4 100644

fs/nfs/nfs4proc.c +0 -0
@@ -919,8 +919,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 923

Quoted 5 lines
fs/nfs/nfs4proc.c +2 -1
 	rcu_read_lock();
 	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL && open_stateid != NULL) {

Well... What I really meant was that we should make sure that we don't get into this situation.

Thanks very much for your explainning.

Quoted 1 line

I think the clear_bit() should be unconditional if delegation == NULL,

en..., i have a question.

If the (deleg_cur == NULL && delegation == NULL) occured, that means there are not any delegation at this nfs_inode, i think this state do not need a NFS_DELEGATED_STATE bit anymore, is it right?

Quoted 4 lines

but if the (delegation == NULL && open_stateid == NULL) _can_ occur, then we should probably mark the nfs4_state for recovery using nfs4_state_mark_reclaim_nograce(), and then fire of a recovery thread.

Quoted 6 lines

It looks like that (delegation == NULL && open_stateid == NULL) can not occur at our kernel.

And..., would you tell me why we must start recovery with using nfs4_state_mark_reclaim_nograce, are there any hint tell us that this state has expired ?

Hi Trond,

Had this bug been fixed ?

Thanks
Bian

A message that is not in the archive (sent off-list or not yet received): 1283978245.2905.23.camel-rJ7iovZKK19ZJLDQqaL3InhyD016LWXt@public.gmane.org

#22 lore raw

Re: [PATCH 2/2] We should clear NFS_DELEGATED_STATE after return delegation

hi Trond

Do you have plan to fix this bug.
Because our custom want to solve it.

If you have any comment or advice please let us know.

thank you.

Quoted 112 lines · Trond Myklebust
Quoted 27 lines · Bian Naimeng

If there are not any delegation at this inode, we should clear stateid's NFS_DELEGATED_STATE when update it.

Signed-off-by Bian Naimeng<[email protected]>
---
  fs/nfs/delegation.c |    1 +
  fs/nfs/nfs4proc.c   |   12 +++++++++++-
  2 files changed, 12 insertions(+), 1 deletions(-)
fs/nfs/delegation.c +1 -0
index b9c3c43..62f296e 100644
@@ -106,6 +106,7 @@ again:
  			continue;
  		if (memcmp(state->stateid.data, stateid->data, sizeof(state->stateid.data)) != 0)
  			continue;
+		clear_bit(NFS_DELEGATED_STATE,&state->flags);
  		get_nfs_open_context(ctx);
  		spin_unlock(&inode->i_lock);
  		err = nfs4_open_delegation_recall(ctx, state, stateid);
fs/nfs/nfs4proc.c +0 -0
index 089da5b..f7e45b4 100644

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 923

Quoted 6 lines
fs/nfs/nfs4proc.c +2 -1
@@ -919,8 +919,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
 
  	rcu_read_lock();
  	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL&&  open_stateid != NULL) {

Well... What I really meant was that we should make sure that we don't get into this situation.

I think the clear_bit() should be unconditional if delegation == NULL, but if the (delegation == NULL&& open_stateid == NULL) _can_ occur, then we should probably mark the nfs4_state for recovery using nfs4_state_mark_reclaim_nograce(), and then fire of a recovery thread.

Quoted 14 lines
fs/nfs/nfs4proc.c +9 -1
+			/*
+			 * FIXME: If the state has NFS_DELEGATED_STATE bit
+			 * we catch a race. Maybe should recover its open
+			 * stateid, now we just clear the NFS_DELEGATED_STATE
+			 * bit.
+			 */
+			clear_bit(NFS_DELEGATED_STATE,&state->flags);
+		}
  		goto no_delegation;
+	}
 
  	spin_lock(&deleg_cur->lock);
  	if (nfsi->delegation != deleg_cur ||
--
Quoted 46 lines

1.7.0

Quoted 42 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
fs/nfs/nfs4proc.c +0 -0
index 36400d3..c0c0320 100644
@@ -900,8 +900,18 @@ static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat

Quoted 6 lines
fs/nfs/nfs4proc.c +3 -1
  	rcu_read_lock();
  	deleg_cur = rcu_dereference(nfsi->delegation);
-	if (deleg_cur == NULL)
+	if (deleg_cur == NULL) {
+		if (delegation == NULL&&
+		    test_bit(NFS_DELEGATED_STATE,&state->flags)) {

Any reason why we can't ditch the 'test_bit'?

Quoted 2 lines

Because i am not sure it's ok that we just clear_bit at here. If you think it's ok, i'd be fine with removing this test_bit.

How is it different from just clearing the bit?

Quoted 9 lines
Quoted 7 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 928

Quoted 4 lines
fs/nfs/nfs4proc.c +4 -0
+			/*FIXME: If the state has NFS_DELEGATED_STATE bit,
+			 * we catch a race. Maybe should recover its open
+			 * stateid, here we just clear the NFS_DELEGATED_STATE
+			 * bit.

Can this ever be called with both deleg_cur==NULL and open_stateid==NULL? If so, then we still have a bug.

Yes, i will add a checking. Thanks very much.

Quoted 8 lines
Quoted 8 lines

fs/nfs/nfs4proc.c static int update_open_stateid(struct nfs4_state *state, nfs4_stateid *open_stat · line 936

Quoted 8 lines
fs/nfs/nfs4proc.c +4 -0
+			 */
+			clear_bit(NFS_DELEGATED_STATE,&state->flags);
+		}
  		goto no_delegation;
+	}
 
  	spin_lock(&deleg_cur->lock);
  	if (nfsi->delegation != deleg_cur ||

Cheers
Trond

--
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

Signature · 12 lines
Regards
Li Yewang
--------------------------------------------------
Li Yewang
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
No. 6 Wenzhu Road, Nanjing, 210012, China
PHONE: +86+25-86630566-8507
COINS: 7998-8507
FAX: +86+25-83317685
MAIL: [email protected]
--------------------------------------------------