Hi
Mike Anderson was doing an OLTP benchmark on a computer with 48 physical

disks mapped to one logical device via device mapper.
He found that there was a slowdown on request_queue->lock in function

generic_unplug_device. The slowdown is caused by the fact that when some

code calls unplug on the device mapper, device mapper calls unplug on all

physical disks. These unplug calls take the lock, find that the queue is

already unplugged, release the lock and exit.
With the below patch, performance of the benchmark was increased by 18%

(the whole OLTP application, not just block layer microbenchmarks).
So I'm submitting this patch for upstream. I think the patch is correct,

because when more threads call simultaneously plug and unplug, it is

unspecified, if the queue is or isn't plugged (so the patch can't make

this worse). And the caller that plugged the queue should unplug it

anyway. (if it doesn't, there's 3ms timeout).
Mikulas
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
---

  block/blk-core.c |    3 +++

  1 file changed, 3 insertions(+)
Index: linux-2.6.25/block/blk-core.c

===================================================================

--- linux-2.6.25.orig/block/blk-core.c	2008-04-17 04:49:44.000000000 +0200

+++ linux-2.6.25/block/blk-core.c	2008-04-29 18:50:37.000000000 +0200

@@ -271,6 +271,9 @@ EXPORT_SYMBOL(__generic_unplug_device);

   **/

  void generic_unplug_device(struct request_queue *q)

  {

+	if (likely(!blk_queue_plugged(q)))

+		return;

+

  	spin_lock_irq(q->queue_lock);

  	__generic_unplug_device(q);

  	spin_unlock_irq(q->queue_lock);

--

Where were these unplug calls coming from? The block layer will

generally only unplug when it is already unplugged, so if you are seeing

so many unplug calls that the patch redues overhead by as much

described, perhaps the callsite is buggy?
--

Jens Axboe
--

I do not have direct access the the benchmark setup, but here is the data

I have received.
The oprofile data was showing ll_rw_blk::generic_unplug_device() as a top

routine at 13% of the samples. Annotation of the samples shows hits on

spin_lock_irq(q->queue_lock).
Here are some sample call traces:
Call trace #1
kernel:  [<ffffffff80058c6c>] generic_unplug_device+0x5d/0xc6

kernel:  [<ffffffff8820ea3e>] :dm_mod:dm_table_unplug_all+0x33/0x41

kernel:  [<ffffffff8820cc85>] :dm_mod:dm_unplug_all+0x1d/0x28

kernel:  [<ffffffff8005a78a>] blk_backing_dev_unplug+0x56/0x5b

kernel:  [<ffffffff80014cdc>] sync_buffer+0x36/0x3f

kernel:  [<ffffffff800629a4>] __wait_on_bit+0x40/0x6f

kernel:  [<ffffffff80014ca6>] sync_buffer+0x0/0x3f

kernel:  [<ffffffff80062a3f>] out_of_line_wait_on_bit+0x6c/0x78

kernel:  [<ffffffff8009c474>] wake_bit_function+0x0/0x23

kernel:  [<ffffffff88034c85>] :jbd:journal_commit_transaction+0x91f/0x1086

kernel:  [<ffffffff8003d038>] lock_timer_base+0x1b/0x3c

kernel:  [<ffffffff8803840e>] :jbd:kjournald+0xc1/0x213

kernel:  [<ffffffff8009c446>] autoremove_wake_function+0x0/0x2e

kernel:  [<ffffffff8009c283>] keventd_create_kthread+0x0/0x61

kernel:  [<ffffffff8803834d>] :jbd:kjournald+0x0/0x213

kernel:  [<ffffffff8009c283>] keventd_create_kthread+0x0/0x61

kernel:  [<ffffffff800321d5>] kthread+0xfe/0x132

kernel:  [<ffffffff8005cfb1>] child_rip+0xa/0x11

kernel:  [<ffffffff8009c283>] keventd_create_kthread+0x0/0x61

kernel:  [<ffffffff800320d7>] kthread+0x0/0x132

kernel:  [<ffffffff8005cfa7>] child_rip+0x0/0x11                       
Call trace #2

kernel:  [<ffffffff80058c6c>] generic_unplug_device+0x5d/0xc6

kernel:  [<ffffffff8820ea3e>] :dm_mod:dm_table_unplug_all+0x33/0x41

kernel:  [<ffffffff8820cc85>] :dm_mod:dm_unplug_all+0x1d/0x28

kernel:  [<ffffffff8005a78a>] blk_backing_dev_unplug+0x56/0x5b

kernel:  [<ffffffff80...
[ message continues ]

So it's basically dm calling into blk_unplug() all the time, which

doesn't check if the queue is plugged. The reason why I didn't like the

initial patch is that ->unplug_fn() really should not be called unless

the queue IS plugged. So how about this instead:
http://git.kernel.dk/?p=linux-2.6-block.git;a=commit;h=c44993018887e82ab...
That's a lot more appropriate, imho.
--

Jens Axboe
--

This doesn't seem correct to me. The difference between blk_unplug and

generic_unplug_device is that blk_unplug is called on every type of device

and generic_unplug_device (pointed to by q->unplug_fn) is a method that is

called on low-level disk devices.
dm and md redefine q->unplug_fn to point to their own method. On dm and

md, blk_unplug is called, but generic_unplug_device is not.
So if you have this setup

dm-linear(unplugged) -> disk(plugged)
then, with your patch, a call to blk_unplug(dm-linear) will not unplug the

disk. With my patch, a call to blk_unplug(dm-linear) will unplug the disk

--- it calls q->unplug_fn that points to dm_unplug_all, that calls

blk_unplug again on the disk and that calls generic_unplug_device on disk

queue.
Mikulas

--

That is because the md/dm don't set the plugged flag which I think they

should. So we fix that instead so that plugging works the same from the

block core or from a driver instead of adding work-arounds in the block

unplug handler. Adding a check for plugged in the plug handler is a

hack, I don't see how you can argue against that.
--

Jens Axboe
--

When should dm/md set the plugged flag? It has several disks (or more

dm/md layers), some of them may be plugged, some not --- furthermore, the

disk queues are shared by other devices --- there may be more devices on

different partitions.
So the question: when do you want dm/md to set the plugged bit? Do you 
I changed generic_unplug_device from:

spin_lock()

if (plugged bit is set) unplug...;

spin_unlock()
to:

if (plugged bit is set)

{

spin_lock()

if (plugged bit is set) unplug...;

spin_unlock()

}
--- I don't see anything wrong with it. At least, the change is so trivial

that we can be sure that it won't have negative effect on performance.
What you propose is complete rewrite of dm/md plugging mechanism to plug

the queue and merge requests at upper layer --- the questions are:
- what exactly you are proposing? (plugging at upper layer? lower layer?

both layers? don't plug and just somehow propagate the plugged bit?)

- why are you proposing that?
Note that for some dm targets it would be benefical to join the requests

at upper layer (dm-linear, raid1), for others (raid0, dm-snapshots) it

damages performance (you merge the requests before passing them to raid0

and you chop them again to smaller pieces in raid0).
--

It will indeed get a lot more involved. Sigh. Well I'll apply your patch

to generic_unplug_device(), it's at least a worth while optimization in

the mean time.
--

Jens Axboe
--

Makes sense to me, ack.
Alasdair

--

agk@redhat.com

--

unplug is called on any wait_on_buffer (and similar calls)

__wait_on_buffer -> sync_buffer -> blk_run_address_space ->

blk_run_backing_dev -> bdi->unplug_io_fn(bdi, page);
(I'm not sure that this was the IBM's case, I'm just guessing - this is

the most obvious example where unplug is called repeatedly)
There is not any test that the queue is plugged and there shouldn't be. If

you have this situation
dm-linear(unplugged) -> physical-disk(plugged)
then uplung should be called on dm-linear (that will call dm-unplug method

dm_unplug_all and that will unplug the disk). If you add the test of

plugged queue to the upper layer, you mess this situation with stacked

drivers completely.
The test for already plugged queue should be at the lowest physical device

driver, not in upper layers.
Mikulas

--

Fair enough, I'll put the patch under closer scrutiny and queue it up.

Thanks!
--

Jens Axboe
--