Re: [PATCH] Optimize lock in queue unplugging

[Jens Axboe <jens.axboe@oracle.com>]

On Tue, Apr 29 2008, Mikulas Patocka wrote:
> 
> 
> On Tue, 29 Apr 2008, Jens Axboe wrote:
> 
> >On Tue, Apr 29 2008, Mikulas Patocka wrote:
> >>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).
> >
> >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
> 
> 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.

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

-- 
Jens Axboe