cfq: Fixes for unaccounted_time variable

Commit "Add unaccounted time to timeslice_used" changed the behavior of cfq_preempt_queue to set cfqq active. Vivek pointed out that other preemption rules might get involved, so we shouldn't manually set which queue is active.

This cleans up the code to just clear the queue stats at preemption time.

Signed-off-by Justin TerAvest <[email protected]>

---
 block/cfq-iosched.c |   39 +++++++++++++++++++++++----------------
 1 files changed, 23 insertions(+), 16 deletions(-)

block/cfq-iosched.c +21 -15

index 12e380b..69208d7 100644
@@ -1620,27 +1620,33 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
 	cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
 }
 
+static void cfq_clear_queue_stats(struct cfq_data *cfqd,
+				  struct cfq_queue *cfqq)
+{
+	cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
+	cfqq->slice_start = 0;
+	cfqq->dispatch_start = jiffies;
+	cfqq->allocated_slice = 0;
+	cfqq->slice_end = 0;
+	cfqq->slice_dispatch = 0;
+	cfqq->nr_sectors = 0;
+
+	cfq_clear_cfqq_wait_request(cfqq);
+	cfq_clear_cfqq_must_dispatch(cfqq);
+	cfq_clear_cfqq_must_alloc_slice(cfqq);
+	cfq_clear_cfqq_fifo_expire(cfqq);
+	cfq_mark_cfqq_slice_new(cfqq);
+
+	cfq_del_timer(cfqd, cfqq);
+}
+
 static void __cfq_set_active_queue(struct cfq_data *cfqd,
 				   struct cfq_queue *cfqq)
 {
 	if (cfqq) {
 		cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
 				cfqd->serving_prio, cfqd->serving_type);
-		cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
-		cfqq->slice_start = 0;
-		cfqq->dispatch_start = jiffies;
-		cfqq->allocated_slice = 0;
-		cfqq->slice_end = 0;
-		cfqq->slice_dispatch = 0;
-		cfqq->nr_sectors = 0;
-
-		cfq_clear_cfqq_wait_request(cfqq);
-		cfq_clear_cfqq_must_dispatch(cfqq);
-		cfq_clear_cfqq_must_alloc_slice(cfqq);
-		cfq_clear_cfqq_fifo_expire(cfqq);
-		cfq_mark_cfqq_slice_new(cfqq);
-
-		cfq_del_timer(cfqd, cfqq);
+		cfq_clear_queue_stats(cfqd, cfqq);
 	}
 
 	cfqd->active_queue = cfqq;

Quoted 67 lines · Justin TerAvest

Commit "Add unaccounted time to timeslice_used" changed the behavior of cfq_preempt_queue to set cfqq active. Vivek pointed out that other preemption rules might get involved, so we shouldn't manually set which queue is active.

This cleans up the code to just clear the queue stats at preemption time.

Signed-off-by Justin TerAvest <[email protected]>

---
 block/cfq-iosched.c |   39 +++++++++++++++++++++++----------------

 1 files changed, 23 insertions(+), 16 deletions(-)

block/cfq-iosched.c +21 -15

index 12e380b..69208d7 100644
@@ -1620,27 +1620,33 @@ static inline void cfq_del_timer(struct cfq_data *cfqd, struct cfq_queue *cfqq)
       cfq_blkiocg_update_idle_time_stats(&cfqq->cfqg->blkg);
  }
 
+static void cfq_clear_queue_stats(struct cfq_data *cfqd,
+                               struct cfq_queue *cfqq)
+{
+     cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
+     cfqq->slice_start = 0;
+     cfqq->dispatch_start = jiffies;
+     cfqq->allocated_slice = 0;
+     cfqq->slice_end = 0;
+     cfqq->slice_dispatch = 0;
+     cfqq->nr_sectors = 0;
+
+     cfq_clear_cfqq_wait_request(cfqq);
+     cfq_clear_cfqq_must_dispatch(cfqq);
+     cfq_clear_cfqq_must_alloc_slice(cfqq);
+     cfq_clear_cfqq_fifo_expire(cfqq);
+     cfq_mark_cfqq_slice_new(cfqq);
+
+     cfq_del_timer(cfqd, cfqq);
+}
+
  static void __cfq_set_active_queue(struct cfq_data *cfqd,
                                  struct cfq_queue *cfqq)
  {
       if (cfqq) {
               cfq_log_cfqq(cfqd, cfqq, "set_active wl_prio:%d wl_type:%d",
                               cfqd->serving_prio, cfqd->serving_type);
-             cfq_blkiocg_update_avg_queue_size_stats(&cfqq->cfqg->blkg);
-             cfqq->slice_start = 0;
-             cfqq->dispatch_start = jiffies;
-             cfqq->allocated_slice = 0;
-             cfqq->slice_end = 0;
-             cfqq->slice_dispatch = 0;
-             cfqq->nr_sectors = 0;
-
-             cfq_clear_cfqq_wait_request(cfqq);
-             cfq_clear_cfqq_must_dispatch(cfqq);
-             cfq_clear_cfqq_must_alloc_slice(cfqq);
-             cfq_clear_cfqq_fifo_expire(cfqq);
-             cfq_mark_cfqq_slice_new(cfqq);
-
-             cfq_del_timer(cfqd, cfqq);
+             cfq_clear_queue_stats(cfqd, cfqq);
       }
 
       cfqd->active_queue = cfqq;

block/cfq-iosched.c static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq) · line 3342

Quoted 6 lines

block/cfq-iosched.c +2 -1

@@ -3332,7 +3338,8 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
       BUG_ON(!cfq_cfqq_on_rr(cfqq));
 
       cfq_service_tree_add(cfqd, cfqq, 1);
-     __cfq_set_active_queue(cfqd, cfqq);
+
+     cfq_clear_queue_stats(cfqd, cfqq);

Hi Justin,

Why do we have to clear queue stats here for the preempting queue?

Especially look at cfqq->dispatch_start = jiffies. We have not started the dispatch yet. When this queue is selected next, then we will start the dispatch.

So this patch has introduced another bug now. Now after preemption if we don't select this group, then we have a queue with wrong dispatch start and that will result in huge slice_used for the queue and it will not get its fair share.

Thanks
Vivek

Quoted 13 lines

block/cfq-iosched.c static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq) · line 3342

Quoted 8 lines

      cfqd->active_queue = cfqq;

block/cfq-iosched.c +2 -1

@@ -3332,7 +3338,8 @@ static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
       BUG_ON(!cfq_cfqq_on_rr(cfqq));
 
       cfq_service_tree_add(cfqd, cfqq, 1);
-     __cfq_set_active_queue(cfqd, cfqq);
+
+     cfq_clear_queue_stats(cfqd, cfqq);

Hi Justin,

Why do we have to clear queue stats here for the preempting queue?

Quoted 8 lines

Especially look at cfqq->dispatch_start = jiffies. We have not started the dispatch yet. When this queue is selected next, then we will start the dispatch.

So this patch has introduced another bug now. Now after preemption if we don't select this group, then we have a queue with wrong dispatch start and that will result in huge slice_used for the queue and it will not get its fair share.

Hi Vivek,

Ugh, you're right. Sorry, I had some bad ideas in my head for how preemption worked that clearly aren't true.
I think that if the stats aren't cleared here, everything should then be fine because jiffies will then be picked up when the active queue is set. Does that sound sane to you?

This should fix both issues that Vivek pointed out:
* We can't set active queue at preempt time, because select queue might select a different queue than the one we chose to preempt.
* Move the new variable to only be exported when debugging options are

    on.