* [PATCH] NFSv4.1: Keep a reference on lock states while checking
@ 2016-11-18 10:03 Benjamin Coddington
2016-11-18 21:55 ` Anna Schumaker
2016-11-19 2:11 ` [PATCH v2] " Benjamin Coddington
0 siblings, 2 replies; 4+ messages in thread
From: Benjamin Coddington @ 2016-11-18 10:03 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs
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;
}
+ spin_lock(&state->state_lock);
}
- };
+ }
+ spin_unlock(&state->state_lock);
+ nfs4_put_lock_state(prev);
out:
return ret;
}
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] NFSv4.1: Keep a reference on lock states while checking
2016-11-18 10:03 [PATCH] NFSv4.1: Keep a reference on lock states while checking Benjamin Coddington
@ 2016-11-18 21:55 ` Anna Schumaker
2016-11-19 2:03 ` Benjamin Coddington
2016-11-19 2:11 ` [PATCH v2] " Benjamin Coddington
1 sibling, 1 reply; 4+ messages in thread
From: Anna Schumaker @ 2016-11-18 21:55 UTC (permalink / raw)
To: Benjamin Coddington, Trond Myklebust; +Cc: linux-nfs
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;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] NFSv4.1: Keep a reference on lock states while checking
2016-11-18 21:55 ` Anna Schumaker
@ 2016-11-19 2:03 ` Benjamin Coddington
0 siblings, 0 replies; 4+ messages in thread
From: Benjamin Coddington @ 2016-11-19 2:03 UTC (permalink / raw)
To: Anna Schumaker; +Cc: Trond Myklebust, linux-nfs
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;
>> }
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] NFSv4.1: Keep a reference on lock states while checking
2016-11-18 10:03 [PATCH] NFSv4.1: Keep a reference on lock states while checking Benjamin Coddington
2016-11-18 21:55 ` Anna Schumaker
@ 2016-11-19 2:11 ` Benjamin Coddington
1 sibling, 0 replies; 4+ messages in thread
From: Benjamin Coddington @ 2016-11-19 2:11 UTC (permalink / raw)
To: Trond Myklebust, Anna Schumaker; +Cc: linux-nfs
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 | 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-19 2:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-18 10:03 [PATCH] NFSv4.1: Keep a reference on lock states while checking Benjamin Coddington
2016-11-18 21:55 ` Anna Schumaker
2016-11-19 2:03 ` Benjamin Coddington
2016-11-19 2:11 ` [PATCH v2] " Benjamin Coddington
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.