Re: [PATCH] backing dev unplugging

[Jeff Garzik <jgarzik@pobox.com>]

Jens Axboe wrote:
> Hi,
> 
> Here's a first cut at killing global plugging of block devices to reduce
> the nasty contention blk_plug_lock caused. This introduceds per-queue
> plugging, controlled by the backing_dev_info. Observations:
> 
> - Most uses of blk_run_queues() without a specific context was bogus.
>   Usually the act of kicking the targets in question should be (and
>   already are) performed by other activities, such as making the vm
>   flushing run to free memory.
> 
> - Some use of blk_run_queues() really just want to kick the final queue
>   where the bio goes to. I've added bio_sync() (BIO_RW_SYNC) to manage
>   those, if the queue needs unplugging we'll do it when holding the lock
>   for the queue already.
> 
> - The dm bit needs careful examination and checking of Joe. It could be
>   more clever and maintain plug state of each target, I just added a
>   dm_table unplug all functionality.
> 
> Patch is against 2.6.4-rc2-mm1.

I like it a lot.

Any chance some of these newly-shortened functions can become static 
inline as well?


> @@ -1100,13 +1092,11 @@
>  	 * don't plug a stopped queue, it must be paired with blk_start_queue()
>  	 * which will restart the queueing
>  	 */
> -	if (!blk_queue_plugged(q)
> -	    && !test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)) {
> -		spin_lock(&blk_plug_lock);
> -		list_add_tail(&q->plug_list, &blk_plug_list);
> +	if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))
> +		return;
> +
> +	if (!test_and_set_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
>  		mod_timer(&q->unplug_timer, jiffies + q->unplug_delay);
> -		spin_unlock(&blk_plug_lock);
> -	}
>  }
>  
>  EXPORT_SYMBOL(blk_plug_device);
> @@ -1118,15 +1108,12 @@
>  int blk_remove_plug(request_queue_t *q)
>  {
>  	WARN_ON(!irqs_disabled());
> -	if (blk_queue_plugged(q)) {
> -		spin_lock(&blk_plug_lock);
> -		list_del_init(&q->plug_list);
> -		del_timer(&q->unplug_timer);
> -		spin_unlock(&blk_plug_lock);
> -		return 1;
> -	}
>  
> -	return 0;
> +	if (!test_and_clear_bit(QUEUE_FLAG_PLUGGED, &q->queue_flags))
> +		return 0;
> +
> +	del_timer(&q->unplug_timer);
> +	return 1;
>  }
>  
>  EXPORT_SYMBOL(blk_remove_plug);

I tend to like

	if (test_and_clear_bit())
		call_out_of_line_function()

style for small functions like this, and inline those.

	Jeff