Re: [PATCH] nfs: don't queue synchronous NFSv4 close rpc_release to nfsiod

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Tue, 2011-02-15 at 18:47 -0500, Trond Myklebust wrote: 
> diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
> index 243fc09..11b71b1 100644
> --- a/net/sunrpc/sched.c
> +++ b/net/sunrpc/sched.c
> @@ -252,23 +252,39 @@ static void rpc_set_active(struct rpc_task *task)
>  
>  /*
>   * Mark an RPC call as having completed by clearing the 'active' bit
> + * and then waking up all tasks that were sleeping.
>   */
> -static void rpc_mark_complete_task(struct rpc_task *task)
> +static int rpc_complete_task(struct rpc_task *task)
>  {
> -	smp_mb__before_clear_bit();
> +	void *m = &task->tk_runstate;
> +	wait_queue_head_t *wq = bit_waitqueue(m, RPC_TASK_ACTIVE);
> +	struct wait_bit_key k = __WAIT_BIT_KEY_INITIALIZER(m, RPC_TASK_ACTIVE);
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&wq->lock, flags);
>  	clear_bit(RPC_TASK_ACTIVE, &task->tk_runstate);
> -	smp_mb__after_clear_bit();
> -	wake_up_bit(&task->tk_runstate, RPC_TASK_ACTIVE);
> +	if (waitqueue_active(wq))
> +		__wake_up_locked_key(wq, TASK_NORMAL, &k);
> +	ret = atomic_dec_and_test(&task->tk_count);
> +	spin_unlock_irqrestore(&wq->lock, flags);
> +	return ret;
>  }
>  
>  /*
>   * Allow callers to wait for completion of an RPC call
> + *
> + * Note the use of out_of_line_wait_on_bit() rather than wait_on_bit()
> + * to enforce taking of the wq->lock and hence avoid races with
> + * rpc_complete_task().
>   */
>  int __rpc_wait_for_completion_task(struct rpc_task *task, int (*action)(void *))
>  {
> +	BUG_ON(!RPC_IS_ASYNC(task));
> +
>  	if (action == NULL)
>  		action = rpc_wait_bit_killable;
> -	return wait_on_bit(&task->tk_runstate, RPC_TASK_ACTIVE,
> +	return out_of_line_wait_on_bit(&task->tk_runstate, RPC_TASK_ACTIVE,
>  			action, TASK_KILLABLE);
>  }
>  EXPORT_SYMBOL_GPL(__rpc_wait_for_completion_task);

Never mind. The above ordering scheme is broken by the fact that
'wake_bit_function' calls autoremove_wake_function, which again means
that finish_wait optimises away the spin lock.

Back to the drawing board...

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com