[PATCH 0/2] Fix bug that client use a invaild delegation

[PATCH 0/2] Fix bug that client use a invaild delegation

[Bian Naimeng]

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

-- 
Regards
Bian Naimeng

[PATCH 1/2] Make lock inode before lock nfs4_state_owner

[Bian Naimeng]

 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 <biannm@cn.fujitsu.com>

---
 fs/nfs/nfs4state.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 34acf59..fe34c41 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -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);
-- 
1.6.5.2



-- 
Regards
Bian Naimeng

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

[Bian Naimeng]

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

Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com>

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

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 70015dd..76cdef4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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)
-- 
1.6.5.2

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

[Trond Myklebust]

On Wed, 2010-08-04 at 17:18 +0800, Bian Naimeng wrote:
> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
> 
> Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com>
> 
> ---
>  fs/nfs/nfs4proc.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 70015dd..76cdef4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -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)

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 <Trond.Myklebust@netapp.com>

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/delegation.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 3016345..56d5d1a 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -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);

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

[Bian Naimeng]

> On Wed, 2010-08-04 at 17:18 +0800, Bian Naimeng wrote:
>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
>>
>> Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com>
>>
>> ---
>>  fs/nfs/nfs4proc.c |   15 ++++++++++++++-
>>  1 files changed, 14 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 70015dd..76cdef4 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c

  ... snip ...

>>  
>>  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 <Trond.Myklebust@netapp.com>
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/delegation.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 3016345..56d5d1a 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -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);
> 

 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 <Trond.Myklebust@netapp.com>
Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com>

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

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 3016345..cee5755 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -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;
-- 
1.6.5.2


-- 
Regards
Bian Naimeng

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

[Trond Myklebust]

On Thu, 2010-08-05 at 10:26 +0800, Bian Naimeng wrote:
> > On Wed, 2010-08-04 at 17:18 +0800, Bian Naimeng wrote:
> >> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
> >>
> >> Signed-off-by: Bian Naimeng <biannm@cn.fujitsu.com>
> >>
> >> ---
> >>  fs/nfs/nfs4proc.c |   15 ++++++++++++++-
> >>  1 files changed, 14 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index 70015dd..76cdef4 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> 
>   ... snip ...
> 
> >>  
> >>  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 <Trond.Myklebust@netapp.com>
> > 
> > Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> > ---
> > 
> >  fs/nfs/delegation.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > index 3016345..56d5d1a 100644
> > --- a/fs/nfs/delegation.c
> > +++ b/fs/nfs/delegation.c
> > @@ -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);
> > 
> 
>  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().

Cheers
  Trond

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

[Bian Naimeng]

Trond Myklebust 写道:
> On Thu, 2010-08-05 at 10:26 +0800, Bian Naimeng wrote:
>>> On Wed, 2010-08-04 at 17:18 +0800, Bian Naimeng wrote:
>>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
>>>>

    ... snip ...

>>  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 <biannm@cn.fujitsu.com>

