Re: [PATCH] NFSv4.1: Keep a reference on lock states while checking

["Benjamin Coddington" <bcodding@redhat.com>]

On 18 Nov 2016, at 16:55, Anna Schumaker wrote:

> Hi Ben,
>
> On 11/18/2016 05:03 AM, Benjamin Coddington wrote:
>> While walking the list of lock_states, keep a reference on each
>> nfs4_lock_state to be checked, otherwise the lock state could be 
>> removed
>> while the check performs TEST_STATEID and possible FREE_STATEID.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>> ---
>>  fs/nfs/nfs4proc.c | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 7897826d7c51..57d102d0f075 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2564,15 +2564,23 @@ static void 
>> nfs41_check_delegation_stateid(struct nfs4_state *state)
>>  static int nfs41_check_expired_locks(struct nfs4_state *state)
>>  {
>>  	int status, ret = NFS_OK;
>> -	struct nfs4_lock_state *lsp;
>> +	struct nfs4_lock_state *lsp, *prev = NULL;
>>  	struct nfs_server *server = NFS_SERVER(state->inode);
>>
>>  	if (!test_bit(LK_STATE_IN_USE, &state->flags))
>>  		goto out;
>> +
>> +	spin_lock(&state->state_lock);
>>  	list_for_each_entry(lsp, &state->lock_states, ls_locks) {
>>  		if (test_bit(NFS_LOCK_INITIALIZED, &lsp->ls_flags)) {
>>  			struct rpc_cred *cred = lsp->ls_state->owner->so_cred;
>>
>> +			atomic_inc(&lsp->ls_count);
>> +			spin_unlock(&state->state_lock);
>> +
>> +			nfs4_put_lock_state(prev);
>> +			prev = lsp;
>> +
>>  			status = nfs41_test_and_free_expired_stateid(server,
>>  					&lsp->ls_stateid,
>>  					cred);
>> @@ -2587,8 +2595,11 @@ static int nfs41_check_expired_locks(struct 
>> nfs4_state *state)
>>  				ret = status;
>>  				break;
>
> Should this "break" be replaced with a "goto out" since we're no 
> longer holding the lock at this point?

Oh, yes otherwise we'd unlock twice.  Thanks for catching that!

If it's changed from break to goto out then the last 
nfs4_put_lock_state()
should be moved below the out: label as well in order to match the
atomic_inc(), and that makes the nfs4_put_lock_state() an additional
unnecessary call in the case that LK_STATE_IN_USE wasn't set.

So, maybe change the break to goto out, and add 
nfs4_put_lock_state(prev)
just before it.  I'll send that as a v2.

Ben

>>  			}
>> +			spin_lock(&state->state_lock);
>>  		}
>> -	};
>> +	}
>> +	spin_unlock(&state->state_lock);
>> +	nfs4_put_lock_state(prev);
>>  out:
>>  	return ret;
>>  }
>>
>