Re: [PATCH 10/10] block: fix request_queue lifetime handling by making blk_queue_cleanup() proper shutdown

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Oct 18, 2011 at 09:26:24PM -0700, Tejun Heo wrote:

[..]
>  void blk_cleanup_queue(struct request_queue *q)
>  {
> -	/*
> -	 * We know we have process context here, so we can be a little
> -	 * cautious and ensure that pending block actions on this device
> -	 * are done before moving on. Going into this function, we should
> -	 * not have processes doing IO to this device.
> -	 */
> -	blk_sync_queue(q);
> -
> -	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
> +	/* mark @q DEAD, no new request or merges will be allowed afterwards */
>  	mutex_lock(&q->sysfs_lock);
> -	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
> +	spin_lock_irq(q->queue_lock);
> +	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
> +	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
> +	queue_flag_set(QUEUE_FLAG_DEAD, q);
> +	spin_unlock_irq(q->queue_lock);
>  	mutex_unlock(&q->sysfs_lock);
>  
> +	/* drain all requests queued before DEAD marking */
> +	blk_drain_queue(q, true);
> +
> +	/* @q won't process any more request, flush async actions */
> +	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
> +	blk_sync_queue(q);
> +
> +	/* @q is and will stay empty, shutdown and put */
>  	if (q->elevator)
>  		elevator_exit(q->elevator);
> -
>  	blk_throtl_exit(q);

This is based on for-next tree? I am cloning it now. In Linus tree, 
I see that blk_throtl_exit() has been moved to blk_release_queue() again.
So now we probably need to bring it back in blk_cleanup_queue(). That


[..]
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index 900a0c9..8edb949 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -309,6 +309,10 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
>  	struct blkio_cgroup *blkcg;
>  	struct request_queue *q = td->queue;
>  
> +	/* no throttling for dead queue */
> +	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
> +		return NULL;
> +

May be blk_throtl_bio() is probably a better place to do this check, just
before callig throtl_get_tg().

Thanks
Vivek