Re: [PATCH 2/4] block: Avoid that request_fn is invoked on a dead queue

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Oct 10, 2012 at 05:08:01PM +0200, Bart Van Assche wrote:
> A block driver may start cleaning up resources needed by its
> request_fn as soon as blk_cleanup_queue() finished, so request_fn
> must not be invoked after draining finished.

Can you please make the commit message more verbose preferably with
example crash trace?  It's difficult to tell what bug it's trying to
fix how.

>  /**
> + * __blk_run_queue_uncond - run a queue whether or not it has been stopped
> + * @q:	The queue to run
> + *
> + * Description:
> + *    Invoke request handling on a queue if there are any pending requests.
> + *    May be used to restart request handling after a request has completed.
> + *    This variant runs the queue whether or not the queue has been
> + *    stopped. Must be called with the queue lock held and interrupts
> + *    disabled. See also @blk_run_queue.
> + */
> +void __blk_run_queue_uncond(struct request_queue *q)
> +{
> +	if (unlikely(blk_queue_dead(q)))
> +		return;
> +
> +	q->request_fn(q);
> +}
> +
> +/**
>   * __blk_run_queue - run a single device queue
>   * @q:	The queue to run
>   *
> @@ -305,7 +324,7 @@ void __blk_run_queue(struct request_queue *q)
>  	if (unlikely(blk_queue_stopped(q)))
>  		return;
>  
> -	q->request_fn(q);
> +	__blk_run_queue_uncond(q);

__blk_run_queue_uncond() is a cold path and I don't think adding a
test there matters but I think it would be better if we avoid an extra
branch if possible for __blk_run_queue().  Can't we merge
blk_queue_stopped/dead() testing?

> @@ -401,6 +420,9 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
>  			}
>  		}
>  
> +		if (!drain && blk_queue_dying(q))
> +			queue_flag_set(QUEUE_FLAG_DEAD, q);
> +

Wouldn't doing this in blk_cleanup_queue() be better?  It may involve
an extra queue locking but I don't think that matter at all in that
path and doing things where they belong is far more important.

Thanks.

-- 
tejun