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

[Shaohua Li <shaohua.li@intel.com>]

On Tue, 2011-06-21 at 23:26 +0800, Vivek Goyal wrote:
> 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.
could happen in multiple queues too, because ncq disks dispatch several
requests in a short time.

> 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);
>         }
Looks fine. the cfqd->busy_queues check for async queue need exclude a
idle queue if there is. That is for a idle queue and an aync queue,
don't expire async. maybe use this:
if (cfqd->busy_queues > 1 && cfq_calss_idle(cfqq) ||
cfqd->busy_sync_queues > 0 && !cfq_cfqq_sync(cfqq))

Thanks,
Shaohua