Re: [PATCH]cfq-iosched: split seeky coop queues after one slice

[Corrado Zoccolo]

On Mon, Dec 28, 2009 at 4:19 AM, Shaohua Li <shaohua.li@xxxxxxxxx> wrote:
> On Fri, Dec 25, 2009 at 06:16:27PM +0800, Corrado Zoccolo wrote:
>> Hi Shaohua,
>> On Thu, Dec 24, 2009 at 1:55 AM, Shaohua Li <shaohua.li@xxxxxxxxx> wrote:
>> > df5fe3e8e13883f58dc97489076bbcc150789a21
>> > b3b6d0408c953524f979468562e7e210d8634150
>> > The coop merge is too aggressive. For example, if two tasks are reading two
>> > files where the two files have some adjecent blocks, cfq will immediately
>> > merge them. cfq_rq_close() also has trouble, sometimes the seek_mean is very
>> > big. I did a test to make cfq_rq_close() always checks the distence according
>> > to CIC_SEEK_THR, but still saw a lot of wrong merge. (BTW, why we take a long
>> > distence far away request as close. Taking them close doesn't improve any thoughtput
>> > to me. Maybe we should always use CIC_SEEK_THR as close criteria).
>> Yes, when deciding if two queues are going to be merged, we should use
>> the constant CIC_SEEK_THR.
>> > So sounds we need make split more aggressive. But the split is too lazay,
>> > which requires to wait 1s. Time based check isn't reliable as queue might not
>> > run at given time, so uses a small time isn't ok.
>> 1s is too much, but I wouldn't abandon a time based approach. To fix
>> the problem of queue not being run, you can consider a slice. If at
>> the end of the slice, the queue is seeky, you split it.
>
> Currently we split seeky coop queues after 1s, which is too big. Below patch
> marks seeky coop queue split_coop flag after one slice. After that, if new
> requests come in, the queues will be splitted. Patch is suggested by Corrado.
>
> Signed-off-by: Shaohua Li <shaohua.li@xxxxxxxxx>
You can also remove the no longer used define:
#define CFQQ_COOP_TOUT          (HZ)

Reviewed-by: Corrado Zoccolo <czoccolo@xxxxxxxxx>

