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

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

[Corrado Zoccolo]

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 |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index d1ad066..5ef9a5d 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -214,7 +214,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
@@ -2066,7 +2066,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;
 }
 
 static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)
@@ -3349,7 +3349,12 @@ 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_noidle(rq);
+			u32 bitmask = 1U << (((int)cfqq) % 31);
+			if (rq_noidle(rq))
+				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
-- 
1.6.4.4

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

[Corrado Zoccolo]

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;
 }
 
 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

[PATCH 2/2] cfq-iosched: RQ_NOIDLE enabled for SYNC_WORKLOAD

[Corrado Zoccolo]

RQ_NOIDLE flag is meaningful and should be honored for SYNC_WORKLOAD,
without further checks.
RQ_NOIDLE can be used to mark the last request of a sequence for which
- we want to idle between the requests of the sequence, to keep locality
- we don't want to idle after the sequence, because we know that no new
  nearby requests will follow, so we should switch servicing other
  queues.
This patch fixes this behaviour, making it similar to how it behaved
before 8e55063, but still fixing the corner cases that were the
motivation for it.

Signed-off-by: Corrado Zoccolo <czoccolo@gmail.com>
---
 block/cfq-iosched.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 596b747..bc5c0f9 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -3428,13 +3428,17 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
 				cfqd->noidle_tree_requires_idle |= bitmask;
 
 			/*
-			 * Idling is enabled for SYNC_WORKLOAD.
-			 * SYNC_NOIDLE_WORKLOAD idles at the end of the tree
-			 * only if at least one queue sent !RQ_NOIDLE requests
-			 * not followed by at least one RQ_NOIDLE request.
+ 			 * Idling is enabled for:
+ 			 * - the last sync queue of a group
+ 			 * - SYNC_WORKLOAD queues, for !RQ_NOIDLE requests
+ 			 * - SYNC_NOIDLE_WORKLOAD "at the end of the tree"
+ 			 *   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
+			if ((cfqd->serving_type == SYNC_WORKLOAD
+			     && !(rq->cmd_flags & REQ_NOIDLE))
+			    || (cfqd->serving_type == SYNC_NOIDLE_WORKLOAD
+				&& cfqd->noidle_tree_requires_idle)
 			    || cfqq->cfqg->nr_cfqq == 1)
 				cfq_arm_slice_timer(cfqd);
 		}
-- 
1.6.4.4

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

[Vivek Goyal]

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