Messages in this thread | | | Date | Tue, 29 Apr 2008 22:05:13 +0200 | From | Jens Axboe <> | Subject | Re: [PATCH] Optimize lock in queue unplugging |
| |
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
| |