>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index e2f8046..d4d5cca 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -52,6 +52,9 @@ static const int cfq_hist_divisor = 4;
> Â#define CFQ_HW_QUEUE_MIN Â Â Â (5)
> Â#define CFQ_SERVICE_SHIFT Â Â Â 12
>
> +#define CFQQ_SEEK_THR Â Â Â Â Â8 * 1024
> +#define CFQQ_SEEKY(cfqq) Â Â Â ((cfqq)->seek_mean > CFQQ_SEEK_THR)
> +
> Â#define RQ_CIC(rq) Â Â Â Â Â Â \
> Â Â Â Â((struct cfq_io_context *) (rq)->elevator_private)
> Â#define RQ_CFQQ(rq) Â Â Â Â Â Â(struct cfq_queue *) ((rq)->elevator_private2)
> @@ -137,7 +140,6 @@ struct cfq_queue {
> Â Â Â Âu64 seek_total;
> Â Â Â Âsector_t seek_mean;
> Â Â Â Âsector_t last_request_pos;
> - Â Â Â unsigned long seeky_start;
>
> Â Â Â Âpid_t pid;
>
> @@ -317,6 +319,7 @@ enum cfqq_state_flags {
> Â Â Â ÂCFQ_CFQQ_FLAG_slice_new, Â Â Â Â/* no requests dispatched in slice */
> Â Â Â Â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 */
> Â};
> @@ -345,6 +348,7 @@ CFQ_CFQQ_FNS(prio_changed);
> Â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
> @@ -1574,6 +1578,15 @@ __cfq_slice_expired(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> Â Â Â Âcfq_clear_cfqq_wait_busy(cfqq);
>
> Â Â Â Â/*
> + Â Â Â Â* If this cfqq is shared between multiple processes, check to
> + Â Â Â Â* make sure that those processes are still issuing I/Os within
> + Â Â Â Â* the mean seek distance. ÂIf not, it may be time to break the
> + Â Â Â Â* queues apart again.
> + Â Â Â Â*/
> + Â Â Â if (cfq_cfqq_coop(cfqq) && CFQQ_SEEKY(cfqq))
> + Â Â Â Â Â Â Â cfq_mark_cfqq_split_coop(cfqq);
> +
> + Â Â Â /*
> Â Â Â Â * store what was left of this slice, if the queue idled/timed out
> Â Â Â Â */
> Â Â Â Âif (timed_out && !cfq_cfqq_slice_new(cfqq)) {
> @@ -1671,9 +1684,6 @@ static inline sector_t cfq_dist_from_last(struct cfq_data *cfqd,
> Â Â Â Â Â Â Â Âreturn cfqd->last_position - blk_rq_pos(rq);
> Â}
>
> -#define CFQQ_SEEK_THR Â Â Â Â Â8 * 1024
> -#define CFQQ_SEEKY(cfqq) Â Â Â ((cfqq)->seek_mean > CFQQ_SEEK_THR)
> -
> Âstatic inline int cfq_rq_close(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct request *rq)
> Â{
> @@ -3027,19 +3037,6 @@ cfq_update_io_seektime(struct cfq_data *cfqd, struct cfq_queue *cfqq,
> Â Â Â Âtotal = cfqq->seek_total + (cfqq->seek_samples/2);
> Â Â Â Âdo_div(total, cfqq->seek_samples);
> Â Â Â Âcfqq->seek_mean = (sector_t)total;
> -
> - Â Â Â /*
> - Â Â Â Â* If this cfqq is shared between multiple processes, check to
> - Â Â Â Â* make sure that those processes are still issuing I/Os within
> - Â Â Â Â* the mean seek distance. ÂIf not, it may be time to break the
> - Â Â Â Â* queues apart again.
> - Â Â Â Â*/
> - Â Â Â if (cfq_cfqq_coop(cfqq)) {
> - Â Â Â Â Â Â Â if (CFQQ_SEEKY(cfqq) && !cfqq->seeky_start)
> - Â Â Â Â Â Â Â Â Â Â Â cfqq->seeky_start = jiffies;
> - Â Â Â Â Â Â Â else if (!CFQQ_SEEKY(cfqq))
> - Â Â Â Â Â Â Â Â Â Â Â cfqq->seeky_start = 0;
> - Â Â Â }
> Â}
>
> Â/*
> @@ -3474,14 +3471,6 @@ cfq_merge_cfqqs(struct cfq_data *cfqd, struct cfq_io_context *cic,
> Â Â Â Âreturn cic_to_cfqq(cic, 1);
> Â}
>
> -static int should_split_cfqq(struct cfq_queue *cfqq)
> -{
> - Â Â Â if (cfqq->seeky_start &&
> - Â Â Â Â Â time_after(jiffies, cfqq->seeky_start + CFQQ_COOP_TOUT))
> - Â Â Â Â Â Â Â return 1;
> - Â Â Â return 0;
> -}
> -
> Â/*
> Â* Returns NULL if a new cfqq should be allocated, or the old cfqq if this
> Â* was the last process referring to said cfqq.
> @@ -3490,9 +3479,9 @@ static struct cfq_queue *
> Âsplit_cfqq(struct cfq_io_context *cic, struct cfq_queue *cfqq)
> Â{
> Â Â Â Âif (cfqq_process_refs(cfqq) == 1) {
> - Â Â Â Â Â Â Â cfqq->seeky_start = 0;
> Â Â Â Â Â Â Â Âcfqq->pid = current->pid;
> Â Â Â Â Â Â Â Âcfq_clear_cfqq_coop(cfqq);
> + Â Â Â Â Â Â Â cfq_clear_cfqq_split_coop(cfqq);
> Â Â Â Â Â Â Â Âreturn cfqq;
> Â Â Â Â}
>
> @@ -3531,7 +3520,7 @@ new_queue:
> Â Â Â Â Â Â Â Â/*
> Â Â Â Â Â Â Â Â * If the queue was seeky for too long, break it apart.
> Â Â Â Â Â Â Â Â */
> - Â Â Â Â Â Â Â if (cfq_cfqq_coop(cfqq) && should_split_cfqq(cfqq)) {
> + Â Â Â Â Â Â Â if (cfq_cfqq_coop(cfqq) && cfq_cfqq_split_coop(cfqq)) {
> Â Â Â Â Â Â Â Â Â Â Â Âcfq_log_cfqq(cfqd, cfqq, "breaking apart cfqq");
> Â Â Â Â Â Â Â Â Â Â Â Âcfqq = split_cfqq(cic, cfqq);
> Â Â Â Â Â Â Â Â Â Â Â Âif (!cfqq)
>
--
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/