Re: [PATCH 1/2] cfq-iosched: fix tree-wide handling of rq_noidle

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Jul 07, 2010 at 05:55:44PM +0200, Corrado Zoccolo wrote:
> Patch: 8e55063 cfq-iosched: fix corner cases in idling logic
> introduced the possibility of iding even on no-idle requests
> in the no_idle tree, if any previous request in the current slice
> could idle. The implementation had a problem, though:
> - if a queue sent several possibly idling requests and a noidle
>   request as last one in the same time slice, the tree was still
>   marked as idle.
> This patch fixes this misbehaviour, by using a 31-bucket hash to
> keep idling/non-idling status for queues in the noidle tree.
> 
> Signed-off-by: Corrado Zoccolo <czoccolo@gmail.com>
> ---
>  block/cfq-iosched.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index eb4086f..596b747 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -216,7 +216,7 @@ struct cfq_data {
>  	enum wl_type_t serving_type;
>  	unsigned long workload_expires;
>  	struct cfq_group *serving_group;
> -	bool noidle_tree_requires_idle;
> +	u32 noidle_tree_requires_idle;
>  
>  	/*
>  	 * Each priority tree is sorted by next_request position.  These
> @@ -2126,7 +2126,7 @@ static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
>  	slice = max_t(unsigned, slice, CFQ_MIN_TT);
>  	cfq_log(cfqd, "workload slice:%d", slice);
>  	cfqd->workload_expires = jiffies + slice;
> -	cfqd->noidle_tree_requires_idle = false;
> +	cfqd->noidle_tree_requires_idle = 0U;
>  }

I think we are keeping this state for too long. IOW, we reset this state
on every service tree expiry. So assume that service tree share is 100ms
and some IO happened initially, for 1ms. Now next queues does WRITE_SYNC
IO for 10ms, in that case we don't have to idle even on SYNC_NOIDLE tree.

I think a better way is to keep track if some other queue has done any
IO on this service tree in last slice_idle period or not. That way the
window we look at is limited to slice_idle and not the whole duration of
service tree slice.

Jeff Moyer implemented this logic in his patches for fixing fsync issue.

Thanks
Vivek


>  
>  static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)
> @@ -3421,12 +3421,17 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
>  			cfq_slice_expired(cfqd, 1);
>  		else if (sync && cfqq_empty &&
>  			 !cfq_close_cooperator(cfqd, cfqq)) {
> -			cfqd->noidle_tree_requires_idle |=
> -				!(rq->cmd_flags & REQ_NOIDLE);
> +			u32 bitmask = 1U << (((int)cfqq) % 31);
> +			if (rq->cmd_flags & REQ_NOIDLE)
> +				cfqd->noidle_tree_requires_idle &= ~bitmask;
> +			else
> +				cfqd->noidle_tree_requires_idle |= bitmask;
> +
>  			/*
>  			 * Idling is enabled for SYNC_WORKLOAD.
>  			 * SYNC_NOIDLE_WORKLOAD idles at the end of the tree
> -			 * only if we processed at least one !REQ_NOIDLE request
> +			 * only if at least one queue sent !RQ_NOIDLE requests
> +			 * not followed by at least one RQ_NOIDLE request.
>  			 */
>  			if (cfqd->serving_type == SYNC_WORKLOAD
>  			    || cfqd->noidle_tree_requires_idle
> -- 
> 1.6.4.4