From ad377cbbd662d8a5ada21ade508cbe6cb54cf4c0 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Sat, 4 Feb 2017 17:31:41 +0100 Subject: [PATCH 01/24] Remove wrong compilation warning BUGFIX: Removed a wrong compilation warning, due to the compiler not taking into account short circuit in a condition. Signed-off-by: Paolo Valente --- block/bfq-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/bfq-sched.c b/block/bfq-sched.c index 797bce75db01..2e9dc59de0ed 100644 --- a/block/bfq-sched.c +++ b/block/bfq-sched.c @@ -1301,7 +1301,7 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, bool expiration) { struct bfq_sched_data *sd; - struct bfq_entity *parent; + struct bfq_entity *parent = NULL; for_each_entity_safe(entity, parent) { sd = entity->sched_data; From 3fbad6c8e9908bacbe1b1075c63e1ca5c9ebada5 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Mon, 19 Dec 2016 17:14:58 +0100 Subject: [PATCH 02/24] Add a ton of forgotten static qualifiers BUGIFX: Added several forgotten static qualifiers in function definitions (completely harmless issue). Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 78baa071a97e..8e83c70f38f6 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -107,7 +107,7 @@ static const int bfq_async_charge_factor = 10; /* Default timeout values, in jiffies, approximating CFQ defaults. */ static const int bfq_timeout = (HZ / 8); -struct kmem_cache *bfq_pool; +static struct kmem_cache *bfq_pool; /* Below this threshold (in ns), we consider thinktime immediate. */ #define BFQ_MIN_TT (2 * NSEC_PER_MSEC) @@ -1868,7 +1868,7 @@ static bool bfq_may_be_close_cooperator(struct bfq_queue *bfqq, * positives. In case bfqq is weight-raised, such false positives * would evidently degrade latency guarantees for bfqq. */ -bool wr_from_too_long(struct bfq_queue *bfqq) +static bool wr_from_too_long(struct bfq_queue *bfqq) { return bfqq->wr_coeff > 1 && time_is_before_jiffies(bfqq->last_wr_start_finish + @@ -2298,7 +2298,7 @@ static unsigned long bfq_calc_max_budget(struct bfq_data *bfqd) * function of the estimated peak rate. See comments on * bfq_calc_max_budget(), and on T_slow and T_fast arrays. */ -void update_thr_responsiveness_params(struct bfq_data *bfqd) +static void update_thr_responsiveness_params(struct bfq_data *bfqd) { int dev_type = blk_queue_nonrot(bfqd->queue); @@ -2333,7 +2333,7 @@ void update_thr_responsiveness_params(struct bfq_data *bfqd) BFQ_RATE_SHIFT); } -void bfq_reset_rate_computation(struct bfq_data *bfqd, struct request *rq) +static void bfq_reset_rate_computation(struct bfq_data *bfqd, struct request *rq) { if (rq != NULL) { /* new rq dispatch now, reset accordingly */ bfqd->last_dispatch = bfqd->first_dispatch = ktime_get_ns() ; @@ -2350,7 +2350,7 @@ void bfq_reset_rate_computation(struct bfq_data *bfqd, struct request *rq) bfqd->tot_sectors_dispatched); } -void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq) +static void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq) { u32 rate, weight, divisor; @@ -2515,7 +2515,7 @@ void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq) * of the observed dispatch rate. The function assumes to be invoked * on every request dispatch. */ -void bfq_update_peak_rate(struct bfq_data *bfqd, struct request *rq) +static void bfq_update_peak_rate(struct bfq_data *bfqd, struct request *rq) { u64 now_ns = ktime_get_ns(); From 001a382f639c2bafc96f408ceede0eb91a08a1aa Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Fri, 3 Feb 2017 11:53:01 +0100 Subject: [PATCH 03/24] BUGFIX: Put async queues on exit also without cgroups BUGFIX: The putting of async queues on scheduler exit was missing in case cgroups support was not active. This fix adds the missing operation. Signed-off-by: Paolo Valente --- block/bfq-cgroup.c | 3 +++ block/bfq-iosched.c | 11 ++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index bbaecd00449e..a5f8dc16900a 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -1140,6 +1140,9 @@ static inline void bfqg_stats_update_idle_time(struct bfq_group *bfqg) { } static inline void bfqg_stats_set_start_idle_time(struct bfq_group *bfqg) { } static inline void bfqg_stats_update_avg_queue_size(struct bfq_group *bfqg) { } +static void bfq_bfqq_move(struct bfq_data *bfqd, struct bfq_queue *bfqq, + struct bfq_group *bfqg) {} + static void bfq_init_entity(struct bfq_entity *entity, struct bfq_group *bfqg) { diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 8e83c70f38f6..fb41aa04c5ba 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -4093,7 +4093,13 @@ static struct bfq_queue *bfq_get_queue(struct bfq_data *bfqd, * prune it. */ if (async_bfqq) { - bfqq->ref++; + bfqq->ref++; /* + * Extra group reference, w.r.t. sync + * queue. This extra reference is removed + * only if bfqq->bfqg disappears, to + * guarantee that this queue is not freed + * until its group goes away. + */ bfq_log_bfqq(bfqd, bfqq, "get_queue, bfqq not in async: %p, %d", bfqq, bfqq->ref); *async_bfqq = bfqq; @@ -4697,7 +4703,6 @@ static void bfq_shutdown_timer_wq(struct bfq_data *bfqd) cancel_work_sync(&bfqd->unplug_work); } -#ifdef CONFIG_BFQ_GROUP_IOSCHED static void __bfq_put_async_bfqq(struct bfq_data *bfqd, struct bfq_queue **bfqq_ptr) { @@ -4730,7 +4735,6 @@ static void bfq_put_async_queues(struct bfq_data *bfqd, struct bfq_group *bfqg) __bfq_put_async_bfqq(bfqd, &bfqg->async_idle_bfqq); } -#endif static void bfq_exit_queue(struct elevator_queue *e) { @@ -4755,6 +4759,7 @@ static void bfq_exit_queue(struct elevator_queue *e) #ifdef CONFIG_BFQ_GROUP_IOSCHED blkcg_deactivate_policy(q, &blkcg_policy_bfq); #else + bfq_put_async_queues(bfqd, bfqd->root_group); kfree(bfqd->root_group); #endif From a04c02c5ad51f810fab131baabf1d68939e770b6 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Sat, 4 Feb 2017 17:54:35 +0100 Subject: [PATCH 04/24] Fix check of the percentage of sequential dispatches BUGFIX: In the peak-rate estimator, there was a serious error in the check that the percentage of sequential I/O-request dispatches was high enough to trigger an update of the peak-rate estimate. This commit fixes that check. Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index fb41aa04c5ba..c023b429ebae 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2400,7 +2400,7 @@ static void bfq_update_rate_reset(struct bfq_data *bfqd, struct request *rq) * total, and rate is below the current estimated peak rate * - rate is unreasonably high (> 20M sectors/sec) */ - if ((bfqd->peak_rate_samples > (3 * bfqd->sequential_samples)>>2 && + if ((bfqd->sequential_samples < (3 * bfqd->peak_rate_samples)>>2 && rate <= bfqd->peak_rate) || rate > 20< Date: Sat, 4 Feb 2017 18:03:06 +0100 Subject: [PATCH 05/24] Better tune weight-raising for slow flash-based devices IMPROVEMENT Luca Miccio has run a few responsiveness tests on recent Android systems with average-speed storage devices. These tests have shown that the following BFQ parameter was too low for these systems: reference duration for slow storage devices of weight raising for interactive applications. This commit raises that duration to a value that is yelding optimal results in our tests. Signed-off-by: Luca Miccio Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index c023b429ebae..6be9b2d8957b 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5260,7 +5260,7 @@ static int __init bfq_init(void) * be run for a long time. */ T_slow[0] = msecs_to_jiffies(3500); /* actually 4 sec */ - T_slow[1] = msecs_to_jiffies(1000); /* actually 1.5 sec */ + T_slow[1] = msecs_to_jiffies(6000); /* actually 6.5 sec */ T_fast[0] = msecs_to_jiffies(7000); /* actually 8 sec */ T_fast[1] = msecs_to_jiffies(2500); /* actually 3 sec */ From 17f1975c1c121aa9e580d670f5433a65144126d9 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Sat, 4 Feb 2017 18:16:59 +0100 Subject: [PATCH 06/24] BFQ-v8r8-rc1 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 6be9b2d8957b..3bd971d10003 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5234,7 +5234,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r7"; + char msg[60] = "BFQ I/O-scheduler: v8r8-rc1"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index a9da00230178..32e900cccdbd 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r7 for 4.9.0: data structures and common functions prototypes. + * BFQ v8r8-rc1 for 4.10.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe From 6b14d93cb0232919b471b299b9b2cf3581c2f8b6 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Sun, 5 Feb 2017 18:50:51 +0100 Subject: [PATCH 07/24] Avoid a second dispatch in case of budget exhaustion IMPROVEMENT This commit anticipates the complete check of budget exhaustion, for the in-service bfq_queue, to when the next bfq_queue to serve is selected (during a dispatch operation). This enables a new bfq_queue to be immediately selected for service in case the in-service bfq_queue has actually exhausted its budget. As a consequence, a second dispatch invocation is not needed any more, to have a new request dispatched. To implement this improvement, this commit implements a further improvement too: the field next_rq of a bfq_queue now always contains the actual next request to dispatch (or NULL if the bfq_queue is empty). Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 122 +++++++++++++++++++++++++++------------------------- 1 file changed, 63 insertions(+), 59 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 3bd971d10003..f1cb2e07606e 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -519,13 +519,45 @@ static void bfq_weights_tree_remove(struct bfq_data *bfqd, entity->weight_counter = NULL; } +/* + * Return expired entry, or NULL to just start from scratch in rbtree. + */ +static struct request *bfq_check_fifo(struct bfq_queue *bfqq, + struct request *last) +{ + struct request *rq; + + if (bfq_bfqq_fifo_expire(bfqq)) + return NULL; + + bfq_mark_bfqq_fifo_expire(bfqq); + + rq = rq_entry_fifo(bfqq->fifo.next); + + if (rq == last || ktime_get_ns() < rq->fifo_time) + return NULL; + + bfq_log_bfqq(bfqq->bfqd, bfqq, "check_fifo: returned %p", rq); + BUG_ON(RB_EMPTY_NODE(&rq->rb_node)); + return rq; +} + static struct request *bfq_find_next_rq(struct bfq_data *bfqd, struct bfq_queue *bfqq, struct request *last) { struct rb_node *rbnext = rb_next(&last->rb_node); struct rb_node *rbprev = rb_prev(&last->rb_node); - struct request *next = NULL, *prev = NULL; + struct request *next, *prev = NULL; + + BUG_ON(list_empty(&bfqq->fifo)); + + /* Follow expired path, else get first next available. */ + next = bfq_check_fifo(bfqq, last); + if (next) { + BUG_ON(next == last); + return next; + } BUG_ON(RB_EMPTY_NODE(&last->rb_node)); @@ -1523,11 +1555,12 @@ static void bfq_remove_request(struct request *rq) elv_rb_del(&bfqq->sort_list, rq); if (RB_EMPTY_ROOT(&bfqq->sort_list)) { + bfqq->next_rq = NULL; + BUG_ON(bfqq->entity.budget < 0); if (bfq_bfqq_busy(bfqq) && bfqq != bfqd->in_service_queue) { bfq_del_bfqq_busy(bfqd, bfqq, false); - /* bfqq emptied. In normal operation, when * bfqq is empty, bfqq->entity.service and * bfqq->entity.budget must contain, @@ -2616,29 +2649,6 @@ static void bfq_dispatch_insert(struct request_queue *q, struct request *rq) elv_dispatch_sort(q, rq); } -/* - * Return expired entry, or NULL to just start from scratch in rbtree. - */ -static struct request *bfq_check_fifo(struct bfq_queue *bfqq) -{ - struct request *rq = NULL; - - if (bfq_bfqq_fifo_expire(bfqq)) - return NULL; - - bfq_mark_bfqq_fifo_expire(bfqq); - - if (list_empty(&bfqq->fifo)) - return NULL; - - rq = rq_entry_fifo(bfqq->fifo.next); - - if (ktime_get_ns() < rq->fifo_time) - return NULL; - - return rq; -} - static void __bfq_bfqq_expire(struct bfq_data *bfqd, struct bfq_queue *bfqq) { BUG_ON(bfqq != bfqd->in_service_queue); @@ -3504,14 +3514,29 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd) !bfq_bfqq_must_idle(bfqq)) goto expire; +check_queue: + /* + * This loop is rarely executed more than once. Even when it + * happens, it is much more convenient to re-execute this loop + * than to return NULL and trigger a new dispatch to get a + * request served. + */ next_rq = bfqq->next_rq; /* * If bfqq has requests queued and it has enough budget left to * serve them, keep the queue, otherwise expire it. */ if (next_rq) { + BUG_ON(RB_EMPTY_ROOT(&bfqq->sort_list)); + if (bfq_serv_to_charge(next_rq, bfqq) > bfq_bfqq_budget_left(bfqq)) { + /* + * Expire the queue for budget exhaustion, + * which makes sure that the next budget is + * enough to serve the next request, even if + * it comes from the fifo expired path. + */ reason = BFQ_BFQQ_BUDGET_EXHAUSTED; goto expire; } else { @@ -3559,9 +3584,16 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd) bfq_bfqq_expire(bfqd, bfqq, false, reason); new_queue: bfqq = bfq_set_in_service_queue(bfqd); - bfq_log(bfqd, "select_queue: new queue %d returned", - bfqq ? bfqq->pid : 0); + if (bfqq) { + bfq_log_bfqq(bfqd, bfqq, "select_queue: checking new queue"); + goto check_queue; + } keep_queue: + if (bfqq) + bfq_log_bfqq(bfqd, bfqq, "select_queue: returned this queue"); + else + bfq_log(bfqd, "select_queue: no queue returned"); + return bfqq; } @@ -3627,45 +3659,17 @@ static int bfq_dispatch_request(struct bfq_data *bfqd, struct bfq_queue *bfqq) { int dispatched = 0; - struct request *rq; + struct request *rq = bfqq->next_rq; unsigned long service_to_charge; BUG_ON(RB_EMPTY_ROOT(&bfqq->sort_list)); - - /* Follow expired path, else get first next available. */ - rq = bfq_check_fifo(bfqq); - if (!rq) - rq = bfqq->next_rq; + BUG_ON(!rq); service_to_charge = bfq_serv_to_charge(rq, bfqq); - if (service_to_charge > bfq_bfqq_budget_left(bfqq)) { - /* - * This may happen if the next rq is chosen in fifo order - * instead of sector order. The budget is properly - * dimensioned to be always sufficient to serve the next - * request only if it is chosen in sector order. The reason - * is that it would be quite inefficient and little useful - * to always make sure that the budget is large enough to - * serve even the possible next rq in fifo order. - * In fact, requests are seldom served in fifo order. - * - * Expire the queue for budget exhaustion, and make sure - * that the next act_budget is enough to serve the next - * request, even if it comes from the fifo expired path. - */ - bfqq->next_rq = rq; - /* - * Since this dispatch is failed, make sure that - * a new one will be performed - */ - if (!bfqd->rq_in_driver) - bfq_schedule_dispatch(bfqd); - BUG_ON(bfqq->entity.budget < bfqq->entity.service); - goto expire; - } + BUG_ON(service_to_charge > bfq_bfqq_budget_left(bfqq)); BUG_ON(bfqq->entity.budget < bfqq->entity.service); - /* Finally, insert request into driver dispatch list. */ + bfq_bfqq_served(bfqq, service_to_charge); BUG_ON(bfqq->entity.budget < bfqq->entity.service); From e7132b1e92d195963988b52a30110b44facba537 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Sun, 5 Feb 2017 22:56:25 +0100 Subject: [PATCH 08/24] BFQ-v8r8-rc2 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 4 ++-- block/bfq.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index f1cb2e07606e..3802db1349eb 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -9,7 +9,7 @@ * * Copyright (C) 2015 Paolo Valente * - * Copyright (C) 2016 Paolo Valente + * Copyright (C) 2017 Paolo Valente * * Licensed under the GPL-2 as detailed in the accompanying COPYING.BFQ * file. @@ -5238,7 +5238,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r8-rc1"; + char msg[60] = "BFQ I/O-scheduler: v8r8-rc2"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index 32e900cccdbd..999b9e2df1f3 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r8-rc1 for 4.10.0: data structures and common functions prototypes. + * BFQ v8r8-rc2 for 4.10.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe @@ -9,7 +9,7 @@ * * Copyright (C) 2015 Paolo Valente * - * Copyright (C) 2016 Paolo Valente + * Copyright (C) 2017 Paolo Valente */ #ifndef _BFQ_H From 3ec936d58fd95e7a9419ff6a1f3ecb8d8a3c4314 Mon Sep 17 00:00:00 2001 From: Oleksandr Natalenko Date: Mon, 20 Feb 2017 16:02:07 +0100 Subject: [PATCH 09/24] block/bfq-cgroup: fix bfq_bic_update_cgroup() API bfq_bic_update_cgroup() should return nothing even if CONFIG_BFQ_GROUP_IOSCHED is disabled. Signed-off-by: Oleksandr Natalenko --- block/bfq-cgroup.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index a5f8dc16900a..de045cff3353 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -1157,13 +1157,7 @@ static void bfq_init_entity(struct bfq_entity *entity, entity->sched_data = &bfqg->sched_data; } -static struct bfq_group * -bfq_bic_update_cgroup(struct bfq_io_cq *bic, struct bio *bio) -{ - struct bfq_data *bfqd = bic_to_bfqd(bic); - - return bfqd->root_group; -} +static void bfq_bic_update_cgroup(struct bfq_io_cq *bic, struct bio *bio) {} static void bfq_end_wr_async(struct bfq_data *bfqd) { From f6553dda1ee2517e2b3e7b01717b06281bd73ab1 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Thu, 23 Feb 2017 12:28:00 +0100 Subject: [PATCH 10/24] BFQ-v8r8 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 3802db1349eb..21197f697dda 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5238,7 +5238,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r8-rc2"; + char msg[60] = "BFQ I/O-scheduler: v8r8"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index 999b9e2df1f3..f433cfed16c3 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r8-rc2 for 4.10.0: data structures and common functions prototypes. + * BFQ v8r8 for 4.10.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe From d5972e7eb3975a07973d0ea69b8c1827b5ba0c4a Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Thu, 16 Mar 2017 14:36:41 +0100 Subject: [PATCH 11/24] BUGFIX: remove use of bfq queues after free bfq queues occasionally happened to be used after being freed, because they were accessed after some invocations of bfq_put_queue that could cause them to be freed. This commit refactors code, when needed, to avoid any occurrence of such a use-after-free of a bfq queue. This commit also adds comments to make references to bfq queues easier to follow. Signed-off-by: Paolo Valente --- block/bfq-cgroup.c | 1 - block/bfq-iosched.c | 36 ++++++++++++++++++--------- block/bfq-sched.c | 71 ++++++++++++++++++++++++++++++++++++----------------- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c index de045cff3353..a66a7232a854 100644 --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -771,7 +771,6 @@ static void bfq_pd_offline(struct blkg_policy_data *pd) __bfq_deactivate_entity(entity, false); bfq_put_async_queues(bfqd, bfqg); - BUG_ON(entity->tree); /* * @blkg is going offline and will be ignored by diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 21197f697dda..f00761bcb6da 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -1391,7 +1391,6 @@ static void bfq_bfqq_handle_idle_busy_switch(struct bfq_data *bfqd, bfq_bfqq_expire(bfqd, bfqd->in_service_queue, false, BFQ_BFQQ_PREEMPTED); - BUG_ON(in_serv->entity.budget < 0); } } @@ -1560,8 +1559,10 @@ static void bfq_remove_request(struct request *rq) BUG_ON(bfqq->entity.budget < 0); if (bfq_bfqq_busy(bfqq) && bfqq != bfqd->in_service_queue) { + BUG_ON(bfqq->ref < 2); /* referred by rq and on tree */ bfq_del_bfqq_busy(bfqd, bfqq, false); - /* bfqq emptied. In normal operation, when + /* + * bfqq emptied. In normal operation, when * bfqq is empty, bfqq->entity.service and * bfqq->entity.budget must contain, * respectively, the service received and the @@ -1570,7 +1571,8 @@ static void bfq_remove_request(struct request *rq) * this last removal occurred while bfqq is * not in service. To avoid inconsistencies, * reset both bfqq->entity.service and - * bfqq->entity.budget. + * bfqq->entity.budget, if bfqq has still a + * process that may issue I/O requests to it. */ bfqq->entity.budget = bfqq->entity.service = 0; } @@ -2062,7 +2064,8 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, new_bfqq->wr_coeff = bfqq->wr_coeff; new_bfqq->wr_cur_max_time = bfqq->wr_cur_max_time; new_bfqq->last_wr_start_finish = bfqq->last_wr_start_finish; - new_bfqq->wr_start_at_switch_to_srt = bfqq->wr_start_at_switch_to_srt; + new_bfqq->wr_start_at_switch_to_srt = + bfqq->wr_start_at_switch_to_srt; if (bfq_bfqq_busy(new_bfqq)) bfqd->wr_busy_queues++; new_bfqq->entity.prio_changed = 1; @@ -2105,6 +2108,7 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, */ new_bfqq->bic = NULL; bfqq->bic = NULL; + /* release process reference to bfqq */ bfq_put_queue(bfqq); } @@ -3077,6 +3081,7 @@ static void bfq_bfqq_expire(struct bfq_data *bfqd, bool slow; unsigned long delta = 0; struct bfq_entity *entity = &bfqq->entity; + int ref; BUG_ON(bfqq != bfqd->in_service_queue); @@ -3184,12 +3189,15 @@ static void bfq_bfqq_expire(struct bfq_data *bfqd, __bfq_bfqq_recalc_budget(bfqd, bfqq, reason); BUG_ON(bfqq->next_rq == NULL && bfqq->entity.budget < bfqq->entity.service); + ref = bfqq->ref; __bfq_bfqq_expire(bfqd, bfqq); - BUG_ON(!bfq_bfqq_busy(bfqq) && reason == BFQ_BFQQ_BUDGET_EXHAUSTED && + BUG_ON(ref > 1 && + !bfq_bfqq_busy(bfqq) && reason == BFQ_BFQQ_BUDGET_EXHAUSTED && !bfq_class_idle(bfqq)); - if (!bfq_bfqq_busy(bfqq) && + /* mark bfqq as waiting a request only if a bic still points to it */ + if (ref > 1 && !bfq_bfqq_busy(bfqq) && reason != BFQ_BFQQ_BUDGET_TIMEOUT && reason != BFQ_BFQQ_BUDGET_EXHAUSTED) bfq_mark_bfqq_non_blocking_wait_rq(bfqq); @@ -3809,7 +3817,8 @@ static int bfq_dispatch_requests(struct request_queue *q, int force) * Task holds one reference to the queue, dropped when task exits. Each rq * in-flight on this queue also holds a reference, dropped when rq is freed. * - * Queue lock must be held here. + * Queue lock must be held here. Recall not to use bfqq after calling + * this function on it. */ static void bfq_put_queue(struct bfq_queue *bfqq) { @@ -3878,7 +3887,7 @@ static void bfq_exit_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq) bfq_put_cooperator(bfqq); - bfq_put_queue(bfqq); + bfq_put_queue(bfqq); /* release process reference */ } static void bfq_init_icq(struct io_cq *icq) @@ -3977,6 +3986,7 @@ static void bfq_check_ioprio_change(struct bfq_io_cq *bic, struct bio *bio) bfqq = bic_to_bfqq(bic, false); if (bfqq) { + /* release process reference on this queue */ bfq_put_queue(bfqq); bfqq = bfq_get_queue(bfqd, bio, BLK_RW_ASYNC, bic); bic_set_bfqq(bic, bfqq, false); @@ -4110,7 +4120,7 @@ static struct bfq_queue *bfq_get_queue(struct bfq_data *bfqd, } out: - bfqq->ref++; + bfqq->ref++; /* get a process reference to this queue */ bfq_log_bfqq(bfqd, bfqq, "get_queue, at end: %p, %d", bfqq, bfqq->ref); rcu_read_unlock(); return bfqq; @@ -4284,10 +4294,14 @@ static void bfq_insert_request(struct request_queue *q, struct request *rq) bfqq->allocated[rq_data_dir(rq)]--; new_bfqq->ref++; bfq_clear_bfqq_just_created(bfqq); - bfq_put_queue(bfqq); if (bic_to_bfqq(RQ_BIC(rq), 1) == bfqq) bfq_merge_bfqqs(bfqd, RQ_BIC(rq), bfqq, new_bfqq); + /* + * rq is about to be enqueued into new_bfqq, + * release rq reference on bfqq + */ + bfq_put_queue(bfqq); rq->elv.priv[1] = new_bfqq; bfqq = new_bfqq; } @@ -4708,7 +4722,7 @@ static void bfq_shutdown_timer_wq(struct bfq_data *bfqd) } static void __bfq_put_async_bfqq(struct bfq_data *bfqd, - struct bfq_queue **bfqq_ptr) + struct bfq_queue **bfqq_ptr) { struct bfq_group *root_group = bfqd->root_group; struct bfq_queue *bfqq = *bfqq_ptr; diff --git a/block/bfq-sched.c b/block/bfq-sched.c index 2e9dc59de0ed..70aac56b02ef 100644 --- a/block/bfq-sched.c +++ b/block/bfq-sched.c @@ -154,7 +154,13 @@ static bool bfq_update_next_in_service(struct bfq_sched_data *sd, #define for_each_entity(entity) \ for (; entity ; entity = entity->parent) -#define for_each_entity_safe(entity, parent) \ +/* + * For each iteration, compute parent in advance, so as to be safe if + * entity is deallocated during the iteration. Such a deallocation may + * happen as a consequence of a bfq_put_queue that frees the bfq_queue + * containing entity. + */ +#define for_each_entity_safe(entity, parent) \ for (; entity && ({ parent = entity->parent; 1; }); entity = parent) /* @@ -691,27 +697,31 @@ static void bfq_idle_insert(struct bfq_service_tree *st, } /** - * bfq_forget_entity - remove an entity from the wfq trees. + * bfq_forget_entity - do not consider entity any longer for scheduling * @st: the service tree. * @entity: the entity being removed. + * @is_in_service: true if entity is currently the in-service entity. * - * Update the device status and forget everything about @entity, putting - * the device reference to it, if it is a queue. Entities belonging to - * groups are not refcounted. + * Forget everything about @entity. In addition, if entity represents + * a queue, and the latter is not in service, then release the service + * reference to the queue (the one taken through bfq_get_entity). In + * fact, in this case, there is really no more service reference to + * the queue, as the latter is also outside any service tree. If, + * instead, the queue is in service, then __bfq_bfqd_reset_in_service + * will take care of putting the reference when the queue finally + * stops being served. */ static void bfq_forget_entity(struct bfq_service_tree *st, - struct bfq_entity *entity) + struct bfq_entity *entity, + bool is_in_service) { struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity); - struct bfq_sched_data *sd; - BUG_ON(!entity->on_st); entity->on_st = false; st->wsum -= entity->weight; - if (bfqq) { - sd = entity->sched_data; - bfq_log_bfqq(bfqq->bfqd, bfqq, "forget_entity: %p %d", + if (bfqq && !is_in_service) { + bfq_log_bfqq(bfqq->bfqd, bfqq, "forget_entity (before): %p %d", bfqq, bfqq->ref); bfq_put_queue(bfqq); } @@ -726,7 +736,8 @@ static void bfq_put_idle_entity(struct bfq_service_tree *st, struct bfq_entity *entity) { bfq_idle_extract(st, entity); - bfq_forget_entity(st, entity); + bfq_forget_entity(st, entity, + entity == entity->sched_data->in_service_entity); } /** @@ -1082,6 +1093,12 @@ static void __bfq_activate_entity(struct bfq_entity *entity, */ entity->start = min_vstart; st->wsum += entity->weight; + /* + * entity is about to be inserted into a service tree, + * and then set in service: get a reference to make + * sure entity does not disappear until it is no + * longer in service or scheduled for service. + */ bfq_get_entity(entity); BUG_ON(entity->on_st && bfqq); @@ -1264,27 +1281,27 @@ static bool __bfq_deactivate_entity(struct bfq_entity *entity, { struct bfq_sched_data *sd = entity->sched_data; struct bfq_service_tree *st = bfq_entity_service_tree(entity); - bool was_in_service = entity == sd->in_service_entity; + bool is_in_service = entity == sd->in_service_entity; if (!entity->on_st) { /* entity never activated, or already inactive */ BUG_ON(entity == entity->sched_data->in_service_entity); return false; } - BUG_ON(was_in_service && entity->tree && entity->tree != &st->active); + BUG_ON(is_in_service && entity->tree && entity->tree != &st->active); - if (was_in_service) + if (is_in_service) bfq_calc_finish(entity, entity->service); if (entity->tree == &st->active) bfq_active_extract(st, entity); - else if (!was_in_service && entity->tree == &st->idle) + else if (!is_in_service && entity->tree == &st->idle) bfq_idle_extract(st, entity); else if (entity->tree) BUG(); if (!ins_into_idle_tree || !bfq_gt(entity->finish, st->vtime)) - bfq_forget_entity(st, entity); + bfq_forget_entity(st, entity, is_in_service); else bfq_idle_insert(st, entity); @@ -1320,8 +1337,8 @@ static void bfq_deactivate_entity(struct bfq_entity *entity, if (!__bfq_deactivate_entity(entity, ins_into_idle_tree)) { /* - * Entity is not any tree any more, so, this - * deactivation is a no-op, and there is + * entity is not in any tree any more, so + * this deactivation is a no-op, and there is * nothing to change for upper-level entities * (in case of expiration, this can never * happen). @@ -1821,14 +1838,16 @@ static struct bfq_queue *bfq_get_next_queue(struct bfq_data *bfqd) static void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) { - struct bfq_entity *entity = &bfqd->in_service_queue->entity; + struct bfq_queue *in_serv_bfqq = bfqd->in_service_queue; + struct bfq_entity *in_serv_entity = &in_serv_bfqq->entity; + struct bfq_entity *entity = in_serv_entity; if (bfqd->in_service_bic) { put_io_context(bfqd->in_service_bic->icq.ioc); bfqd->in_service_bic = NULL; } - bfq_clear_bfqq_wait_request(bfqd->in_service_queue); + bfq_clear_bfqq_wait_request(in_serv_bfqq); hrtimer_try_to_cancel(&bfqd->idle_slice_timer); bfqd->in_service_queue = NULL; @@ -1840,6 +1859,14 @@ static void __bfq_bfqd_reset_in_service(struct bfq_data *bfqd) */ for_each_entity(entity) entity->sched_data->in_service_entity = NULL; + + /* + * in_serv_entity is no longer in service, so, if it is in no + * service tree either, then release the service reference to + * the queue it represents (taken with bfq_get_entity). + */ + if (!in_serv_entity->on_st) + bfq_put_queue(in_serv_bfqq); } static void bfq_deactivate_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq, @@ -1904,8 +1931,6 @@ static void bfq_del_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq, BUG_ON(bfqq->entity.budget < 0); bfq_deactivate_bfqq(bfqd, bfqq, true, expiration); - - BUG_ON(bfqq->entity.budget < 0); } /* From 0a909c1b8fe3422efcb5fde78502e546b633d3b9 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Tue, 21 Mar 2017 09:56:39 -0400 Subject: [PATCH 12/24] BFQ-v8r9 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index f00761bcb6da..1e46d685088b 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5252,7 +5252,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r8"; + char msg[60] = "BFQ I/O-scheduler: v8r9"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index f433cfed16c3..6faed4f2adf4 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r8 for 4.10.0: data structures and common functions prototypes. + * BFQ v8r9 for 4.10.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe From 9b86b6f28e426e634b38b34be7aba749a6c8f409 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Tue, 11 Apr 2017 12:28:10 +0200 Subject: [PATCH 13/24] BUGFIX: Handle failure of weight-counter allocation Reported-by: Bart Van Assche Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 1e46d685088b..bbfb5d656fbe 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -483,6 +483,22 @@ static void bfq_weights_tree_add(struct bfq_data *bfqd, entity->weight_counter = kzalloc(sizeof(struct bfq_weight_counter), GFP_ATOMIC); + + /* + * In the unlucky event of an allocation failure, we just + * exit. This will cause the weight of entity to not be + * considered in bfq_differentiated_weights, which, in its + * turn, causes the scenario to be deemed wrongly symmetric in + * case entity's weight would have been the only weight making + * the scenario asymmetric. On the bright side, no unbalance + * will however occur when entity becomes inactive again (the + * invocation of this function is triggered by an activation + * of entity). In fact, bfq_weights_tree_remove does nothing + * if !entity->weight_counter. + */ + if (unlikely(!entity->weight_counter)) + return; + entity->weight_counter->weight = entity->weight; rb_link_node(&entity->weight_counter->weights_node, parent, new); rb_insert_color(&entity->weight_counter->weights_node, root); From 9fdfc0417af29166b0d8045bf55e2d79d1f0d2c2 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Tue, 11 Apr 2017 12:32:03 +0200 Subject: [PATCH 14/24] BUGFIX: Remove problematic check on max service duration Reported-by: Bart Van Assche Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index bbfb5d656fbe..445456aac7ff 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -2934,8 +2934,8 @@ static bool bfq_bfqq_is_slow(struct bfq_data *bfqd, struct bfq_queue *bfqq, delta_ktime = ktime_sub(delta_ktime, bfqd->last_budget_start); delta_usecs = ktime_to_us(delta_ktime); - /* don't trust short/unrealistic values. */ - if (delta_usecs < 1000 || delta_usecs >= LONG_MAX) { + /* don't use too short time intervals */ + if (delta_usecs < 1000) { if (blk_queue_nonrot(bfqd->queue)) /* * give same worst-case guarantees as idling @@ -2945,7 +2945,7 @@ static bool bfq_bfqq_is_slow(struct bfq_data *bfqd, struct bfq_queue *bfqq, else /* charge at least one seek */ *delta_ms = bfq_slice_idle / NSEC_PER_MSEC; - bfq_log(bfqd, "bfq_bfqq_is_slow: unrealistic %u", delta_usecs); + bfq_log(bfqd, "bfq_bfqq_is_slow: too short %u", delta_usecs); return slow; } From 10efd6662a26b36bd605994be6a49e8044f955e3 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Tue, 11 Apr 2017 12:35:09 +0200 Subject: [PATCH 15/24] BFQ-v8r10-rc1 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 445456aac7ff..a7a619771d6e 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5268,7 +5268,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r9"; + char msg[60] = "BFQ I/O-scheduler: v8r10-rc1"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index 6faed4f2adf4..259568b4e1cd 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r9 for 4.10.0: data structures and common functions prototypes. + * BFQ v8r10-rc1 for 4.11.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe From 02f8cb3db04551904017bd40170bd8b1f46d220a Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Thu, 20 Apr 2017 09:46:53 +0200 Subject: [PATCH 16/24] BFQ-v8r10 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index a7a619771d6e..96b79a0dae9f 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5268,7 +5268,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r10-rc1"; + char msg[60] = "BFQ I/O-scheduler: v8r10"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index 259568b4e1cd..e3855af1211b 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r10-rc1 for 4.11.0: data structures and common functions prototypes. + * BFQ v8r10 for 4.9.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe From ba3240c4e9a0d324c4a8115b6fce7265afdda78f Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Wed, 26 Apr 2017 19:14:04 +0200 Subject: [PATCH 17/24] block, bfq: use pointer entity->sched_data only if set In the function __bfq_deactivate_entity, the pointer entity->sched_data could happen to be used before being properly initialized. This led to a NULL pointer dereference. This commit fixes this bug by just using this pointer only where it is safe to do so. Reported-by: Tom Harrison Tested-by: Tom Harrison Signed-off-by: Paolo Valente --- block/bfq-sched.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/block/bfq-sched.c b/block/bfq-sched.c index 70aac56b02ef..8311bdbeceea 100644 --- a/block/bfq-sched.c +++ b/block/bfq-sched.c @@ -1280,14 +1280,23 @@ static bool __bfq_deactivate_entity(struct bfq_entity *entity, bool ins_into_idle_tree) { struct bfq_sched_data *sd = entity->sched_data; - struct bfq_service_tree *st = bfq_entity_service_tree(entity); - bool is_in_service = entity == sd->in_service_entity; + struct bfq_service_tree *st; + bool is_in_service; if (!entity->on_st) { /* entity never activated, or already inactive */ - BUG_ON(entity == entity->sched_data->in_service_entity); + BUG_ON(sd && entity == sd->in_service_entity); return false; } + /* + * If we get here, then entity is active, which implies that + * bfq_group_set_parent has already been invoked for the group + * represented by entity. Therefore, the field + * entity->sched_data has been set, and we can safely use it. + */ + st = bfq_entity_service_tree(entity); + is_in_service = entity == sd->in_service_entity; + BUG_ON(is_in_service && entity->tree && entity->tree != &st->active); if (is_in_service) From 3057823d0979d5ae6b9bab0142c79d90b6535036 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Thu, 20 Apr 2017 15:07:18 +0100 Subject: [PATCH 18/24] block, bfq: don't dereference bic before null checking it The call to bfq_check_ioprio_change will dereference bic, however, the null check for bic is after this call. Move the the null check on bic to before the call to avoid any potential null pointer dereference issues. Detected by CoverityScan, CID#1430138 ("Dereference before null check") Signed-off-by: Colin Ian King Signed-off-by: Jens Axboe --- block/bfq-iosched.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 96b79a0dae9f..4aa8ff5b3bb5 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -4572,6 +4572,8 @@ static int bfq_set_request(struct request_queue *q, struct request *rq, if (!bic) goto queue_fail; + bfq_check_ioprio_change(bic, bio); + bfq_bic_update_cgroup(bic, bio); new_queue: From d231bcc3154bb0036123fb4aa487bc75daf43aa5 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Mon, 1 May 2017 19:15:43 +0200 Subject: [PATCH 19/24] BFQ-v8r11 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 4aa8ff5b3bb5..158628392d92 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5270,7 +5270,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r10"; + char msg[60] = "BFQ I/O-scheduler: v8r11"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index e3855af1211b..a521ac40cac0 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r10 for 4.9.0: data structures and common functions prototypes. + * BFQ v8r11 for 4.9.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe From 463bdf6f4bf02080f52de8987ca61b8b16c059bf Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Thu, 4 May 2017 17:47:13 +0200 Subject: [PATCH 20/24] block, bfq: update wr_busy_queues if needed on a queue split This commit fixes a bug triggered by a non-trivial sequence of events. These events are briefly described in the next two paragraphs. The impatiens, or those who are familiar with queue merging and splitting, can jump directly to the last paragraph. On each I/O-request arrival for a shared bfq_queue, i.e., for a bfq_queue that is the result of the merge of two or more bfq_queues, BFQ checks whether the shared bfq_queue has become seeky (i.e., if too many random I/O requests have arrived for the bfq_queue; if the device is non rotational, then random requests must be also small for the bfq_queue to be tagged as seeky). If the shared bfq_queue is actually detected as seeky, then a split occurs: the bfq I/O context of the process that has issued the request is redirected from the shared bfq_queue to a new non-shared bfq_queue. As a degenerate case, if the shared bfq_queue actually happens to be shared only by one process (because of previous splits), then no new bfq_queue is created: the state of the shared bfq_queue is just changed from shared to non shared. Regardless of whether a brand new non-shared bfq_queue is created, or the pre-existing shared bfq_queue is just turned into a non-shared bfq_queue, several parameters of the non-shared bfq_queue are set (restored) to the original values they had when the bfq_queue associated with the bfq I/O context of the process (that has just issued an I/O request) was merged with the shared bfq_queue. One of these parameters is the weight-raising state. If, on the split of a shared bfq_queue, 1) a pre-existing shared bfq_queue is turned into a non-shared bfq_queue; 2) the previously shared bfq_queue happens to be busy; 3) the weight-raising state of the previously shared bfq_queue happens to change; the number of weight-raised busy queues changes. The field wr_busy_queues must then be updated accordingly, but such an update was missing. This commit adds the missing update. Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 47 +++++++++++++++++++++++++++++++++++++++-------- block/bfq-sched.c | 9 +++++++-- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 158628392d92..9c7d6c4fe2f8 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -682,8 +682,12 @@ static unsigned int bfq_wr_duration(struct bfq_data *bfqd) } static void -bfq_bfqq_resume_state(struct bfq_queue *bfqq, struct bfq_io_cq *bic) +bfq_bfqq_resume_state(struct bfq_queue *bfqq, struct bfq_data *bfqd, + struct bfq_io_cq *bic, bool bfq_already_existing) { + unsigned int old_wr_coeff; + bool busy = bfq_already_existing && bfq_bfqq_busy(bfqq); + if (bic->saved_idle_window) bfq_mark_bfqq_idle_window(bfqq); else @@ -694,6 +698,9 @@ bfq_bfqq_resume_state(struct bfq_queue *bfqq, struct bfq_io_cq *bic) else bfq_clear_bfqq_IO_bound(bfqq); + if (unlikely(busy)) + old_wr_coeff = bfqq->wr_coeff; + bfqq->wr_coeff = bic->saved_wr_coeff; bfqq->wr_start_at_switch_to_srt = bic->saved_wr_start_at_switch_to_srt; BUG_ON(time_is_after_jiffies(bfqq->wr_start_at_switch_to_srt)); @@ -702,8 +709,8 @@ bfq_bfqq_resume_state(struct bfq_queue *bfqq, struct bfq_io_cq *bic) BUG_ON(time_is_after_jiffies(bfqq->last_wr_start_finish)); if (bfqq->wr_coeff > 1 && (bfq_bfqq_in_large_burst(bfqq) || - time_is_before_jiffies(bfqq->last_wr_start_finish + - bfqq->wr_cur_max_time))) { + time_is_before_jiffies(bfqq->last_wr_start_finish + + bfqq->wr_cur_max_time))) { bfq_log_bfqq(bfqq->bfqd, bfqq, "resume state: switching off wr (%lu + %lu < %lu)", bfqq->last_wr_start_finish, bfqq->wr_cur_max_time, @@ -711,8 +718,20 @@ bfq_bfqq_resume_state(struct bfq_queue *bfqq, struct bfq_io_cq *bic) bfqq->wr_coeff = 1; } + /* make sure weight will be updated, however we got here */ bfqq->entity.prio_changed = 1; + + if (likely(!busy)) + return; + + if (old_wr_coeff == 1 && bfqq->wr_coeff > 1) { + bfqd->wr_busy_queues++; + BUG_ON(bfqd->wr_busy_queues > bfqd->busy_queues); + } else if (old_wr_coeff > 1 && bfqq->wr_coeff == 1) { + bfqd->wr_busy_queues--; + BUG_ON(bfqd->wr_busy_queues < 0); + } } static int bfqq_process_refs(struct bfq_queue *bfqq) @@ -1460,6 +1479,7 @@ static void bfq_add_request(struct request *rq) bfqq->wr_cur_max_time = bfq_wr_duration(bfqd); bfqd->wr_busy_queues++; + BUG_ON(bfqd->wr_busy_queues > bfqd->busy_queues); bfqq->entity.prio_changed = 1; bfq_log_bfqq(bfqd, bfqq, "non-idle wrais starting, " @@ -1702,8 +1722,10 @@ static void bfq_bfqq_end_wr(struct bfq_queue *bfqq) { BUG_ON(!bfqq); - if (bfq_bfqq_busy(bfqq)) + if (bfq_bfqq_busy(bfqq)) { bfqq->bfqd->wr_busy_queues--; + BUG_ON(bfqq->bfqd->wr_busy_queues < 0); + } bfqq->wr_coeff = 1; bfqq->wr_cur_max_time = 0; bfqq->last_wr_start_finish = jiffies; @@ -2082,8 +2104,11 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, new_bfqq->last_wr_start_finish = bfqq->last_wr_start_finish; new_bfqq->wr_start_at_switch_to_srt = bfqq->wr_start_at_switch_to_srt; - if (bfq_bfqq_busy(new_bfqq)) + if (bfq_bfqq_busy(new_bfqq)) { bfqd->wr_busy_queues++; + BUG_ON(bfqd->wr_busy_queues > bfqd->busy_queues); + } + new_bfqq->entity.prio_changed = 1; bfq_log_bfqq(bfqd, new_bfqq, "wr start after merge with %d, rais_max_time %u", @@ -2094,8 +2119,11 @@ bfq_merge_bfqqs(struct bfq_data *bfqd, struct bfq_io_cq *bic, if (bfqq->wr_coeff > 1) { /* bfqq has given its wr to new_bfqq */ bfqq->wr_coeff = 1; bfqq->entity.prio_changed = 1; - if (bfq_bfqq_busy(bfqq)) + if (bfq_bfqq_busy(bfqq)) { bfqd->wr_busy_queues--; + BUG_ON(bfqd->wr_busy_queues < 0); + } + } bfq_log_bfqq(bfqd, new_bfqq, "merge_bfqqs: wr_busy %d", @@ -4564,7 +4592,7 @@ static int bfq_set_request(struct request_queue *q, struct request *rq, const int is_sync = rq_is_sync(rq); struct bfq_queue *bfqq; unsigned long flags; - bool split = false; + bool bfqq_already_existing = false, split = false; spin_lock_irqsave(q->queue_lock, flags); bfq_check_ioprio_change(bic, bio); @@ -4624,6 +4652,8 @@ static int bfq_set_request(struct request_queue *q, struct request *rq, split = true; if (!bfqq) goto new_queue; + else + bfqq_already_existing = true; } } @@ -4649,7 +4679,8 @@ static int bfq_set_request(struct request_queue *q, struct request *rq, * queue, restore the idle window and the possible * weight raising period. */ - bfq_bfqq_resume_state(bfqq, bic); + bfq_bfqq_resume_state(bfqq, bfqd, bic, + bfqq_already_existing); } } diff --git a/block/bfq-sched.c b/block/bfq-sched.c index 8311bdbeceea..d8efd6bad965 100644 --- a/block/bfq-sched.c +++ b/block/bfq-sched.c @@ -1932,8 +1932,10 @@ static void bfq_del_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq, bfq_weights_tree_remove(bfqd, &bfqq->entity, &bfqd->queue_weights_tree); - if (bfqq->wr_coeff > 1) + if (bfqq->wr_coeff > 1) { bfqd->wr_busy_queues--; + BUG_ON(bfqd->wr_busy_queues < 0); + } bfqg_stats_update_dequeue(bfqq_group(bfqq)); @@ -1962,6 +1964,9 @@ static void bfq_add_bfqq_busy(struct bfq_data *bfqd, struct bfq_queue *bfqq) bfq_weights_tree_add(bfqd, &bfqq->entity, &bfqd->queue_weights_tree); - if (bfqq->wr_coeff > 1) + if (bfqq->wr_coeff > 1) { bfqd->wr_busy_queues++; + BUG_ON(bfqd->wr_busy_queues > bfqd->busy_queues); + } + } From 671d03329dc0fbbac634f4fcfee11be26b5cccb6 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Tue, 9 May 2017 12:52:16 +0200 Subject: [PATCH 21/24] block, bfq: stress that low_latency must be off to get max throughput The introduction of the BFQ and Kyber I/O schedulers has triggered a new wave of I/O benchmarks. Unfortunately, comments and discussions on these benchmarks confirm that there is still little awareness that it is very hard to achieve, at the same time, a low latency and a high throughput. In particular, virtually all benchmarks measure throughput, or throughput-related figures of merit, but, for BFQ, they use the scheduler in its default configuration. This configuration is geared, instead, toward a low latency. This is evidently a sign that BFQ documentation is still too unclear on this important aspect. This commit addresses this issue by stressing how BFQ configuration must be (easily) changed if the only goal is maximum throughput. Signed-off-by: Paolo Valente --- Documentation/block/bfq-iosched.txt | 17 ++++++++++++++++- block/bfq-iosched.c | 5 +++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/Documentation/block/bfq-iosched.txt b/Documentation/block/bfq-iosched.txt index 13b5248eba7e..0539e87962ed 100644 --- a/Documentation/block/bfq-iosched.txt +++ b/Documentation/block/bfq-iosched.txt @@ -11,6 +11,13 @@ controllers), BFQ's main features are: groups (switching back to time distribution when needed to keep throughput high). +In its default configuration, BFQ privileges latency over +throughput. So, when needed for achieving a lower latency, BFQ builds +schedules that may lead to a lower throughput. If your main or only +goal, for a given device, is to achieve the maximum-possible +throughput at all times, then do switch off all low-latency heuristics +for that device, by setting low_latency to 0. Full details in Section 3. + On average CPUs, the current version of BFQ can handle devices performing at most ~30K IOPS; at most ~50 KIOPS on faster CPUs. As a reference, 30-50 KIOPS correspond to very high bandwidths with @@ -374,11 +381,19 @@ default, low latency mode is enabled. If enabled, interactive and soft real-time applications are privileged and experience a lower latency, as explained in more detail in the description of how BFQ works. -DO NOT enable this mode if you need full control on bandwidth +DISABLE this mode if you need full control on bandwidth distribution. In fact, if it is enabled, then BFQ automatically increases the bandwidth share of privileged applications, as the main means to guarantee a lower latency to them. +In addition, as already highlighted at the beginning of this document, +DISABLE this mode if your only goal is to achieve a high throughput. +In fact, privileging the I/O of some application over the rest may +entail a lower throughput. To achieve the highest-possible throughput +on a non-rotational device, setting slice_idle to 0 may be needed too +(at the cost of giving up any strong guarantee on fairness and low +latency). + timeout_sync ------------ diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 9c7d6c4fe2f8..82b1545e655c 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -36,6 +36,11 @@ * boost the throughput), and yet guarantee a low latency to * interactive and soft real-time applications. * + * NOTE: if the main or only goal, with a given device, is to achieve + * the maximum-possible throughput at all times, then do switch off + * all low-latency heuristics for that device, by setting low_latency + * to 0. + * * BFQ is described in [1], where also a reference to the initial, more * theoretical paper on BFQ can be found. The interested reader can find * in the latter paper full details on the main algorithm, as well as From 5da1dcad659cf3c043c9b062f8800060147dcf5a Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Fri, 12 May 2017 10:19:52 +0200 Subject: [PATCH 22/24] Fix commit "don't dereference bic before null checking it" Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 1 - 1 file changed, 1 deletion(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 82b1545e655c..877a5068e2c1 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -4600,7 +4600,6 @@ static int bfq_set_request(struct request_queue *q, struct request *rq, bool bfqq_already_existing = false, split = false; spin_lock_irqsave(q->queue_lock, flags); - bfq_check_ioprio_change(bic, bio); if (!bic) goto queue_fail; From 5ca5a4edf9a316c92da3f6917de6efbeb474be96 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Wed, 14 Jun 2017 15:35:44 +0200 Subject: [PATCH 23/24] block, bfq: don't change ioprio class for a bfq_queue on a service tree On each deactivation or re-scheduling (after being served) of a bfq_queue, BFQ invokes the function __bfq_entity_update_weight_prio(), to perform pending updates of ioprio, weight and ioprio class for the bfq_queue. BFQ also invokes this function on I/O-request dispatches, to raise or lower weights more quickly when needed, thereby improving latency. However, the entity representing the bfq_queue may be on the active (sub)tree of a service tree when this happens, and, although with a very low probability, the bfq_queue may happen to also have a pending change of its ioprio class. If both conditions hold when __bfq_entity_update_weight_prio() is invoked, then the entity moves to a sort of hybrid state: the new service tree for the entity, as returned by bfq_entity_service_tree(), differs from service tree on which the entity still is. The functions that handle activations and deactivations of entities do not cope with such a hybrid state (and would need to become more complex to cope). This commit addresses this issue by just making __bfq_entity_update_weight_prio() not perform also a possible pending change of ioprio class, when invoked on an I/O-request dispatch for a bfq_queue. Such a change is thus postponed to when __bfq_entity_update_weight_prio() is invoked on deactivation or re-scheduling of the bfq_queue. Reported-by: Marco Piazza Reported-by: Laurentiu Nicola Signed-off-by: Paolo Valente Tested-by: Marco Piazza --- block/bfq-iosched.c | 14 ++++++++++---- block/bfq-sched.c | 38 ++++++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 877a5068e2c1..572d007f6981 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -3701,11 +3701,17 @@ static void bfq_update_wr_data(struct bfq_data *bfqd, struct bfq_queue *bfqq) } } } - /* Update weight both if it must be raised and if it must be lowered */ + /* + * To improve latency (for this or other queues), immediately + * update weight both if it must be raised and if it must be + * lowered. Since, entity may be on some active tree here, and + * might have a pending change of its ioprio class, invoke + * next function with the last parameter unset (see the + * comments on the function). + */ if ((entity->weight > entity->orig_weight) != (bfqq->wr_coeff > 1)) - __bfq_entity_update_weight_prio( - bfq_entity_service_tree(entity), - entity); + __bfq_entity_update_weight_prio(bfq_entity_service_tree(entity), + entity, false); } /* diff --git a/block/bfq-sched.c b/block/bfq-sched.c index d8efd6bad965..90d2856358a1 100644 --- a/block/bfq-sched.c +++ b/block/bfq-sched.c @@ -765,9 +765,28 @@ static void bfq_forget_idle(struct bfq_service_tree *st) bfq_put_idle_entity(st, first_idle); } +/* + * Update weight and priority of entity. If update_class_too is true, + * then update the ioprio_class of entity too. + * + * The reason why the update of ioprio_class is controlled through the + * last parameter is as follows. Changing the ioprio class of an + * entity implies changing the destination service trees for that + * entity. If such a change occurred when the entity is already on one + * of the service trees for its previous class, then the state of the + * entity would become more complex: none of the new possible service + * trees for the entity, according to bfq_entity_service_tree(), would + * match any of the possible service trees on which the entity + * is. Complex operations involving these trees, such as entity + * activations and deactivations, should take into account this + * additional complexity. To avoid this issue, this function is + * invoked with update_class_too unset in the points in the code where + * entity may happen to be on some tree. + */ static struct bfq_service_tree * __bfq_entity_update_weight_prio(struct bfq_service_tree *old_st, - struct bfq_entity *entity) + struct bfq_entity *entity, + bool update_class_too) { struct bfq_service_tree *new_st = old_st; @@ -812,9 +831,15 @@ __bfq_entity_update_weight_prio(struct bfq_service_tree *old_st, bfq_weight_to_ioprio(entity->orig_weight); } - if (bfqq) + if (bfqq && update_class_too) bfqq->ioprio_class = bfqq->new_ioprio_class; - entity->prio_changed = 0; + + /* + * Reset prio_changed only if the ioprio_class change + * is not pending any longer. + */ + if (!bfqq || bfqq->ioprio_class == bfqq->new_ioprio_class) + entity->prio_changed = 0; /* * NOTE: here we may be changing the weight too early, @@ -963,7 +988,12 @@ static void bfq_update_fin_time_enqueue(struct bfq_entity *entity, struct bfq_queue *bfqq = bfq_entity_to_bfqq(entity); struct bfq_sched_data *sd = entity->sched_data; - st = __bfq_entity_update_weight_prio(st, entity); + /* + * When this function is invoked, entity is not in any service + * tree, then it is safe to invoke next function with the last + * parameter set (see the comments on the function). + */ + st = __bfq_entity_update_weight_prio(st, entity, true); bfq_calc_finish(entity, entity->budget); /* From 7994bcf9f58d20baa8fbab76d28ab2de6fe19e27 Mon Sep 17 00:00:00 2001 From: Paolo Valente Date: Mon, 3 Jul 2017 22:00:29 +0200 Subject: [PATCH 24/24] BFQ-v8r12 Signed-off-by: Paolo Valente --- block/bfq-iosched.c | 2 +- block/bfq.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c index 572d007f6981..d15ef0b87d12 100644 --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5311,7 +5311,7 @@ static struct blkcg_policy blkcg_policy_bfq = { static int __init bfq_init(void) { int ret; - char msg[60] = "BFQ I/O-scheduler: v8r11"; + char msg[60] = "BFQ I/O-scheduler: v8r12"; #ifdef CONFIG_BFQ_GROUP_IOSCHED ret = blkcg_policy_register(&blkcg_policy_bfq); diff --git a/block/bfq.h b/block/bfq.h index a521ac40cac0..6269fd7ac5ae 100644 --- a/block/bfq.h +++ b/block/bfq.h @@ -1,5 +1,5 @@ /* - * BFQ v8r11 for 4.9.0: data structures and common functions prototypes. + * BFQ v8r12 for 4.9.0: data structures and common functions prototypes. * * Based on ideas and code from CFQ: * Copyright (C) 2003 Jens Axboe