---
 fs/nfs/delegation.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 3016345..94a63e3 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -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;
 }
 
 /*
-- 
1.6.5.2



-- 
Regards
Bian Naimeng

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

[Trond Myklebust]

On Fri, 2010-08-06 at 12:10 +0800, Bian Naimeng wrote:
> 
> Trond Myklebust 写道:
> > On Thu, 2010-08-05 at 10:26 +0800, Bian Naimeng wrote:
> >>> On Wed, 2010-08-04 at 17:18 +0800, Bian Naimeng wrote:
> >>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
> >>>>
> 
>     ... snip ...
> 
> >>  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 <biannm@cn.fujitsu.com>
> 
> ---
>  fs/nfs/delegation.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 3016345..94a63e3 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -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;
>  }
>  

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

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

[Bian Naimeng]

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

 ... snip ...

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

   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.

-- 
Regards
Bian Naimeng

> Cheers
>   Trond

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

[Trond Myklebust]

On Mon, 2010-08-16 at 15:50 +0800, Bian Naimeng wrote:
> >>>>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
> 
>  ... snip ...
> 
> >> +
> >> +	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.
> > 
> 
>    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.
> 

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

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

[Bian Naimeng]

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

-- 
Regards
Bian Naimeng

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

[Bian Naimeng]

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

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

-- 
Regards
Bian Naimeng

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

[Bian Naimeng]

> On Mon, 2010-08-16 at 15:50 +0800, Bian Naimeng wrote:
>>>>>>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.

... snip ...

>>    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.
>>
> 
> 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 <biannm@cn.fujitsu.com>

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

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 83bb16f..3a7a19d 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -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);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 36400d3..c0c0320 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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 ||
-- 
1.6.5.2

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

[Trond Myklebust]

On Wed, 2010-09-01 at 14:40 +0800, Bian Naimeng wrote:
> > On Mon, 2010-08-16 at 15:50 +0800, Bian Naimeng wrote:
> >>>>>>>> We should clear NFS_DELEGATED_STATE bit for inode->open_states after return delegation.
> 
> ... snip ...
> 
> >>    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.
> >>
> > 
> > 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.

> 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 <biannm@cn.fujitsu.com>
> 
> ---
>  fs/nfs/delegation.c |    1 +
>  fs/nfs/nfs4proc.c   |   12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 83bb16f..3a7a19d 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -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);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 36400d3..c0c0320 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -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'?

> +			/*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.

> +			 */
> +			clear_bit(NFS_DELEGATED_STATE, &state->flags);
> +		}
>  		goto no_delegation;
> +	}
>  
>  	spin_lock(&deleg_cur->lock);
>  	if (nfsi->delegation != deleg_cur ||

Cheers
  Trond

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

[Bian Naimeng]

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

>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 36400d3..c0c0320 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -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'?
> 

  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.

>> +			/*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.

>> +			 */
>> +			clear_bit(NFS_DELEGATED_STATE, &state->flags);
>> +		}
>>  		goto no_delegation;
>> +	}
>>  
>>  	spin_lock(&deleg_cur->lock);
>>  	if (nfsi->delegation != deleg_cur ||


-- 
Regards
Bian Naimeng

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

[Trond Myklebust]

On Wed, 2010-09-08 at 09:33 +0800, Bian Naimeng wrote:
> >>> 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.
> 
> >> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >> index 36400d3..c0c0320 100644
> >> --- a/fs/nfs/nfs4proc.c
> >> +++ b/fs/nfs/nfs4proc.c
> >> @@ -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'?
> > 
> 
>   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?

> >> +			/*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.
> 
> >> +			 */
> >> +			clear_bit(NFS_DELEGATED_STATE, &state->flags);
> >> +		}
> >>  		goto no_delegation;
> >> +	}
> >>  
> >>  	spin_lock(&deleg_cur->lock);
> >>  	if (nfsi->delegation != deleg_cur ||

Cheers
  Trond

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

[Bian Naimeng]

>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 36400d3..c0c0320 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -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'?
>>>
>>   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.

> 
>>>> +			/*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.
>>
>>>> +			 */
>>>> +			clear_bit(NFS_DELEGATED_STATE, &state->flags);
>>>> +		}
>>>>  		goto no_delegation;
>>>> +	}
>>>>  
>>>>  	spin_lock(&deleg_cur->lock);
>>>>  	if (nfsi->delegation != deleg_cur ||
> 
> Cheers
>   Trond
> 
 

-- 
Regards
Bian Naimeng

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

[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 <biannm@cn.fujitsu.com>

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

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index b9c3c43..62f296e 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -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);
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 089da5b..f7e45b4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -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 ||
-- 
1.7.0


>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>> index 36400d3..c0c0320 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -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'?
>>>
>>   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?
> 
>>>> +			/*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.
>>
>>>> +			 */
>>>> +			clear_bit(NFS_DELEGATED_STATE, &state->flags);
>>>> +		}
>>>>  		goto no_delegation;
>>>> +	}
>>>>  
>>>>  	spin_lock(&deleg_cur->lock);
>>>>  	if (nfsi->delegation != deleg_cur ||
> 
> Cheers
>   Trond
> 
> 
> 

-- 
Regards
Bian Naimeng

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

[Trond Myklebust]

On Wed, 2010-09-08 at 11:11 +0800, Bian Naimeng wrote:
> 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 <biannm@cn.fujitsu.com>
> 
> ---
>  fs/nfs/delegation.c |    1 +
>  fs/nfs/nfs4proc.c   |   12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index b9c3c43..62f296e 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -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);
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 089da5b..f7e45b4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -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.

> +			/*
> +			 * 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 ||
> -- 
> 1.7.0
> 
> 
> >>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >>>> index 36400d3..c0c0320 100644
> >>>> --- a/fs/nfs/nfs4proc.c
> >>>> +++ b/fs/nfs/nfs4proc.c
> >>>> @@ -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'?
> >>>
> >>   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?
> > 
> >>>> +			/*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.
> >>
> >>>> +			 */
> >>>> +			clear_bit(NFS_DELEGATED_STATE, &state->flags);
> >>>> +		}
> >>>>  		goto no_delegation;
> >>>> +	}
> >>>>  
> >>>>  	spin_lock(&deleg_cur->lock);
> >>>>  	if (nfsi->delegation != deleg_cur ||
> > 
> > Cheers
> >   Trond
> > 
> > 
> > 
> 

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

[Bian Naimeng]

>> index 089da5b..f7e45b4 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -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.
> 

  Thanks very much for your explainning.

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

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

-- 
Regards
Bian Naimeng

>> +			/*
>> +			 * 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 ||
>> -- 
>> 1.7.0

-- 
Regards
Bian Naimeng

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

[Bian Naimeng]

Bian Naimeng wrote:
>>> index 089da5b..f7e45b4 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -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.
>>
> 
>   Thanks very much for your explainning.
> 
>> 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?
> 
>> 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 ?

  Hi Trond,

     Had this bug been fixed ?

  Thanks
    Bian

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

[Li Yewang]

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.


At 2010-9-9 4:37, Trond Myklebust wrote:
> On Wed, 2010-09-08 at 11:11 +0800, Bian Naimeng wrote:
>> 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<biannm@cn.fujitsu.com>
>>
>> ---
>>   fs/nfs/delegation.c |    1 +
>>   fs/nfs/nfs4proc.c   |   12 +++++++++++-
>>   2 files changed, 12 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
>> index b9c3c43..62f296e 100644
>> --- a/fs/nfs/delegation.c
>> +++ b/fs/nfs/delegation.c
>> @@ -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);
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 089da5b..f7e45b4 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -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.
>
>> +			/*
>> +			 * 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 ||
>> --
>> 1.7.0
>>
>>
>>>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>>>>> index 36400d3..c0c0320 100644
>>>>>> --- a/fs/nfs/nfs4proc.c
>>>>>> +++ b/fs/nfs/nfs4proc.c
>>>>>> @@ -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'?
>>>>>
>>>>    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?
>>>
>>>>>> +			/*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.
>>>>
>>>>>> +			 */
>>>>>> +			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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

-- 
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: lyw@cn.fujitsu.com
--------------------------------------------------