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

[Bian Naimeng <biannm@cn.fujitsu.com>]

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