Re: [PATCH] cfq-iosched: Revert the logic of deep queues

[Corrado Zoccolo]

On Wed, May 19, 2010 at 10:33 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> o This patch basically reverts following commit.
>
> Â Â Â Â76280af cfq-iosched: idling on deep seeky sync queues
>
> o Idling in CFQ is bad on high end storage. This is especially more true of
> Ârandom reads. Idling works very well for SATA disks with single
> Âspindle but harms a lot on powerful storage boxes.
>
> ÂSo even if deep queues can be little unfair to other random workload with
> Âshallow depths, treat deep queues as sync-noidle workload and not sync,
> Âbecause with sync workload we dispatch IO from only one queue at a time
> Âand idle and we don't drive enough queue depth to keep the array busy.

Maybe we should have a tunable for this behavior, like we have
group_isolation for groups? It seems the same trade off to me
(fairness vs. performance).
And we could let it default off (on NCQ disks), since I guess deep
seeky queues are more frequent on high end hardware than on our
desktops.
Note that on non-NCQ disks there is not only a fairness problem, but
also a latency problem by having those deep queues as noidle (so I
would say that non-NCQ disks should default to on, instead).

Thanks
Corrado
>
> o I am running aio-stress (random reads) as follows.
>
> Âaio-stress -s 2g -O -t 4 -r 64k aio5 aio6 aio7 aio8 -o 3
>
> ÂFollowing are results with various combinations.
>
> Âdeadline: Â Â Â Â Â Â 232.94 MB/s
>
> Âwithout patch
> Â-------------
> Âcfq default      75.32 MB/s
> Âcfq, quantum=64 Â Â Â 134.58 MB/s
>
> Âwith patch
> Â----------
> Âcfq default      78.37 MB/s
> Âcfq, quantum=64 Â Â Â 213.94 MB
>
> ÂNote that with the patch applied, cfq really scales well if "quantum" is
> Âincreased and comes close to deadline performance.
>
> o Point being that on powerful arrays one queue is not sufficient to keep
> Âarray busy. This is already a bottleneck for sequential workloads. Lets
> Ânot aggravate the problem by marking random read queues as sync and
> Âgiving them exclusive access and hence effectively serializing the
> Âaccess to array.
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> Âblock/cfq-iosched.c | Â 12 +-----------
> Â1 files changed, 1 insertions(+), 11 deletions(-)
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 5f127cf..3336bd7 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -313,7 +313,6 @@ enum cfqq_state_flags {
> Â Â Â ÂCFQ_CFQQ_FLAG_sync, Â Â Â Â Â Â /* synchronous queue */
> Â Â Â ÂCFQ_CFQQ_FLAG_coop, Â Â Â Â Â Â /* cfqq is shared */
> Â Â Â ÂCFQ_CFQQ_FLAG_split_coop, Â Â Â /* shared cfqq will be splitted */
> - Â Â Â CFQ_CFQQ_FLAG_deep, Â Â Â Â Â Â /* sync cfqq experienced large depth */
> Â Â Â ÂCFQ_CFQQ_FLAG_wait_busy, Â Â Â Â/* Waiting for next request */
> Â};
>
> @@ -342,7 +341,6 @@ CFQ_CFQQ_FNS(slice_new);
> ÂCFQ_CFQQ_FNS(sync);
> ÂCFQ_CFQQ_FNS(coop);
> ÂCFQ_CFQQ_FNS(split_coop);
> -CFQ_CFQQ_FNS(deep);
> ÂCFQ_CFQQ_FNS(wait_busy);
> Â#undef CFQ_CFQQ_FNS
>
> @@ -3036,11 +3034,8 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>
> Â Â Â Âenable_idle = old_idle = cfq_cfqq_idle_window(cfqq);
>
> - Â Â Â if (cfqq->queued[0] + cfqq->queued[1] >= 4)
> - Â Â Â Â Â Â Â cfq_mark_cfqq_deep(cfqq);
> -
> Â Â Â Âif (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
> - Â Â Â Â Â (!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
> + Â Â Â Â Â CFQQ_SEEKY(cfqq))
> Â Â Â Â Â Â Â Âenable_idle = 0;
> Â Â Â Âelse if (sample_valid(cic->ttime_samples)) {
> Â Â Â Â Â Â Â Âif (cic->ttime_mean > cfqd->cfq_slice_idle)
> @@ -3593,11 +3588,6 @@ static void cfq_idle_slice_timer(unsigned long data)
> Â Â Â Â Â Â Â Â */
> Â Â Â Â Â Â Â Âif (!RB_EMPTY_ROOT(&cfqq->sort_list))
> Â Â Â Â Â Â Â Â Â Â Â Âgoto out_kick;
> -
> - Â Â Â Â Â Â Â /*
> - Â Â Â Â Â Â Â Â* Queue depth flag is reset only when the idle didn't succeed
> - Â Â Â Â Â Â Â Â*/
> - Â Â Â Â Â Â Â cfq_clear_cfqq_deep(cfqq);
> Â Â Â Â}
> Âexpire:
> Â Â Â Âcfq_slice_expired(cfqd, timed_out);
> --
> 1.6.5.2
>
>



-- 
__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:czoccolo@xxxxxxxxx
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/