Re: [RFC V2 PATCH 3/5] cfq-iosched: reimplement priorities using different service trees

[Jeff Moyer <jmoyer@redhat.com>]

Corrado Zoccolo <czoccolo@gmail.com> writes:

> We use different service trees for different priority classes.
> This allows a simplification in the service tree insertion code, that no
> longer has to consider priority while walking the tree.

This is kind of funny, considering things used to be divied up into
lists by class and priority.

> + * Index in the service_trees.
> + * IDLE is handled separately, so it has negative index
> + */
> +enum wl_prio_t {
> +	IDLE_WORKLOAD = -1,
> +	BE_WORKLOAD = 0,
> +	RT_WORKLOAD = 1
> +};

What's wrong with IOPRIO_CLASS_(RT|BE|IDLE)?  Why invent another enum?

> +
> +/*
>   * Per block device queue structure
>   */
>  struct cfq_data {
[...]
> +	struct cfq_rb_root service_trees[2];
> +	struct cfq_rb_root service_tree_idle;

Why separate out the idle service tree from the others?

> +static struct cfq_rb_root *service_tree_for(enum wl_prio_t prio,
> +					    struct cfq_data *cfqd)
> +{
> +	if (prio == IDLE_WORKLOAD)
> +		return &cfqd->service_tree_idle;
> +
> +	return &cfqd->service_trees[prio];
> +}

This should just turn into cfqd->service_trees[IOPRIO_CLASS_*] in the
callers.

[...]

>  /*
> @@ -1106,6 +1134,10 @@ static struct cfq_queue *cfq_close_cooperator(struct cfq_data *cfqd,
>  	if (cfq_cfqq_coop(cfqq))
>  		return NULL;
>  
> +	/* we don't want to mix processes with different characteristics */
> +	if (cfqq->service_tree != cur_cfqq->service_tree)
> +		return NULL;
> +

Hmm, that looks like a current bug in the close cooperator code.  It
shouldn't allow cooperation between differring scheduling classes.

Cheers,
Jeff