2016-11-18 10:03:24

by Benjamin Coddington

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

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

2016-11-18 21:55:46

by Anna Schumaker

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

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 <[email protected]>
> ---
> 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;
> }
>

2016-11-19 02:03:34

by Benjamin Coddington

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

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 <[email protected]>
>> ---
>> 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;
>> }
>>
>

2016-11-19 02:11:41

by Benjamin Coddington

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

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 <[email protected]>
---
fs/nfs/nfs4proc.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 7897826d7c51..c2b87dd6b20f 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);
@@ -2585,10 +2593,14 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
set_bit(NFS_LOCK_LOST, &lsp->ls_flags);
} else if (status != NFS_OK) {
ret = status;
- break;
+ nfs4_put_lock_state(prev);
+ goto out;
}
+ spin_lock(&state->state_lock);
}
- };
+ }
+ spin_unlock(&state->state_lock);
+ nfs4_put_lock_state(prev);
out:
return ret;
}
--
2.5.5