Jens Axboe wrote:
On Mon, Mar 28 2005, Chen, Kenneth W wrote:
This patch was posted last year and if I remember correctly, Jens said
he is OK with the patch. In function __generic_unplug_deivce(), kernel
can use a cheaper function elv_queue_empty() instead of more expensive
elv_next_request to find whether the queue is empty or not. blk_run_queue
can also made conditional on whether queue's emptiness before calling
request_fn().
Signed-off-by: Ken Chen <[EMAIL PROTECTED]>
Looks good, thanks.
Signed-off-by: Jens Axboe <[EMAIL PROTECTED]>
Speaking of which, I've had a few ideas lying around for possible
performance improvement in the block code.
I haven't used a big disk array (or tried any simulation), but I'll
attach the patch if you're looking into that area.
It puts in a few unlikely()s, but the main changes are:
- don't generic_unplug_device unconditionally in get_request_wait,
- removes the relock/retry merge mechanism in __make_request if we
aren't able to get the GFP_ATOMIC allocation. Just fall through
and assume the chances of getting a merge will be small (is this
a valid assumption? Should measure it I guess).
- removes the GFP_ATOMIC allocation. That's always a good thing.
---
linux-2.6-npiggin/drivers/block/ll_rw_blk.c | 63 ++++++++++------------------
1 files changed, 23 insertions(+), 40 deletions(-)
diff -puN drivers/block/ll_rw_blk.c~blk-efficient drivers/block/ll_rw_blk.c
--- linux-2.6/drivers/block/ll_rw_blk.c~blk-efficient 2005-03-29
19:00:07.000000000 +1000
+++ linux-2.6-npiggin/drivers/block/ll_rw_blk.c 2005-03-29 19:10:45.000000000
+1000
@@ -1450,7 +1450,7 @@ EXPORT_SYMBOL(blk_remove_plug);
*/
void __generic_unplug_device(request_queue_t *q)
{
- if (test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags))
+ if (unlikely(test_bit(QUEUE_FLAG_STOPPED, &q->queue_flags)))
return;
if (!blk_remove_plug(q))
@@ -1955,7 +1955,6 @@ static struct request *get_request_wait(
DEFINE_WAIT(wait);
struct request *rq;
- generic_unplug_device(q);
do {
struct request_list *rl = &q->rq;
@@ -1967,6 +1966,7 @@ static struct request *get_request_wait(
if (!rq) {
struct io_context *ioc;
+ generic_unplug_device(q);
io_schedule();
/*
@@ -2557,7 +2557,7 @@ EXPORT_SYMBOL(__blk_attempt_remerge);
static int __make_request(request_queue_t *q, struct bio *bio)
{
- struct request *req, *freereq = NULL;
+ struct request *req;
int el_ret, rw, nr_sectors, cur_nr_sectors, barrier, err;
sector_t sector;
@@ -2577,19 +2577,18 @@ static int __make_request(request_queue_
spin_lock_prefetch(q->queue_lock);
barrier = bio_barrier(bio);
- if (barrier && (q->ordered == QUEUE_ORDERED_NONE)) {
+ if (unlikely(barrier) && (q->ordered == QUEUE_ORDERED_NONE)) {
err = -EOPNOTSUPP;
goto end_io;
}
-again:
spin_lock_irq(q->queue_lock);
if (elv_queue_empty(q)) {
blk_plug_device(q);
goto get_rq;
}
- if (barrier)
+ if (unlikely(barrier))
goto get_rq;
el_ret = elv_merge(q, &req, bio);
@@ -2632,40 +2631,23 @@ again:
elv_merged_request(q, req);
goto out;
- /*
- * elevator says don't/can't merge. get new request
- */
- case ELEVATOR_NO_MERGE:
- break;
-
+ /* ELV_NO_MERGE: elevator says don't/can't merge. */
default:
- printk("elevator returned crap (%d)\n", el_ret);
- BUG();
+ ;
}
+get_rq:
/*
- * Grab a free request from the freelist - if that is empty, check
- * if we are doing read ahead and abort instead of blocking for
- * a free slot.
+ * Grab a free request. This is might sleep but can not fail.
+ */
+ spin_unlock_irq(q->queue_lock);
+ req = get_request_wait(q, rw);
+ /*
+ * After dropping the lock and possibly sleeping here, our request
+ * may now be mergeable after it had proven unmergeable (above).
+ * We don't worry about that case for efficiency. It won't happen
+ * often, and the elevators are able to handle it.
*/
-get_rq:
- if (freereq) {
- req = freereq;
- freereq = NULL;
- } else {
- spin_unlock_irq(q->queue_lock);
- if ((freereq = get_request(q, rw, GFP_ATOMIC)) == NULL) {
- /*
- * READA bit set
- */
- err = -EWOULDBLOCK;
- if (bio_rw_ahead(bio))
- goto end_io;
-
- freereq = get_request_wait(q, rw);
- }
- goto again;
- }
req->flags |= REQ_CMD;
@@ -2678,7 +2660,7 @@ get_rq:
/*
* REQ_BARRIER implies no merging, but lets make it explicit
*/
- if (barrier)
+ if (unlikely(barrier))
req->flags |= (REQ_HARDBARRIER | REQ_NOMERGE);
req->errors = 0;
@@ -2693,10 +2675,11 @@ get_rq:
req->rq_disk = bio->bi_bdev->bd_disk;
req->start_time = jiffies;
+ spin_lock_irq(q->queue_lock);
+ if (elv_queue_empty(q))
+ blk_plug_device(q);
add_request(q, req);
out:
- if (freereq)
- __blk_put_request(q, freereq);
if (bio_sync(bio))
__generic_unplug_device(q);
@@ -2802,7 +2785,7 @@ static inline void block_wait_queue_runn
{
DEFINE_WAIT(wait);
- while (test_bit(QUEUE_FLAG_DRAIN, &q->queue_flags)) {
+ while (unlikely(test_bit(QUEUE_FLAG_DRAIN, &q->queue_flags))) {
struct request_list *rl = &q->rq;
prepare_to_wait_exclusive(&rl->drain, &wait,
@@ -2911,7 +2894,7 @@ end_io:
goto end_io;
}
- if (test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))
+ if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
goto end_io;
block_wait_queue_running(q);
_
