NFSv4.1: Keep a reference on lock states while checking
diff mbox

Message ID 6a50c9ed9077e43de5da454a2413769a0b383d58.1479427989.git.bcodding@redhat.com
State New
Headers show

Commit Message

Benjamin Coddington Nov. 18, 2016, 10:03 a.m. UTC
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(-)

Comments

Anna Schumaker Nov. 18, 2016, 9:55 p.m. UTC | #1
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?

Thanks,
Anna

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

--
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
Benjamin Coddington Nov. 19, 2016, 2:03 a.m. UTC | #2
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;
>>  }
>>
>
--
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

Patch
diff mbox

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;
 			}
+			spin_lock(&state->state_lock);
 		}
-	};
+	}
+	spin_unlock(&state->state_lock);
+	nfs4_put_lock_state(prev);
 out:
 	return ret;
 }