Re: [PATCH] backing dev unplugging

[Jens Axboe <axboe@suse.de>]

On Wed, Mar 10 2004, Jeff Garzik wrote:
> 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.

Thanks

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

Yeah most likely. Remember this is just a first version, there's room
for a little cleanup here and there for sure.

> >@@ -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.

I like to do

	if (expected condition)
		do_stuff

	slow_path

-- 
Jens Axboe