Re: performance "regression" in cfq compared to anticipatory, deadline and noop

[Matthew <jackdachef@gmail.com>]

On Fri, May 16, 2008 at 10:57 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
> On Fri, May 16 2008, Daniel J Blueman wrote:
>> On Fri, May 16, 2008 at 8:57 AM, Jens Axboe <jens.axboe@oracle.com> wrote:
>> > On Fri, May 16 2008, Jens Axboe wrote:
>> >> On Fri, May 16 2008, Fabio Checconi wrote:
>> >> > > From: Jens Axboe <jens.axboe@oracle.com>
>> >> > > Date: Fri, May 16, 2008 08:40:03AM +0200
>> >> > >
>> >> > ...
>> >> > > I think we can improve this further without getting too involved. If a
>> >> > > 2nd request is seen in cfq_rq_enqueued(), then DO schedule a dispatch
>> >> > > since this likely means that we wont be doing more merges on the first
>> >> > > one.
>> >> > >
>> >> >
>> >> > But isn't there the risk that even the second request would be
>> >> > dispatched, while it still could have grown?
>> >>
>> >> Certainly, you'd only want to dispatch the first request. Ideally we'd
>> >> just get rid of this logic of 'did empty dispatch round' and only
>> >> dispatch requests once merging is done, it's basically the wrong thing
>> >> to do to make it visible to the io scheduler so soon. Well of course
>> >> even more ideally we'd always get big requests submitted, but
>> >> unfortunately many producers aren't that nice.
>> >>
>> >> The per-process plugging actually solves this nicely, since we do the
>> >> merging outside of the io scheduler. Perhaps just not dispatch on a
>> >> plugged queue would help a bit. I'm somewhat against this principle of
>> >> messing too much with dispatch logic in the schedulers, it'd be nicer to
>> >> solve this higher up.
>> >
>> > Something like this...
>> >
>> > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> > index 5dfb7b9..5ab1a17 100644
>> > --- a/block/cfq-iosched.c
>> > +++ b/block/cfq-iosched.c
>> > @@ -1775,6 +1775,9 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>> >
>> >        cic->last_request_pos = rq->sector + rq->nr_sectors;
>> >
>> > +       if (blk_queue_plugged(cfqd->queue))
>> > +               return;
>> > +
>> >        if (cfqq == cfqd->active_queue) {
>> >                /*
>> >                 * if we are waiting for a request for this queue, let it rip
>> > @@ -1784,7 +1787,7 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>> >                if (cfq_cfqq_wait_request(cfqq)) {
>> >                        cfq_mark_cfqq_must_dispatch(cfqq);
>> >                        del_timer(&cfqd->idle_slice_timer);
>> > -                       blk_start_queueing(cfqd->queue);
>> > +                       cfq_schedule_dispatch(cfqd);
>> >                }
>> >        } else if (cfq_should_preempt(cfqd, cfqq, rq)) {
>> >                /*
>> > @@ -1794,7 +1797,7 @@ cfq_rq_enqueued(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>> >                 */
>> >                cfq_preempt_queue(cfqd, cfqq);
>> >                cfq_mark_cfqq_must_dispatch(cfqq);
>> > -               blk_start_queueing(cfqd->queue);
>> > +               cfq_schedule_dispatch(cfqd);
>> >        }
>> >  }
>> >
>> > @@ -1997,11 +2000,10 @@ static void cfq_kick_queue(struct work_struct *work)
>> >        struct cfq_data *cfqd =
>> >                container_of(work, struct cfq_data, unplug_work);
>> >        struct request_queue *q = cfqd->queue;
>> > -       unsigned long flags;
>> >
>> > -       spin_lock_irqsave(q->queue_lock, flags);
>> > +       spin_lock_irq(q->queue_lock);
>> >        blk_start_queueing(q);
>> > -       spin_unlock_irqrestore(q->queue_lock, flags);
>> > +       spin_unlock_irq(q->queue_lock);
>> >  }
>> >
>> >  /*
>>
>> Platter speed at 64KB stride, but 16% (101MB/s) less performance at
>> 4KB stride - perhaps merging isn't quite right?
>>
>> Both traces at http://quora.org/blktrace-profiles-3.tar.bz2 ; let me
>> know if you'd like me to test Fabio's patch still.
>
> If you have time, please do test that one as well, thanks :-)
>
> --
> Jens Axboe
>
>

thanks for the 2 patches, please keep them coming :)

a short report (due to the time shortage):

I tested both patches this morning and got for both (still) around
52-58 MB/s (/dev/sdd & /dev/sde)

thanks & have a nice weekend :)

Mat