On Jan 4, 2012, at 4:56 PM, Chuck Lever wrote:
>
> On Jan 4, 2012, at 4:52 PM, Trond Myklebust wrote:
>
>> The garbage collector needs to remove the entry from the lru list
>> _and_ the red-black tree atomically in order avoid races in
>> nfs4_get_state_owner.
>>
>> Fix a case in nfs4_get_state_owner in which the garbage-collector
>> list was being manipulated while not holding the appropriate spin
>> lock.
>>
>> nfs4_drop_state_owner doesn't need to look at sp->so_lru: the caller
>> must have a reference to the state owner, so it can't be on the
>> garbage-collection list.
>>
>> Run the garbage collector _after_ we've looked up the state owner
>> for efficiency reasons.
>>
>> Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
>
> Reviewed-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
You might consider squashing this with my patch. The list_del_init() outside the cl_lock is enough to warrant it.
>
>> ---
>> fs/nfs/nfs4state.c | 23 ++++++++++++++++-------
>> 1 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index b9ade99..dc78e0e 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -389,6 +389,8 @@ nfs4_find_state_owner_locked(struct nfs_server *server, struct rpc_cred *cred)
>> else if (cred > sp->so_cred)
>> p = &parent->rb_right;
>> else {
>> + if (!list_empty(&sp->so_lru))
>> + list_del_init(&sp->so_lru);
>> atomic_inc(&sp->so_count);
>> return sp;
>> }
>> @@ -413,6 +415,8 @@ nfs4_insert_state_owner_locked(struct nfs4_state_owner *new)
>> else if (new->so_cred > sp->so_cred)
>> p = &parent->rb_right;
>> else {
>> + if (!list_empty(&sp->so_lru))
>> + list_del_init(&sp->so_lru);
>> atomic_inc(&sp->so_count);
>> return sp;
>> }
>> @@ -459,6 +463,13 @@ nfs4_alloc_state_owner(void)
>> }
>>
>> static void
>> +nfs4_drop_state_owner_locked(struct nfs_server *server, struct nfs4_state_owner *sp)
>> +{
>> + rb_erase(&sp->so_server_node, &server->state_owners);
>> + RB_CLEAR_NODE(&sp->so_server_node);
>> +}
>> +
>> +static void
>> nfs4_drop_state_owner(struct nfs4_state_owner *sp)
>> {
>> if (!RB_EMPTY_NODE(&sp->so_server_node)) {
>> @@ -466,9 +477,8 @@ nfs4_drop_state_owner(struct nfs4_state_owner *sp)
>> struct nfs_client *clp = server->nfs_client;
>>
>> spin_lock(&clp->cl_lock);
>> - list_del_init(&sp->so_lru);
>> - rb_erase(&sp->so_server_node, &server->state_owners);
>> - RB_CLEAR_NODE(&sp->so_server_node);
>> + if (!RB_EMPTY_NODE(&sp->so_server_node))
>> + nfs4_drop_state_owner_locked(server, sp);
>> spin_unlock(&clp->cl_lock);
>> }
>> }
>> @@ -499,6 +509,7 @@ static void nfs4_gc_state_owners(struct nfs_server *server)
>> sp->so_expires + clp->cl_lease_time))
>> break;
>> list_move(&sp->so_lru, &doomed);
>> + nfs4_drop_state_owner_locked(server, sp);
>> }
>> spin_unlock(&clp->cl_lock);
>>
>> @@ -521,8 +532,6 @@ struct nfs4_state_owner *nfs4_get_state_owner(struct nfs_server *server,
>> struct nfs_client *clp = server->nfs_client;
>> struct nfs4_state_owner *sp, *new;
>>
>> - nfs4_gc_state_owners(server);
>> -
>> spin_lock(&clp->cl_lock);
>> sp = nfs4_find_state_owner_locked(server, cred);
>> spin_unlock(&clp->cl_lock);
>> @@ -530,7 +539,7 @@ struct nfs4_state_owner *nfs4_get_state_owner(struct nfs_server *server,
>> goto out;
>> new = nfs4_alloc_state_owner();
>> if (new == NULL)
>> - return NULL;
>> + goto out;
>> new->so_server = server;
>> new->so_cred = cred;
>> spin_lock(&clp->cl_lock);
>> @@ -543,7 +552,7 @@ struct nfs4_state_owner *nfs4_get_state_owner(struct nfs_server *server,
>> kfree(new);
>> }
>> out:
>> - list_del_init(&sp->so_lru);
>> + nfs4_gc_state_owners(server);
>> return sp;
>> }
>>
>> --
>> 1.7.7.5
>>
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
