| Message ID | 6a50c9ed9077e43de5da454a2413769a0b383d58.1479427989.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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; }
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(-)