IO scheduler, queue depth, nr_requests
Subsystems (3)
Hello,
as you might have seen from the linux-kernel mailinglist
I have been testing for months now with a fileserver set up to use XFS over LVM2 on a 3ware RAID5 controller.
I asked for help several times on the list, but nobody really replied, so now I'm taking a shot at mailing you directly, since you appear to be the I/O request queueing guru of the kernel ;)
Cc: sent to [email protected]. Any hint appreciated.
For some reason, when using LVM, write requests get queued out of order to the 3ware controller, which results in quite a bit of seeking and thus performance loss.
The default queue depth of the 3ware controller is 254. I found out that lowering it to 64 in the driver fixed my problems, and I advised 3ware support about this. They weren't really convinced..
By fiddling about today I just found that changing /sys/block/sda/queue/nr_requests from 128 to something above the queue depth of the 3ware controller (256 doesn't work, 384 and up do) also fixes the problem.
Does that actually make sense ?
Ah yes, I'm currently using 2.6.2 with a 3ware 8506-8 in hardware raid5 mode, deadline scheduler, PIV 3.0 Ghz, 2 GB RAM.
Debug output ("mydd" works just like "dd", but has an fsync option):
- /mnt is an XFS filesystem on a LVM2 volume on the 3ware
- /mnt2 is an XFS filesystem directly on /dev/sda1 of the 3ware
- First on /mnt, the LVM partition. Note that a small "dd" runs fast, a larger one runs slower:
# cd /mnt # cat /sys/block/sda/device/queue_depth 254 # cat /sys/block/sda/queue/nr_requests 128 # ~/mydd --if /dev/zero --of file --bs 4096 --count 50000 --fsync 204800000 bytes transferred in 2.679812 seconds (76423271 bytes/sec) # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 9.501549 seconds (43108760 bytes/sec)
- Now I set the nr_requests to 512:
# echo 512 > /sys/block/sda/queue/nr_requests # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 5.374437 seconds (76212634 bytes/sec)
See that ? Weird thing is, it's only on LVM, directly on /dev/sda1 no problem at all:
# cat /sys/block/sda/device/queue_depth 254 # cat /sys/block/sda/queue/nr_requests 128 # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 5.135642 seconds (79756338 bytes/sec)
Somehow, LVM is causing the requests to the underlying 3ware device to get out of order, and increasing nr_requests to be larger than the queue_depth of the device fixes this.
I tried the latest dm-patches in -mm (applied those to vanilla 2.6.2), which include a patch called dm-04-maintain-bio-ordering.patch but that doesn't really help (at first I though otherwise, but the tests scripts I used lowered the queue_depth of the 3ware to 64 by accident) - if anything, it makes things worse.
# ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 13.138224 seconds (31176208 bytes/sec)
Setting nr_requests to 512 fixes things up again.
Mike.
Quoted 19 lines · Miquel van Smoorenburg
Hello,
as you might have seen from the linux-kernel mailinglist
I have been testing for months now with a fileserver set up to use XFS over LVM2 on a 3ware RAID5 controller.I asked for help several times on the list, but nobody really replied, so now I'm taking a shot at mailing you directly, since you appear to be the I/O request queueing guru of the kernel ;)
Cc: sent to [email protected]. Any hint appreciated.For some reason, when using LVM, write requests get queued out of order to the 3ware controller, which results in quite a bit of seeking and thus performance loss.
The default queue depth of the 3ware controller is 254. I found out that lowering it to 64 in the driver fixed my problems, and I advised 3ware support about this. They weren't really convinced..
Quoted 6 lines
By fiddling about today I just found that changing
/sys/block/sda/queue/nr_requestsfrom 128 to something above the queue depth of the 3ware controller (256 doesn't work, 384 and up do) also fixes the problem.Does that actually make sense ?
Yes, it makes perfect sense, I've been aware of this problem for quite some time. If you look init_tag_map() in ll_rw_blk.c:
if (depth > q->nr_requests / 2) {
q->nr_requests = depth * 2;
printk(KERN_INFO "%s: large TCQ depth: adjusted nr_requests "
"to %lu\n", __FUNCTION__, q->nr_requests);
}it pretty much matches the problem you outlined. Unfortunately, the tagging depth of SCSI drivers cannot be controlled unless they use the generic block tagging helpers, and to my knowledge only a single driver does...
Quoted 46 lines
Ah yes, I'm currently using 2.6.2 with a 3ware 8506-8 in hardware raid5 mode, deadline scheduler, PIV 3.0 Ghz, 2 GB RAM.
Debug output ("mydd" works just like "dd", but has an fsync option):
- /mnt is an XFS filesystem on a LVM2 volume on the 3ware
- /mnt2 is an XFS filesystem directly on/dev/sda1of the 3ware- First on /mnt, the LVM partition. Note that a small "dd" runs fast, a larger one runs slower:
# cd /mnt # cat /sys/block/sda/device/queue_depth 254 # cat /sys/block/sda/queue/nr_requests 128 # ~/mydd --if /dev/zero --of file --bs 4096 --count 50000 --fsync 204800000 bytes transferred in 2.679812 seconds (76423271 bytes/sec) # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 9.501549 seconds (43108760 bytes/sec)- Now I set the
nr_requeststo 512:# echo 512 > /sys/block/sda/queue/nr_requests # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 5.374437 seconds (76212634 bytes/sec)See that ? Weird thing is, it's only on LVM, directly on
/dev/sda1no problem at all:# cat /sys/block/sda/device/queue_depth 254 # cat /sys/block/sda/queue/nr_requests 128 # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 5.135642 seconds (79756338 bytes/sec)Somehow, LVM is causing the requests to the underlying 3ware device to get out of order, and increasing
nr_requeststo be larger than thequeue_depthof the device fixes this.I tried the latest dm-patches in -mm (applied those to vanilla 2.6.2), which include a patch called dm-04-maintain-bio-ordering.patch but that doesn't really help (at first I though otherwise, but the tests scripts I used lowered the
queue_depthof the 3ware to 64 by accident) - if anything, it makes things worse.
Quoted 4 lines
# ~/mydd --if
/dev/zero--of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 13.138224 seconds (31176208 bytes/sec)Setting
nr_requeststo 512 fixes things up again.
Seems there's an extra problem here, the nr_requests vs depth problem should not be too problematic unless you have heavy random io. Doesn't look like dm is reordering (bio_list_add() adds to tail, flush_deferred_io() processes from head. direct queueing doesn't look like it's reordering). Can the dm folks verify this?
Or, you are just being hit by the problem first listed - requests get no hold time in the io scheduler for merging, because the driver drains them too quickly because of this artificially huge queue depth. If you did some stats on average request size and io/sec rate that should tell you for sure. I don't know what you have behind the 3ware, but it's generally not advised to use more than 4 tags per spindle.
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
Seems there's an extra problem here, the
nr_requestsvs depth problem should not be too problematic unless you have heavy random io. Doesn't look like dm is reordering (bio_list_add()adds to tail,flush_deferred_io()processes from head. direct queueing doesn't look like it's reordering). Can the dm folks verify this?
Ordering is certainly maintained for the simple targets (linear, striped). Snapshots and mirroring still need some work in this regard, but I don't think these are begin used for the tests.
The place that I am expecting dm to cause poor performance is due to this bit of stupidity in dm-table.c. It's on my TODO list, but is not getting to the op since no-one seems to be complaining about it yet.
/* FIXME: Device-Mapper on top of RAID-0 breaks because DM
* currently doesn't honor MD's merge_bvec_fn routine.
* In this case, we'll force DM to use PAGE_SIZE or
* smaller I/O, just to be safe. A better fix is in the
* works, but add this for the time being so it will at
* least operate correctly.
*/
if (q->merge_bvec_fn)
rs->max_sectors =
min_not_zero(rs->max_sectors,
(unsigned short)(PAGE_SIZE >> 9));- Joe
Quoted 8 lines · Jens Axboe
Quoted 6 lines · Miquel van Smoorenburg
Hello,
as you might have seen from the linux-kernel mailinglist
I have been testing for months now with a fileserver set up to use XFS over LVM2 on a 3ware RAID5 controller.
I too have been fighting lower-than-expected performance with this identical combination, other than using a P4 2.8GHz instead of 3.0GHz, and 1GB RAM instead of 2GB. My 3ware 8506-8 has six Seagate Barracuda SATA drives behind it, which are very fast on their own :-) Try as I might, with the 2.6 kernel I have not been able to generate a bonnie++ or iozone run that provided performance better than a single disk, even with the 3ware card in JBOD, RAID-0 or RAID-1 mode. I had been conversing with 3ware about it, but to no real effect given than they don't support 2.6 yet.
Quoted 4 lines
Quoted 4 lines
By fiddling about today I just found that changing
/sys/block/sda/queue/nr_requestsfrom 128 to something above the queue depth of the 3ware controller (256 doesn't work, 384 and up do) also fixes the problem.
I will try this as well this evening and report my results.
Quoted 4 lines · Jens Axboe
Quoted 4 lines
By fiddling about today I just found that changing
/sys/block/sda/queue/nr_requestsfrom 128 to something above the queue depth of the 3ware controller (256 doesn't work, 384 and up do) also fixes the problem.
OK, I have run three sets of bonnie++ tests on my system. This was using the 2.6-bk kernel as of about midnight GMT of 2004-02-17.
System is single P4HT 2.8GHz (SMP kernel), 1GB RAM (4GB highmem enabled), Intel 865G chipset, 3ware 8506-8 with six Seagate 160GB Barracuda SATA disks in a single RAID-5. bonnie++ tests run on an XFS filesystem (made with default mkfs.xfs parameters using a recent xfstools package) over a 200GB LVM2 volume (dm-linear).
bonnie++ -r 512 -s 40960 -f -b
Anticipatory scheduler, nr_requests=128 (default)
Seq. Write 53772 Rand. Write 15557 Seq. Read 47730 Rand. Seek 146.5
Anticipatory scheduler, nr_requests=384
Seq. Write 53843 Rand. Write 17595 Seq. Read 44663 Rand. Seek 143.1
Deadline scheduler, nr_requests=384
Seq. Write 54973 Rand. Write 18897 Seq. Read 41476 Rand. Seek 227.3 (!)
Miquel's suggestion has a definite positive effect, except on sequential reads. This system still doesn't produce anywhere near the throughput I'd expect though, and still doesn't come close to 2.4 numbers either (but those are somewhat bogus, as they rely on setting extremely large readahead settings). There may be some sort of hardware problem that is limiting me to ~60MB/s, although with 2.4 I was able to get sequential read numbers around 95MB/s, so I think the hardware is OK.
Quoted 7 lines · Jens Axboe
Quoted 5 lines · Miquel van Smoorenburg
By fiddling about today I just found that changing
/sys/block/sda/queue/nr_requestsfrom 128 to something above the queue depth of the 3ware controller (256 doesn't work, 384 and up do) also fixes the problem.
[....]
Quoted 20 lines
Quoted 10 lines
See that ? Weird thing is, it's only on LVM, directly on
/dev/sda1no problem at all:# cat /sys/block/sda/device/queue_depth 254 # cat /sys/block/sda/queue/nr_requests 128 # ~/mydd --if /dev/zero --of file --bs 4096 --count 100000 --fsync 409600000 bytes transferred in 5.135642 seconds (79756338 bytes/sec)Quoted 3 lines
Somehow, LVM is causing the requests to the underlying 3ware device to get out of order, and increasing
nr_requeststo be larger than thequeue_depthof the device fixes this.Seems there's an extra problem here, the
nr_requestsvs depth problem should not be too problematic unless you have heavy random io. Doesn't look like dm is reordering (bio_list_add()adds to tail,flush_deferred_io()processes from head. direct queueing doesn't look like it's reordering). Can the dm folks verify this?
Quoted 6 lines
Or, you are just being hit by the problem first listed - requests get no hold time in the io scheduler for merging, because the driver drains them too quickly because of this artificially huge queue depth. If you did some stats on average request size and io/sec rate that should tell you for sure. I don't know what you have behind the 3ware, but it's generally not advised to use more than 4 tags per spindle.
Okay I repeated some earlier tests, and I added some debug code in several places.
I added logging to tw_scsi_queue() in the 3ware driver to log the start sector and length of each request. It logs something like:
3wdbg: id 119, lba = 0x2330bc33, num_sectors = 256
With a perl script, I can check if the requests are sent to the host in order. That outputs something like this:
Consecutive: start 1180906348, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180906340, length 8 sec (4 KB), requests: 1
Consecutive: start 1180914292, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180914284, length 8 sec (4 KB), requests: 1
Consecutive: start 1180922236, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180922228, length 8 sec (4 KB), requests: 1
Consecutive: start 1180930180, length 7936 sec (3968 KB), requests: 31
See, 31 requests in order, then one request "backwards", then 31 in order, etc.
I added some queue debug code as well, both the LVM2 queue and 3ware queue have the following settings:
max_sectors: 256max_phys_segments: 128max_hw_segments: 62hardsect_size: 512max_segment_size: 65536seg_boundary_mask: ffffffff
Now 31 * 2 == 62 == max_hw_segments .. coincidence ?
Weird thing is, still still only happens with LVM over 3ware raid5, not on /dev/sda1 of the 3ware directly.
I added some printk's to scsi_request_fn() in scsi_lib.c to see if a requests was getting requeued - but no.
Upping nr_requests to 2 * queue_depth does still fix things, but as you said that should not be necessary. This bugs me, I want to find out why this only happens with LVM and not without ...
Mike.
Quoted 3 lines · Miquel van Smoorenburg
For some reason, when using LVM, write requests get queued out of order to the 3ware controller, which results in quite a bit of seeking and thus performance loss.
[..]
Quoted 18 lines
Okay I repeated some earlier tests, and I added some debug code in several places.
I added logging to
tw_scsi_queue()in the 3ware driver to log the start sector and length of each request. It logs something like:
3wdbg: id 119, lba =0x2330bc33,num_sectors= 256With a perl script, I can check if the requests are sent to the host in order. That outputs something like this:
Consecutive: start 1180906348, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180906340, length 8 sec (4 KB), requests: 1
Consecutive: start 1180914292, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180914284, length 8 sec (4 KB), requests: 1
Consecutive: start 1180922236, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180922228, length 8 sec (4 KB), requests: 1
Consecutive: start 1180930180, length 7936 sec (3968 KB), requests: 31
Quoted 1 line
See, 31 requests in order, then one request "backwards", then 31 in order, etc.
I found out what causes this. It's get_request_wait().
When the request queue is full, and a new request needs to be created, __make_request() blocks in get_request_wait().
Another process wakes up first (pdflush / process submitting I/O itself / xfsdatad / etc) and sends the next bio's to __make_request(). In the mean time some free requests have become available, and the bios are merged into a new request. Those requests are submitted to the device.
Then, get_request_wait() returns but the bio is not mergeable anymore - and that results in a backwards seek, severely limiting the I/O rate.
Wouldn't it be better to allow the request allocation and queue the request, and /then/ put the process to sleep ? The queue will grow larger than nr_requests, but it does that anyway.
Mike.
Quoted 16 lines · Miquel van Smoorenburg
Quoted 3 lines · Miquel van Smoorenburg
For some reason, when using LVM, write requests get queued out of order to the 3ware controller, which results in quite a bit of seeking and thus performance loss.
[..]
Quoted 10 lines
Okay I repeated some earlier tests, and I added some debug code in several places.
I added logging to
tw_scsi_queue()in the 3ware driver to log the start sector and length of each request. It logs something like:
3wdbg: id 119, lba =0x2330bc33,num_sectors= 256With a perl script, I can check if the requests are sent to the host in order. That outputs something like this:
Quoted 7 lines
Quoted 7 lines
Consecutive: start 1180906348, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180906340, length 8 sec (4 KB), requests: 1
Consecutive: start 1180914292, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180914284, length 8 sec (4 KB), requests: 1
Consecutive: start 1180922236, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180922228, length 8 sec (4 KB), requests: 1
Consecutive: start 1180930180, length 7936 sec (3968 KB), requests: 31
3968 / 31 = 128 exactly. I think when you say "requests: 31" you are actually referring to a single request which has 31 128k BIOs in it, yes?
Quoted 13 lines
Quoted 2 lines
See, 31 requests in order, then one request "backwards", then 31 in order, etc.
I found out what causes this. It's
get_request_wait().When the request queue is full, and a new request needs to be created,
__make_request()blocks inget_request_wait().Another process wakes up first (pdflush / process submitting I/O itself / xfsdatad / etc) and sends the next bio's to
__make_request(). In the mean time some free requests have become available, and the bios are merged into a new request. Those requests are submitted to the device.
Quoted 6 lines
Then,
get_request_wait()returns but the bio is not mergeable anymore - and that results in a backwards seek, severely limiting the I/O rate.Wouldn't it be better to allow the request allocation and queue the request, and /then/ put the process to sleep ? The queue will grow larger than
nr_requests, but it does that anyway.
That would help, but is a bit kludgy.
What _should_ have happened was:
a) The queue gets plugged
b) The maximal-sized 31-bio request is queued
c) The 4k request gets inserted *before* the 3968k request
d) The queue gets unplugged, and both requests stream smoothly.
(And this assumes that the queue was initially empty. ALmost certainly that was not the case).
So the question is, why did the little, later 4k request not get itself inserted in front of the earlier, large 3968k request?
Are you using md as well? If so, try removing the blk_run_queues() statements from md_thread() and md_do_sync(). And generally hunt around and try nuking blk_run_queues() statements from the device driver/md/dm layer - they may well be wrong.
Is this problem specific to md? Can it be reproduced on a boring-old-disk?
Does elevator=deadline change anything?
Quoted 25 lines · Andrew Morton
Quoted 16 lines · Miquel van Smoorenburg
Quoted 3 lines · Miquel van Smoorenburg
For some reason, when using LVM, write requests get queued out of order to the 3ware controller, which results in quite a bit of seeking and thus performance loss.
[..]
Quoted 10 lines
Okay I repeated some earlier tests, and I added some debug code in several places.
I added logging to
tw_scsi_queue()in the 3ware driver to log the start sector and length of each request. It logs something like:
3wdbg: id 119, lba =0x2330bc33,num_sectors= 256With a perl script, I can check if the requests are sent to the host in order. That outputs something like this:
Quoted 7 lines
Quoted 7 lines
Consecutive: start 1180906348, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180906340, length 8 sec (4 KB), requests: 1
Consecutive: start 1180914292, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180914284, length 8 sec (4 KB), requests: 1
Consecutive: start 1180922236, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180922228, length 8 sec (4 KB), requests: 1
Consecutive: start 1180930180, length 7936 sec (3968 KB), requests: 31
Quoted 2 lines
3968 / 31 = 128 exactly. I think when you say "requests: 31" you are actually referring to a single request which has 31 128k BIOs in it, yes?
No, I'm actually referring to a struct request. I'm logging this in the SCSI layer, in scsi_request_fn(), just after elv_next_request(). I have in fact logged all the bio's submitted to __make_request, and the output of the elevator from elv_next_request(). The bio's are submitted sequentially, the resulting requests aren't. But this is because nr_requests is 128, while the 3ware device has a queue of 254 entries (no tagging though). Upping nr_requests to 512 makes this go away ..
That shouldn't be necessary though. I only see this with LVM over 3ware-raid5, not on the 3ware-raid5 array directly (/dev/sda1). And it gets less troublesome with a lot of debugging (unless I set nr_requests lower again), which points to a timing issue.
Quoted 12 lines
Quoted 12 lines
Quoted 1 line
See, 31 requests in order, then one request "backwards", then 31 in order, etc.
I found out what causes this. It's
get_request_wait().When the request queue is full, and a new request needs to be created,
__make_request()blocks inget_request_wait().Another process wakes up first (pdflush / process submitting I/O itself / xfsdatad / etc) and sends the next bio's to
__make_request(). In the mean time some free requests have become available, and the bios are merged into a new request. Those requests are submitted to the device.
Quoted 8 lines
Quoted 6 lines
Then,
get_request_wait()returns but the bio is not mergeable anymore - and that results in a backwards seek, severely limiting the I/O rate.Wouldn't it be better to allow the request allocation and queue the request, and /then/ put the process to sleep ? The queue will grow larger than
nr_requests, but it does that anyway.That would help, but is a bit kludgy.
Patch that does exactly this (it's probably butt-ugly, but it proves the point) below.
Quoted 4 lines
What _should_ have happened was:
a) The queue gets plugged
Quoted 8 lines
b) The maximal-sized 31-bio request is queued
c) The 4k request gets inserted *before* the 3968k request
d) The queue gets unplugged, and both requests stream smoothly.
(And this assumes that the queue was initially empty. ALmost certainly that was not the case).
The thing is, the bio's are submitted perfectly sequentially. It's just that every so often a request (with just its initial bio) gets stuck for a while. Because the bio's after it are merged and sent to the device, it's not possible to merge that stuck request later on when it gets unstuck, because the other bio's have already left the building so to speak.
I'm not using MD at all, and I tried all elevators. All the same results, so I stayed with elevator=noop because it's so much easier to understand ;)
This proof-of-concept patch fixes things:
ll_rw_blk.c.patch
(Hmm, 03:11, time to go to bed. I'm happy I finally found this though!)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c.ORIG +27 -11
@@ -1545,7 +1545,8 @@/* * Get a free request, queue_lock must not be held */ -static struct request *get_request (request_queue_t *q,int rw,int gfp_mask) +static struct request *get_request (request_queue_t *q,int rw, +int gfp_mask,int *qfull) {struct request *rq =NULL ;struct request_list *rl = &q->rq;
@@ -1569,10 +1570,15 @@ && !ioc_batching(ioc) && !elv_may_queue(q, rw)) {/* * The queue is full and the allocating process is not a -* "batcher", and not exempted by the IO scheduler +* "batcher", and not exempted by the IO scheduler. +* If "qfull" is a valid pointer, set it to 1 to return +* this info to the caller so that it can sleep. */ -spin_unlock_irq (q->queue_lock); - goto out; +if (qfull ==NULL ) { +spin_unlock_irq (q->queue_lock); + goto out; + }else + *qfull =1 ; } rl->count[rw]++;
@@ -1627,7 +1633,7 @@* No available requests for this queue, unplug the device and wait for some * requests to become available. */ -static struct request *get_request_wait (request_queue_t *q,int rw) +static struct request *get_request_wait (request_queue_t *q,int rw,int wantreq) {DEFINE_WAIT (wait);struct request *rq;
@@ -1639,7 +1645,7 @@prepare_to_wait_exclusive (&rl->wait[rw], &wait,TASK_UNINTERRUPTIBLE ); - rq =get_request (q, rw,GFP_NOIO ); + rq = wantreq ?get_request (q, rw,GFP_NOIO ,NULL ) :NULL ;if (!rq) {struct io_context *ioc;
@@ -1657,7 +1663,7 @@put_io_context (ioc); }finish_wait (&rl->wait[rw], &wait); - }while (!rq); + }while (!rq && wantreq);return rq; }
@@ -1669,9 +1675,9 @@BUG_ON (rw !=READ && rw !=WRITE );if (gfp_mask & __GFP_WAIT) - rq =get_request_wait (q, rw); + rq =get_request_wait (q, rw,1 );else - rq =get_request (q, rw, gfp_mask); + rq =get_request (q, rw, gfp_mask,NULL );return rq; }
@@ -2002,6 +2008,7 @@struct request *req, *freereq =NULL ;int el_ret, rw, nr_sectors, cur_nr_sectors, barrier, ra;sector_t sector; +int qfull; sector = bio->bi_sector; nr_sectors =bio_sectors (bio);
@@ -2023,6 +2030,7 @@ ra = bio->bi_rw & (1 <<BIO_RW_AHEAD ); again: + qfull =0 ;spin_lock_irq (q->queue_lock);if (elv_queue_empty (q)) {
@@ -2096,14 +2104,15 @@ freereq =NULL ; }else {spin_unlock_irq (q->queue_lock); -if ((freereq =get_request (q, rw,GFP_ATOMIC )) ==NULL ) { + freereq =get_request (q, rw,GFP_ATOMIC , ra ?NULL : &qfull); +if (freereq ==NULL ) {/* * READA bit set */ if (ra) goto end_io; - freereq =get_request_wait (q, rw); + freereq =get_request_wait (q, rw,1 ); } goto again; }
@@ -2141,6 +2150,13 @@ req->start_time = jiffies;add_request (q, req); + +if (qfull) { +spin_unlock_irq (q->queue_lock); +get_request_wait (q, rw,0 ); +spin_lock_irq (q->queue_lock); + } + out:if (freereq)__blk_put_request (q, freereq);
Quoted 6 lines · Miquel van Smoorenburg
The thing is, the bio's are submitted perfectly sequentially. It's just that
every so often a request (with just its initial bio) gets stuck for a while. Because the bio's after it are merged and sent to the device, it's not possible to merge that stuck request later on when it gets unstuck, because the other bio's have already left the building so to speak.
Oh. So the raid controller's queue depth is larger than the kernel's. So everything gets immediately shovelled into the device and the kernel is left with nothing to merge the little request against.
Shouldn't the controller itself be performing the insertion?
Quoted 11 lines · Andrew Morton
Quoted 6 lines · Miquel van Smoorenburg
The thing is, the bio's are submitted perfectly sequentially. It's just that
every so often a request (with just its initial bio) gets stuck for a while. Because the bio's after it are merged and sent to the device, it's not possible to merge that stuck request later on when it gets unstuck, because the other bio's have already left the building so to speak.Oh. So the raid controller's queue depth is larger than the kernel's. So everything gets immediately shovelled into the device and the kernel is left with nothing to merge the little request against.
Well, the request queue of the kernel is max'ed out too, otherwise get_request_wait() wouldn't be called. It's just an unfortunate timing issue.
Quoted 1 line
Shouldn't the controller itself be performing the insertion?
Well, you would indeed expect the 3ware hardware to be smarter than that, but in its defence, the driver doesn't set sdev->simple_tags or sdev->ordered_tags at all. It just has a large queue on the host, in hardware.
Perhaps this info should be exported into the request queue of the device, so that ll_rw_blk knows about this and can do something similar to the hack I posted ?
Note that AFAICS nothing in drivers/scsi uses the tagging stuff in ll_rw_blk.c. blk_queue_init_tags() is only called by scsi_activate_tcq(), and nothing ever calls that (except the 53c700.c driver). So you can't just check for QUEUE_FLAG_QUEUED. Hmm, nothing in drivers/block calls it either. It's not being used at all yet ? Or am I being dense ?
Mike.
Quoted 9 lines · Miquel van Smoorenburg
Quoted 8 lines · Andrew Morton
Quoted 6 lines · Miquel van Smoorenburg
The thing is, the bio's are submitted perfectly sequentially. It's just that
every so often a request (with just its initial bio) gets stuck for a while. Because the bio's after it are merged and sent to the device, it's not possible to merge that stuck request later on when it gets unstuck, because the other bio's have already left the building so to speak.
Quoted 7 lines
Quoted 3 lines
Oh. So the raid controller's queue depth is larger than the kernel's. So everything gets immediately shovelled into the device and the kernel is left with nothing to merge the little request against.
Well, the request queue of the kernel is max'ed out too, otherwise
get_request_wait()wouldn't be called. It's just an unfortunate timing issue.
Indeed
Quoted 6 lines
Quoted 1 line
Shouldn't the controller itself be performing the insertion?
Well, you would indeed expect the 3ware hardware to be smarter than that, but in its defence, the driver doesn't set
sdev->simple_tagsorsdev->ordered_tagsat all. It just has a large queue on the host, in hardware.
A too large queue. IMHO the simple and correct solution to your problem is to diminish the host queue (sane solution), or bump the block layer queue size (dumb solution).
Quoted 10 lines
Perhaps this info should be exported into the request queue of the device, so that
ll_rw_blkknows about this and can do something similar to the hack I posted ?Note that AFAICS nothing in
drivers/scsiuses the tagging stuff inll_rw_blk.c.blk_queue_init_tags()is only called byscsi_activate_tcq(), and nothing ever calls that (except the53c700.cdriver). So you can't just check forQUEUE_FLAG_QUEUED. Hmm, nothing indrivers/blockcalls it either. It's not being used at all yet ? Or am I being dense ?
No you are correct, I already outlined that to you explicitly in the very first mail in this thread. Hopefully this will change with 2.7 so we have some block layer control over tagging in general.
Signature · 1 line
Jens Axboe
Quoted 4 lines · Jens Axboe
Quoted 2 lines · Miquel van Smoorenburg
Quoted 1 line
Shouldn't the controller itself be performing the insertion?
Quoted 8 lines
Quoted 4 lines
Well, you would indeed expect the 3ware hardware to be smarter than that, but in its defence, the driver doesn't set
sdev->simple_tagsorsdev->ordered_tagsat all. It just has a large queue on the host, in hardware.A too large queue. IMHO the simple and correct solution to your problem is to diminish the host queue (sane solution), or bump the block layer queue size (dumb solution).
Well, I did that. Lowering the queue size of the 3ware controller to 64 does help a bit, but performance is still not optimal - leaving it at 254 and increasing the nr_requests of the queue to 512 helps the most.
But the patch I posted does just as well, without any tuning. I changed it a little though - it only has the "new" behaviour (instead of blocking on allocating a request, allocate it, queue it, _then_ block) for WRITEs. That results in the best performance I've seen, by far.
Now the style of my patch might be ugly, but what is conceptually wrong with allocating the request and queueing it, then block if the queue is full, versus blocking on allocating the request and keeping a bio "stuck" for quite some time, resulting in out-of-order requests to the hardware ?
Note that this is not an issue of '2 processes writing to 1 file', really. It's one process and pdflush writing the same dirty pages of the same file.
Mike.
Quoted 17 lines · Miquel van Smoorenburg
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Miquel van Smoorenburg
Quoted 2 lines
Shouldn't the controller itself be performing the insertion?
Quoted 9 lines
Quoted 5 lines
Well, you would indeed expect the 3ware hardware to be smarter than that, but in its defence, the driver doesn't set
sdev->simple_tagsorsdev->ordered_tagsat all. It just has a large queue on the host, in hardware.A too large queue. IMHO the simple and correct solution to your problem is to diminish the host queue (sane solution), or bump the block layer queue size (dumb solution).
Quoted 10 lines
Well, I did that. Lowering the queue size of the 3ware controller to 64 does help a bit, but performance is still not optimal - leaving it at 254 and increasing the
nr_requestsof the queue to 512 helps the most.But the patch I posted does just as well, without any tuning. I changed it a little though - it only has the "new" behaviour (instead of blocking on allocating a request, allocate it, queue it, _then_ block) for WRITEs. That results in the best performance I've seen, by far.
That's because you are half introducing per-process limits.
Quoted 7 lines
Now the style of my patch might be ugly, but what is conceptually wrong with allocating the request and queueing it, then block if the queue is full, versus blocking on allocating the request and keeping a bio "stuck" for quite some time, resulting in out-of-order requests to the hardware ?
Conceptually? The concept that you have everything you need to continue and yet you block anyway is wrong.
Quoted 4 lines
Note that this is not an issue of '2 processes writing to 1 file', really. It's one process and pdflush writing the same dirty pages of the same file.
pdflush is a process though, that's all that matters.
Quoted 42 lines · Nick Piggin
Quoted 17 lines · Miquel van Smoorenburg
Quoted 5 lines · Jens Axboe
Quoted 2 lines · Miquel van Smoorenburg
Quoted 2 lines
Shouldn't the controller itself be performing the insertion?
Quoted 9 lines
Quoted 5 lines
Well, you would indeed expect the 3ware hardware to be smarter than that, but in its defence, the driver doesn't set
sdev->simple_tagsorsdev->ordered_tagsat all. It just has a large queue on the host, in hardware.A too large queue. IMHO the simple and correct solution to your problem is to diminish the host queue (sane solution), or bump the block layer queue size (dumb solution).
Quoted 10 lines
Well, I did that. Lowering the queue size of the 3ware controller to 64 does help a bit, but performance is still not optimal - leaving it at 254 and increasing the
nr_requestsof the queue to 512 helps the most.But the patch I posted does just as well, without any tuning. I changed it a little though - it only has the "new" behaviour (instead of blocking on allocating a request, allocate it, queue it, _then_ block) for WRITEs. That results in the best performance I've seen, by far.
That's because you are half introducing per-process limits.
Quoted 7 lines
Now the style of my patch might be ugly, but what is conceptually wrong with allocating the request and queueing it, then block if the queue is full, versus blocking on allocating the request and keeping a bio "stuck" for quite some time, resulting in out-of-order requests to the hardware ?
Quoted 2 lines
Conceptually? The concept that you have everything you need to continue and yet you block anyway is wrong.
For reading, I agree. For writing .. ah well, English is not my first language, let's not argue about language semantics.
Quoted 4 lines
Quoted 2 lines
Note that this is not an issue of '2 processes writing to 1 file', really. It's one process and pdflush writing the same dirty pages of the same file.
pdflush is a process though, that's all that matters.
I understand that when the two processes are unrelated, the patch as I sent it will do the wrong thing.
But the thing is, you get this:
- "dd" process writes requests
- pdflush triggers to write dirty pages
- too many pages are dirty so "dd" blocks as well to write synchronously
- "dd" process triggers "queue full" but gets marked as "batching" so can continue (get_request)
- pdflush tries to submit one bio and gets blocked (get_request_wait)
- "dd" continues, but that one bio from pdflush remains stuck for a while
That's stupid, that one bio from pdflush should really be allowed on the queue, since "dd" is adding requests from the same source to it anyway.
Perhaps writes from pdflush should be handled differently to prevent this specific case ?
Say, if pdflush adds request #128, don't mark it as batching, but let it block. The next process will be the one marked as batching and can continue. If pdflush tries to add a request > 128, allow it, but _then_ block it.
Would something like that work ? Would it be a good idea to never mark a pdflush process as batching, or would that have a negative impact for some things ?
Mike.
Quoted 6 lines · Miquel van Smoorenburg
Quoted 3 lines · Nick Piggin
Miquel van Smoorenburg wrote:
...
Quoted 11 lines
Quoted 5 lines
Quoted 3 lines
Note that this is not an issue of '2 processes writing to 1 file', really. It's one process and pdflush writing the same dirty pages of the same file.
pdflush is a process though, that's all that matters.
I understand that when the two processes are unrelated, the patch as I sent it will do the wrong thing.
But the thing is, you get this:
Quoted 9 lines
- "dd" process writes requests
- pdflush triggers to write dirty pages
- too many pages are dirty so "dd" blocks as well to write synchronously
- "dd" process triggers "queue full" but gets marked as "batching" so can continue (get_request)
- pdflush tries to submit one bio and gets blocked (get_request_wait)
- "dd" continues, but that one bio from pdflush remains stuck for a while
The batching logic can probably all be ripped out with per process limits. It's too complex anyway really.
Quoted 5 lines
That's stupid, that one bio from pdflush should really be allowed on the queue, since "dd" is adding requests from the same source to it anyway.
But the whole reason it is getting blocked in the first place is because your controller is sucking up all your requests. The whole problem is not a problem if you use properly sized queues.
I'm a bit surprised that it wasn't working well with a controller queue depth of 64 and 128 nr_requests. I'll give you a per process request limit patch to try in a minute.
Quoted 13 lines
Perhaps writes from pdflush should be handled differently to prevent this specific case ?
Say, if pdflush adds request #128, don't mark it as batching, but let it block. The next process will be the one marked as batching and can continue. If pdflush tries to add a request > 128, allow it, but _then_ block it.
Would something like that work ? Would it be a good idea to never mark a pdflush process as batching, or would that have a negative impact for some things ?
It's hard to know. Maybe a better solution would be to allow pdflush to be exempt from the limits entirely as long as it tries not to write to congested queues (which is what it does)...
PATCH 1/1 per process request limits (was Re: IO scheduler, queue depth, nr_requests)
In reply to Miquel van SmoorenburgHi Miquel,
Can you see if this patch helps you?
Even with this patch, it might still be a good idea to allow pdflush to disregard the limits...
Nick
linux-2.6-npiggin/drivers/block/ll_rw_blk.c | 113 ++++++++-------------------- linux-2.6-npiggin/include/linux/blkdev.h | 8 - 2 files changed, 36 insertions(+), 85 deletions(-)
include/linux/blkdev.h renamed from include/linux/blkdev.h~blk-per-process-rqlim +3 -5
@@ -25,6 +25,7 @@ struct request_pm_state;#define BLKDEV_MIN_RQ 4#define BLKDEV_MAX_RQ 128/* Default maximum */ +#define BLKDEV_MIN_PROC_RQ 64/* * This is the per-process anticipatory I/O scheduler state.
@@ -61,11 +62,7 @@ struct io_context {atomic_t refcount;pid_t pid; -/* -* For request batching -*/ -unsigned long last_waited;/* Time last woken after wait for request */ -int nr_batch_requests;/* Number of requests left in the batch */ +unsigned int nr_requests;/* Outstanding requests */ struct as_io_context *aic; };
@@ -141,6 +138,7 @@ struct request {int ref_count;request_queue_t *q;struct request_list *rl; +struct io_context *ioc;struct completion *waiting;void *special;
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~blk-per-process-rqlim +33 -80
214 lines not on this page · raw message
_
Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests)
Quoted 3 lines · Nick Piggin
Even with this patch, it might still be a good idea to allow pdflush to disregard the limits...
Has it been confirmed that pdflush is blocking in get_request_wait()? I guess that can happen very occasionally because we don't bother with any locking around there but if it's happening a lot then something is bust.
Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests)
Quoted 11 lines · Andrew Morton
Quoted 3 lines · Nick Piggin
Even with this patch, it might still be a good idea to allow pdflush to disregard the limits...
Has it been confirmed that pdflush is blocking in
get_request_wait()? I guess that can happen very occasionally because we don't bother with any locking around there but if it's happening a lot then something is bust.
Miquel's analysis is pretty plausible, but I'm not sure if he's confirmed it or not, Miquel? Even if it isn't happening a lot, and something isn't bust it might be a good idea to do this.
Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests)
Quoted 17 lines · Nick Piggin
Quoted 11 lines · Andrew Morton
Quoted 3 lines · Nick Piggin
Even with this patch, it might still be a good idea to allow pdflush to disregard the limits...
Has it been confirmed that pdflush is blocking in
get_request_wait()? I guess that can happen very occasionally because we don't bother with any locking around there but if it's happening a lot then something is bust.
Quoted 4 lines
Miquel's analysis is pretty plausible, but I'm not sure if he's confirmed it or not, Miquel? Even if it isn't happening a lot, and something isn't bust it might be a good idea to do this.
Seems OK from a quick check. pdflush will block in get_request_wait() occasionally, but not at all often. Perhaps we could move the write_congested test into the mpage_writepages() inner loop but it hardly seems worth the risk.
Maybe things are different on Miquel's clockwork controller.
drivers/block/ll_rw_blk.c | 2 ++ fs/fs-writeback.c | 2 ++ 2 files changed, 4 insertions(+)
drivers/block/ll_rw_blk.c renamed from drivers/block/ll_rw_blk.c~pdflush-blockage-check +2 -0
@@ -1651,6 +1651,8 @@ static struct request *get_request_wait(if (!rq) {struct io_context *ioc; +WARN_ON (current_is_pdflush ()); +io_schedule ();/*
fs/fs-writeback.c renamed from fs/fs-writeback.c~pdflush-blockage-check +2 -0
@@ -279,6 +279,8 @@ sync_sb_inodes(struct super_block *sb, s }if (wbc->nonblocking &&bdi_write_congested (bdi)) { +if (current_is_pdflush ()) +printk ("saved pdflush\n" ); wbc->encountered_congestion =1 ;if (sb != blockdev_superblock)break ;/* Skip a congested fs */
_
PATCH 1/1 bdi_congestion_funp (was: Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests))
Quoted 12 lines · Andrew Morton
Quoted 11 lines · Nick Piggin
Quoted 6 lines · Andrew Morton
Quoted 3 lines · Nick Piggin
Even with this patch, it might still be a good idea to allow pdflush to disregard the limits...
Quoted 8 lines
Quoted 8 lines
Quoted 5 lines
Has it been confirmed that pdflush is blocking in
get_request_wait()? I guess that can happen very occasionally because we don't bother with any locking around there but if it's happening a lot then something is bust.Miquel's analysis is pretty plausible, but I'm not sure if he's confirmed it or not, Miquel?
Yes, I've added current->comm to my debug printk's, and it's dd/pdflush that both block in get_request_wait (alternating between the two).
Quoted 10 lines
Quoted 3 lines
Even if it isn't happening
a lot, and something isn't bust it might be a good idea to do this.Seems OK from a quick check. pdflush will block in
get_request_wait()occasionally, but not at all often. Perhaps we could move thewrite_congestedtest into thempage_writepages()inner loop but it hardly seems worth the risk.Maybe things are different on Miquel's clockwork controller.
I haven't tested it yet because of the "This patch isn't actually so good" comment, but I found another explanation.
Quoted 3 lines
drivers/block/ll_rw_blk.c | 2 ++ fs/fs-writeback.c | 2 ++ 2 files changed, 4 insertions(+)
*Lightbulb on* .. I just read fs-writeback.c. As I said, this happens with an LVM device. Could it be that because LVM and the actual device have different struct request_queue's things go awry ?
In fs-writeback.c, your're looking at the LVM device (and its request_queue, and its backing_dev_info). In__make_request, you're looking at the SCSI device.
So that's why I saw "dd" and pdflush fighting over get_request. pdflush isn't supposed to run when the blockdev is congested, but ofcourse the LVM device is never marked as congested.
The same can happen with MD devices, I think. Here's another proof-of-concept patch that fixes things for LVM. With this patch applied, I never see pdflush running when the queue is congested. You'll probably mark it as "horrible", but hey, it shows the problem clear enough..
This patch adds a function pointer to the backing_dev_info struct that can be called to check on the congestion if it's anything other than a simple device. Dm.c now sets this pointer to a function defined in dm-table.c that checks the congestion bits on all devices part of the logical volume.
I'm not sure if it should be the other way around (ll_rw_blk setting the congested bit in the LVM device instead). Also if any queue of any device under LVM is congested, pdflush won't run on the whole device - but I think that harms less than running on a device with a congested queue.
bdi-congestion-funp.patch
include/linux/backing-dev.h renamed from include/linux/backing-dev.h.ORIG +6 -0
@@ -24,6 +24,8 @@unsigned long ra_pages;/* max readahead in PAGE_CACHE_SIZE units */ unsigned long state;/* Always use atomic bitops on this */ int memory_backed;/* Cannot clean pages with writepage */ +int (*congested)(int ,void *);/* Function pointer if device is md/dm */ +void *aux;/* Pointer to aux data for congested func */ };extern struct backing_dev_info default_backing_dev_info;
@@ -34,11 +36,15 @@static inline int bdi_read_congested (struct backing_dev_info *bdi) { +if (bdi->congested) +return bdi->congested (BDI_read_congested, bdi->aux);return test_bit (BDI_read_congested, &bdi->state); }static inline int bdi_write_congested (struct backing_dev_info *bdi) { +if (bdi->congested) +return bdi->congested (BDI_write_congested, bdi->aux);return test_bit (BDI_write_congested, &bdi->state); }
drivers/md/dm.h renamed from drivers/md/dm.h.ORIG +1 -0
@@ -115,6 +115,7 @@int dm_table_get_mode (struct dm_table *t);void dm_table_suspend_targets (struct dm_table *t);void dm_table_resume_targets (struct dm_table *t); +int dm_table_any_congested (int bdi_state,void *aux);/*----------------------------------------------------------------- * A registry of target types.
drivers/md/dm-table.c renamed from drivers/md/dm-table.c.ORIG +24 -0
@@ -857,6 +857,30 @@ } } +/* +* See if any device in the logical volume has a queue +* that is congested - in that case, pdflush skips it. +*/ +int dm_table_any_congested (int bdi_state,void *aux) +{ +struct mapped_device *md = aux; +struct dm_table *t; +struct list_head *d, *devices; +int r =0 ; + +if ((t =dm_get_table (md)) ==NULL ) +return 0 ; + + devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *q =bdev_get_queue (dd->bdev); + r |=test_bit (bdi_state, &(q->backing_dev_info.state)); + } +dm_table_put (t); + +return r; +}EXPORT_SYMBOL (dm_get_device);EXPORT_SYMBOL (dm_put_device);
drivers/md/dm.c renamed from drivers/md/dm.c.ORIG +2 -0
@@ -608,6 +608,8 @@ } md->queue->queuedata = md; + md->queue->backing_dev_info.congested = dm_table_any_congested; + md->queue->backing_dev_info.aux = md;blk_queue_make_request (md->queue, dm_request); md->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
Re: [PATCH] bdi_congestion_funp (was: Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests))
Quoted 2 lines · Miquel van Smoorenburg
drivers/md/dm-table.c renamed from drivers/md/dm-table.c.ORIG +0
-0
<snip>
drivers/md/dm-table.c line 872
Quoted 2 lines
drivers/md/dm-table.c renamed from drivers/md/dm-table.c.ORIG +2
-0+if ((t =dm_get_table (md)) ==NULL ) +return 0 ;
struct mapped_device has no business in this file. You should move this function to dm.c, and provide accessor fns in dm-table.c.
drivers/md/dm-table.c line 878
Quoted 5 lines
drivers/md/dm-table.c renamed from drivers/md/dm-table.c.ORIG +5
-0+ devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *q =bdev_get_queue (dd->bdev); + r |=test_bit (bdi_state, &(q->backing_dev_info.state));
Shouldn't this be calling your bdi_*_congested function rather than assuming it is a real device under dm ? (often not true).
I'm also very slightly worried that or'ing together the congestion results for all the seperate devices isn't always the right thing. These devices include anything that the targets are using, exception stores for snapshots, logs for mirror, all paths for multipath (or'ing is most likely to be wrong for multipath).
- Joe
Re: [PATCH] bdi_congestion_funp (was: Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests))
Quoted 9 lines · Joe Thornber
drivers/md/dm-table.c line 878
Quoted 5 lines
drivers/md/dm-table.c renamed from drivers/md/dm-table.c.ORIG +5
-0+ devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *q =bdev_get_queue (dd->bdev); + r |=test_bit (bdi_state, &(q->backing_dev_info.state));Shouldn't this be calling your bdi_*_congested function rather than assuming it is a real device under dm ? (often not true).
Quoted 5 lines
I'm also very slightly worried that or'ing together the congestion results for all the seperate devices isn't always the right thing. These devices include anything that the targets are using, exception stores for snapshots, logs for mirror, all paths for multipath (or'ing is most likely to be wrong for multipath).
Yeah the patch is pretty much crap in that area, I don't think Miquel was aiming for inclusion :)
I'd suggest making queue functions for congestion state as well so it stacks properly.
Signature · 1 line
Jens Axboe
Re: [PATCH] bdi_congestion_funp (was: Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests))
Quoted 16 lines · Jens Axboe
Quoted 9 lines · Joe Thornber
drivers/md/dm-table.c line 878
Quoted 5 lines
drivers/md/dm-table.c renamed from drivers/md/dm-table.c.ORIG +5
-0+ devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *q =bdev_get_queue (dd->bdev); + r |=test_bit (bdi_state, &(q->backing_dev_info.state));Shouldn't this be calling your bdi_*_congested function rather than assuming it is a real device under dm ? (often not true).
Quoted 5 lines
I'm also very slightly worried that or'ing together the congestion results for all the seperate devices isn't always the right thing. These devices include anything that the targets are using, exception stores for snapshots, logs for mirror, all paths for multipath (or'ing is most likely to be wrong for multipath).
Quoted 5 lines
Yeah the patch is pretty much crap in that area, I don't think Miquel was aiming for inclusion :)
I'd suggest making queue functions for congestion state as well so it stacks properly.
I've been looking at this. If you want to keep the struct backing_dev_info reasonably stand-alone (no function pointer and aux data like I did) you probably need to add a list of parent_queues to every request_list. If the queue get congested, you mark the parent queue(s) congested as well. Congested state would need to be changed into a counter instead of a bitfield, I think.
The pdflush-is-running-on-this-queue bit can probably remain as-is. It's mostly meant to prevent 2 pdflush daemons from running the same queue. I don't see much harm in pdflush #1 running on /dev/md0 which consists of /dev/sda1 and /dev/sdb1 and pdflush #2 running on /dev/sdb2, right ?
Would that be the way to go? If so, I'll take a stab at it.
Mike.
Re: [PATCH] bdi_congestion_funp (was: Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests))
Quoted 5 lines · Miquel van Smoorenburg
The pdflush-is-running-on-this-queue bit can probably remain as-is. It's
mostly meant to prevent 2 pdflush daemons from running the same queue. I don't see much harm in pdflush #1 running on/dev/md0which consists of/dev/sda1and/dev/sdb1and pdflush #2 running on/dev/sdb2, right ?
Yes, having two pdflushes working the same spindle is a bit pointless but is presumably fairly harmless.
The most important thing here is to prevent pdflush from blocking on request exhaustion. Because while pdflush sleeps on a particular disk, all the others remain unserviced.
Re: [PATCH] bdi_congestion_funp (was: Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests))
In reply to Miquel van SmoorenburgQuoted 18 lines · Miquel van Smoorenburg
Quoted 10 lines
Quoted 3 lines
Even if it isn't happening
a lot, and something isn't bust it might be a good idea to do this.Seems OK from a quick check. pdflush will block in
get_request_wait()occasionally, but not at all often. Perhaps we could move thewrite_congestedtest into thempage_writepages()inner loop but it hardly seems worth the risk.Maybe things are different on Miquel's clockwork controller.
I haven't tested it yet because of the "This patch isn't actually so good" comment, but I found another explanation.
Quoted 3 lines
drivers/block/ll_rw_blk.c | 2 ++ fs/fs-writeback.c | 2 ++ 2 files changed, 4 insertions(+)
Quoted 7 lines
*Lightbulb on* .. I just read
fs-writeback.c. As I said, this happens with an LVM device. Could it be that because LVM and the actual device have different struct request_queue's things go awry ?In
fs-writeback.c, your're looking at the LVM device (and itsrequest_queue, and itsbacking_dev_info).In__make_request, you're looking at the SCSI device.
In principle, the lvm/md queues themselves will never be congested. But the underlying queues can be, of course.
Now this approach is _much_ better, imo. I don't particularly care very much for how you solved it, though, I'd much rather just see both setting and testing passed down (and kill the ->aux as well).
Regardless of the initial hw depth vs block depth (which is also a generic device problem, not just dm related), this would be a good addition to the congestion logic.
Signature · 1 line
Jens Axboe
[linux-lvm] Queue congestion: passing down vs passing up [PATCH]
[weeded out the enormous Cc: list]
Quoted 27 lines · Jens Axboe
Quoted 18 lines · Miquel van Smoorenburg
Quoted 10 lines
Quoted 3 lines
Even if it isn't happening
a lot, and something isn't bust it might be a good idea to do this.Seems OK from a quick check. pdflush will block in
get_request_wait()occasionally, but not at all often. Perhaps we could move thewrite_congestedtest into thempage_writepages()inner loop but it hardly seems worth the risk.Maybe things are different on Miquel's clockwork controller.
I haven't tested it yet because of the "This patch isn't actually so good" comment, but I found another explanation.
Quoted 3 lines
drivers/block/ll_rw_blk.c | 2 ++ fs/fs-writeback.c | 2 ++ 2 files changed, 4 insertions(+)Quoted 7 lines
*Lightbulb on* .. I just read
fs-writeback.c. As I said, this happens with an LVM device. Could it be that because LVM and the actual device have different struct request_queue's things go awry ?In
fs-writeback.c, your're looking at the LVM device (and itsrequest_queue, and itsbacking_dev_info).In__make_request, you're looking at the SCSI device.
Quoted 6 lines
In principle, the lvm/md queues themselves will never be congested. But the underlying queues can be, of course.
Now this approach is _much_ better, imo. I don't particularly care very much for how you solved it, though, I'd much rather just see both setting and testing passed down (and kill the ->aux as well).
I just sent a patch to linux-lvm / Joe Thornber that does this. It adds a void *request_queue to the struct backing_dev_info, and a congestion_fn to the request_queue_t struct. bdi_{read,write}_congested got moved to ll_rw_blk.c. dm.c sets the congestion_fn to an internal function that checks the congestion of all the underlying block devices.
See https://www.redhat.com/archives/linux-lvm/2004-February/msg00203.html
for the actual patch.
So that's testing being passed down.
The setting you cannot pass down - you need to pass it "up". I implemented that too, added a list of "parent queues" to the request_queue, and a function so that for example dm can register its queue as "parent" of an underlying device.
It needs to be a list, since different partitions on one device can be members of different meta-devices.
When the congestion functions in ll_rw_blk.c are called, if a queue is congested, that info is also passed "up" to all of the "parent" queues. backing_dev_info uses a counter instead of a bit to mark read/write as congested. Perhaps it would be better to call a function (congestion_fn).
I think that we only need one of the two - either testing being passed down or setting being passed up. The first one is the easiest. The second one keeps the struct backing_dev_info clean (no request_queue pointer).
What would you prefer ?
For reference, a first cut at the "passing up queue congestion" is below.
Mike.
queue-parent.patch
include/linux/backing-dev.h +4 -2
@@ -24,6 +24,8 @@unsigned long ra_pages;/* max readahead in PAGE_CACHE_SIZE units */ unsigned long state;/* Always use atomic bitops on this */ int memory_backed;/* Cannot clean pages with writepage */ +int read_congested;/* Write queue or child write queue congested */ +int write_congested;/* Read queue or child read queue congested */ };extern struct backing_dev_info default_backing_dev_info;
@@ -34,12 +36,12 @@static inline int bdi_read_congested (struct backing_dev_info *bdi) { -return test_bit (BDI_read_congested, &bdi->state); +return bdi->read_congested; }static inline int bdi_write_congested (struct backing_dev_info *bdi) { -return test_bit (BDI_write_congested, &bdi->state); +return bdi->write_congested; }#endif /* _LINUX_BACKING_DEV_H */
include/linux/blkdev.h +9 -0
@@ -267,6 +267,11 @@atomic_t refcnt;/* map can be shared */ }; +struct rqueue_list { +struct list_head rqueue_list_head; +struct request_queue *rqueue; +}; +struct request_queue {/*
@@ -301,6 +306,8 @@struct backing_dev_info backing_dev_info; +struct list_head parents; +/* * The queue owner gets to use this for whatever they like. * ll_rw_blk doesn't touch it.
@@ -514,6 +521,8 @@extern void __blk_stop_queue (request_queue_t *q);extern void blk_run_queue (request_queue_t *q);extern void blk_queue_activity_fn (request_queue_t *,activity_fn *,void *); +extern int blk_queue_add_parent (request_queue_t *q,request_queue_t *parent); +extern void blk_queue_remove_parent (request_queue_t *q,request_queue_t *parent);static inline request_queue_t *bdev_get_queue (struct block_device *bdev) {
drivers/block/ll_rw_blk.c +105 -8
157 lines not on this page · raw message
drivers/md/dm.h +2 -0
@@ -110,6 +110,8 @@struct dm_target *dm_table_get_target (struct dm_table *t,unsigned int index);struct dm_target *dm_table_find_target (struct dm_table *t,sector_t sector);void dm_table_set_restrictions (struct dm_table *t,struct request_queue *q); +void dm_table_add_parent (struct dm_table *t,struct request_queue *q); +void dm_table_remove_parent (struct dm_table *t,struct request_queue *q);unsigned int dm_table_get_num_targets (struct dm_table *t);struct list_head *dm_table_get_devices (struct dm_table *t);int dm_table_get_mode (struct dm_table *t);
drivers/md/dm-table.c +24 -0
@@ -818,6 +818,30 @@ q->seg_boundary_mask = t->limits.seg_boundary_mask; } +void dm_table_add_parent (struct dm_table *t,struct request_queue *q) +{ +struct list_head *d, *devices; + + devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *bq =bdev_get_queue (dd->bdev); +blk_queue_add_parent (bq, q); + } +} + +void dm_table_remove_parent (struct dm_table *t,struct request_queue *q) +{ +struct list_head *d, *devices; + + devices =dm_table_get_devices (t); +for (d = devices->next; d != devices; d = d->next) { + struct dm_dev *dd = list_entry(d, struct dm_dev, list); +request_queue_t *bq =bdev_get_queue (dd->bdev); +blk_queue_remove_parent (bq, q); + } +} +unsigned int dm_table_get_num_targets (struct dm_table *t) {return t->num_targets;
drivers/md/dm.c +5 -1
@@ -41,6 +41,7 @@struct mapped_device {struct rw_semaphore lock;atomic_t holders; +spinlock_t queue_lock;unsigned long flags;
@@ -600,7 +601,7 @@memset (md,0 ,sizeof (*md));init_rwsem (&md->lock);atomic_set (&md->holders,1 ); - + md->queue_lock =SPIN_LOCK_UNLOCKED ; md->queue =blk_alloc_queue (GFP_KERNEL );if (!md->queue) {kfree (md);
@@ -608,6 +609,7 @@ } md->queue->queuedata = md; + md->queue->queue_lock = &md->queue_lock;blk_queue_make_request (md->queue, dm_request); md->io_pool = mempool_create(MIN_IOS, mempool_alloc_slab,
@@ -695,6 +697,7 @@dm_table_get (t);dm_table_set_restrictions (t, q); +dm_table_add_parent (t, q);return 0 ; }
@@ -704,6 +707,7 @@return ;dm_table_event_callback (md->map,NULL ,NULL ); +dm_table_remove_parent (md->map, md->queue);dm_table_put (md->map); md->map =NULL ; }
Re: [PATCH] per process request limits (was Re: IO scheduler, queue depth, nr_requests)
In reply to Nick PigginQuoted 3 lines · Nick Piggin
Hi Miquel,
Can you see if this patch helps you?
This patch isn't actually so good because it doesn't wake a specific process when it gets below it's limit...
Quoted 9 lines · Miquel van Smoorenburg
No, I'm actually referring to a struct request. I'm logging this in the SCSI layer, in
scsi_request_fn(), just afterelv_next_request(). I have in fact logged all the bio's submitted to__make_request, and the output of the elevator fromelv_next_request(). The bio's are submitted sequentially, the resulting requests aren't. But this is becausenr_requestsis 128, while the 3ware device has a queue of 254 entries (no tagging though). Uppingnr_requeststo 512 makes this go away ..
Quoted 6 lines
That shouldn't be necessary though. I only see this with LVM over 3ware-raid5, not on the 3ware-raid5 array directly (
/dev/sda1). And it gets less troublesome with a lot of debugging (unless I setnr_requestslower again), which points to a timing issue.
So the problem you are seeing is due to "unlucky" timing between two processes submitting IO. And the very efficient mechanisms (merging, sorting) we have to improve situations exactly like this is effectively disabled. And to make it worse, it appears that your controller shits itself on this trivially simple pattern.
Your hack makes a baby step in the direction of per *process* request limits, which I happen to be an advocate of. As it stands though, I don't like it.
Jens has the final say when it comes to the block layer though.
Quoted 31 lines · Nick Piggin
Quoted 11 lines · Miquel van Smoorenburg
No, I'm actually referring to a struct request. I'm logging this in the SCSI layer, in
scsi_request_fn(), just afterelv_next_request(). I have in fact logged all the bio's submitted to__make_request, and the output of the elevator fromelv_next_request(). The bio's are submitted sequentially,
the resulting requests aren't. But this is becausenr_requestsis 128, while
the 3ware device has a queue of 254 entries (no tagging though). Uppingnr_requeststo 512 makes this go away ..Quoted 9 lines
That shouldn't be necessary though. I only see this with LVM over 3ware-raid5,
not on the 3ware-raid5 array directly (/dev/sda1). And it gets less troublesome
with a lot of debugging (unless I setnr_requestslower again), which points
to a timing issue.So the problem you are seeing is due to "unlucky" timing between two processes submitting IO. And the very efficient mechanisms (merging, sorting) we have to improve situations exactly like this is effectively disabled. And to make it worse, it appears that your controller shits itself on this trivially simple pattern.
Quoted 3 lines
Your hack makes a baby step in the direction of per *process* request limits, which I happen to be an advocate of. As it stands though, I don't like it.
I'm very much an advocate for per process request limits as well. Would be trivial to add... Miquels patch is horrible, I appreciate it being posted as a cry for help.
Signature · 1 line
Jens Axboe
Quoted 39 lines · Miquel van Smoorenburg
Quoted 4 lines · Miquel van Smoorenburg
For some reason, when using LVM, write requests get queued out of order to the 3ware controller, which results in quite a bit of seeking and thus performance loss.
[..]
Quoted 18 lines
Okay I repeated some earlier tests, and I added some debug code in several places.
I added logging to
tw_scsi_queue()in the 3ware driver to log the start sector and length of each request. It logs something like:
3wdbg: id 119, lba =0x2330bc33,num_sectors= 256With a perl script, I can check if the requests are sent to the host in order. That outputs something like this:
Consecutive: start 1180906348, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180906340, length 8 sec (4 KB), requests: 1
Consecutive: start 1180914292, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180914284, length 8 sec (4 KB), requests: 1
Consecutive: start 1180922236, length 7936 sec (3968 KB), requests: 31
Consecutive: start 1180922228, length 8 sec (4 KB), requests: 1
Consecutive: start 1180930180, length 7936 sec (3968 KB), requests: 31Quoted 2 lines
See, 31 requests in order, then one request "backwards", then 31 in order, etc.
I found out what causes this. It's
get_request_wait().When the request queue is full, and a new request needs to be created,
__make_request()blocks inget_request_wait().Another process wakes up first (pdflush / process submitting I/O itself / xfsdatad / etc) and sends the next bio's to
__make_request(). In the mean time some free requests have become available, and the bios are merged into a new request. Those requests are submitted to the device.
Quoted 8 lines
Then,
get_request_wait()returns but the bio is not mergeable anymore - and that results in a backwards seek, severely limiting the I/O rate.Wouldn't it be better to allow the request allocation and queue the request, and /then/ put the process to sleep ? The queue will grow larger than
nr_requests, but it does that anyway.
The "batching" logic there should allow a process to submit a number of requests even above the nr_requests limit to prevent this interleave and context switching.
Are you using tagged command queueing? What depth?
Quoted 16 lines · Nick Piggin
Quoted 5 lines · Miquel van Smoorenburg
I found out what causes this. It's
get_request_wait().When the request queue is full, and a new request needs to be created,
__make_request()blocks inget_request_wait().Quoted 8 lines
Another process wakes up first (pdflush / process submitting I/O itself / xfsdatad / etc) and sends the next bio's to
__make_request(). In the mean time some free requests have become available, and the bios are merged into a new request. Those requests are submitted to the device.Then,
get_request_wait()returns but the bio is not mergeable anymore - and that results in a backwards seek, severely limiting the I/O rate.
Quoted 5 lines
The "batching" logic there should allow a process to submit a number of requests even above the
nr_requestslimit to prevent this interleave and context switching.Are you using tagged command queueing? What depth?
No, I'm not using tagged command queueing. The 3ware controller is not a real scsi controller, the driver just emulates one. It's a raid5 controller that drives SATA disks. It has an internal request queue ("can_queu") of 254 outstanding commands. Because that is way bigger than nr_requests this happens - if I set nr_requests to 512, the problem goes away. But that shouldn't happen ;)
I'm preparing a proof-of-concept patch now, if it works and I don't wedge the remote machine I'm testing this on I'll post it in a few minutes.
Mike.
Quoted 3 lines
Quoted 23 lines · Miquel van Smoorenburg
Quoted 21 lines · Nick Piggin
Quoted 5 lines · Miquel van Smoorenburg
I found out what causes this. It's
get_request_wait().When the request queue is full, and a new request needs to be created,
__make_request()blocks inget_request_wait().Quoted 9 lines
Another process wakes up first (pdflush / process submitting I/O itself / xfsdatad / etc) and sends the next bio's to
__make_request(). In the mean time some free requests have become available, and the bios are merged into a new request. Those requests are submitted to the device.Then,
get_request_wait()returns but the bio is not mergeable anymore - and that results in a backwards seek, severely limiting the I/O rate.The "batching" logic there should allow a process to submit a number of requests even above the
nr_requestslimit to prevent this interleave and context switching.
Quoted 8 lines
Quoted 2 lines
Are you using tagged command queueing? What depth?
No, I'm not using tagged command queueing. The 3ware controller is not a real scsi controller, the driver just emulates one. It's a raid5 controller that drives SATA disks. It has an internal request queue ("
can_queu") of 254 outstanding commands.
This is what I mean by tagged command queueing.
Quoted 5 lines
Because that is way bigger than
nr_requests
this happens - if I setnr_requeststo 512, the problem goes away. But that shouldn't happen ;)
What shouldn't happen?
Quoted 4 lines
I'm preparing a proof-of-concept patch now, if it works and I don't wedge the remote machine I'm testing this on I'll post it in a few minutes.
I'm not very happy with forcing a process to sleep _after_ it has submitted a request... but I'd be interested to see exactly what your patch does.
By far the best option is to use appropriately sized queues. The below patch is a start, but it unfortunately doesn't help drivers which use private queueing implementations.