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

[Trond Myklebust <Trond.Myklebust@netapp.com>]

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