Re: [PATCH] cfq: Fix starvation of async writes in presence of heavy sync workload

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Jun 21, 2011 at 10:15:15AM +0800, Shaohua Li wrote:
> 2011/6/20 Vivek Goyal <vgoyal@redhat.com>:
> > In presence of heavy sync workload CFQ can starve asnc writes.
> > If one launches multiple readers (say 16), then one can notice
> > that CFQ can withhold dispatch of WRITEs for a very long time say
> > 200 or 300 seconds.
> >
> > Basically CFQ schedules an async queue but does not dispatch any
> > writes because it is waiting for exisintng sync requests in queue to
> > finish. While it is waiting, one or other reader gets queued up and
> > preempts the async queue. So we did schedule the async queue but never
> > dispatched anything from it. This can repeat for long time hence
> > practically starving Writers.
> >
> > This patch allows async queue to dispatch atleast 1 requeust once
> > it gets scheduled and denies preemption if async queue has been
> > waiting for sync requests to drain and has not been able to dispatch
> > a request yet.
> >
> > One concern with this fix is that how does it impact readers
> > in presence of heavy writting going on.
> >
> > I did a test where I launch firefox, load a website and close
> > firefox and measure the time. I ran the test 3 times and took
> > average.
> >
> > - Vanilla kernel time ~= 1 minute 40 seconds
> > - Patched kenrel time ~= 1 minute 35 seconds
> >
> > Basically it looks like that for this test times have not
> > changed much for this test. But I would not claim that it does
> > not impact reader's latencies at all. It might show up in
> > other workloads.
> >
> > I think we anyway need to fix writer starvation. If this patch
> > causes issues, then we need to look at reducing writer's
> > queue depth further to improve latencies for readers.
> I'm afraid this can causes read latency because cfq_dispatch_requests
> doesn't check preempt. we will dispatch 4 requests at least instead of
> just one. can we add a logic to force it just dispatches one request?

This will happen only if some other read queue does not preempt write
queue after disptaching 1 request.

Anyway, agreed that with single reader, it will not preempt writer and
then writer gets to dispatch bunch of requests.

If we want to protect against that, then we can simply expire writer
after dispatching one request if there are busy queues.

I could change following code.

        /*
         * expire an async queue immediately if it has used up its slice.
         * idle
         * queue always expire after 1 dispatch round.
         */
        if (cfqd->busy_queues > 1 && ((!cfq_cfqq_sync(cfqq) &&
            cfqq->slice_dispatch >= cfq_prio_to_maxrq(cfqd, cfqq)) ||
            cfq_class_idle(cfqq))) {
                cfqq->slice_end = jiffies + 1;
                cfq_slice_expired(cfqd, 0);
        }

to look as follows.

        /*
         * expire an async queue and idle queue after 1 dispatch round.
         */
        if (cfqd->busy_queues > 1 && ((!cfq_cfqq_sync(cfqq) ||
	    cfq_class_idle(cfqq))) {
                cfqq->slice_end = jiffies + 1;
                cfq_slice_expired(cfqd, 0);
        }

Will this help?

Thanks
Vivek