Hi, this series contain two patches to fix bug in request injection
mechanism and some random cleancode patches.
Thanks!
---
V4:
-Thanks Jan and Damien for review. Collect Reviewed-by from Jan and
Damien. Now all patches get Reviewed-by tag.
-Make patches based on latest code and fix conflicts of patch 1/8
"block, bfq: correctly raise inject limit in bfq_choose_bfqq_for_injection"
and 5/8 "block, bfq: remove unnecessary dereference to get async_bfqq"
---
V3:
-Thanks Jan for review. Remove unnecessary brace in patch "block, bfq:
remove unnecessary goto tag in bfq_dispatch_rq_from_bfqq" according to
recommend from Jan and collect Reviewed-by tag from Jan for rest
patches.
-Drop patch "block, bfq: remove redundant bfqd->rq_in_driver > 0 check
in bfq_add_request" and "block, bfq: remove check of
bfq_wr_max_softrt_rate which is always greater than 0".
---
v2:
-improve git log.
---
Kemeng Shi (8):
block, bfq: correctly raise inject limit in
bfq_choose_bfqq_for_injection
block, bfq: remove unsed parameter reason in bfq_bfqq_is_slow
block, bfq: initialize bfqq->decrease_time_jif correctly
block, bfq: use helper macro RQ_BFQQ to get bfqq of request
block, bfq: remove unnecessary dereference to get async_bfqq
block, bfq: remove redundant check in bfq_put_cooperator
block, bfq: remove unnecessary goto tag in bfq_dispatch_rq_from_bfqq
block, bfq: remove unused bfq_wr_max_time in struct bfq_data
block/bfq-iosched.c | 40 +++++++++++++++-------------------------
block/bfq-iosched.h | 2 --
2 files changed, 15 insertions(+), 27 deletions(-)
--
2.30.0
The async_bfqq is assigned with bfqq->bic->bfqq[0], use it directly.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
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 6f38a0130034..4a17b22327f1 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4989,7 +4989,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd)
icq_to_bic(async_bfqq->next_rq->elv.icq) == bfqq->bic &&
bfq_serv_to_charge(async_bfqq->next_rq, async_bfqq) <=
bfq_bfqq_budget_left(async_bfqq))
- bfqq = bfqq->bic->bfqq[0][act_idx];
+ bfqq = async_bfqq;
else if (bfqq->waker_bfqq &&
bfq_bfqq_busy(bfqq->waker_bfqq) &&
bfqq->waker_bfqq->next_rq &&
--
2.30.0
bfqd->bfq_wr_max_time is set to 0 in bfq_init_queue and is never changed.
It is only used in bfq_wr_duration when bfq_wr_max_time > 0 which never
meets, so bfqd->bfq_wr_max_time is not used actually. Just remove it.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
block/bfq-iosched.c | 4 ----
block/bfq-iosched.h | 2 --
2 files changed, 6 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 0416dfe05983..4705c4be90e7 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1093,9 +1093,6 @@ static unsigned int bfq_wr_duration(struct bfq_data *bfqd)
{
u64 dur;
- if (bfqd->bfq_wr_max_time > 0)
- return bfqd->bfq_wr_max_time;
-
dur = bfqd->rate_dur_prod;
do_div(dur, bfqd->peak_rate);
@@ -7299,7 +7296,6 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
*/
bfqd->bfq_wr_coeff = 30;
bfqd->bfq_wr_rt_max_time = msecs_to_jiffies(300);
- bfqd->bfq_wr_max_time = 0;
bfqd->bfq_wr_min_idle_time = msecs_to_jiffies(2000);
bfqd->bfq_wr_min_inter_arr_async = msecs_to_jiffies(500);
bfqd->bfq_wr_max_softrt_rate = 7000; /*
diff --git a/block/bfq-iosched.h b/block/bfq-iosched.h
index 058af701bbbe..ed2be7d32151 100644
--- a/block/bfq-iosched.h
+++ b/block/bfq-iosched.h
@@ -769,8 +769,6 @@ struct bfq_data {
* is multiplied.
*/
unsigned int bfq_wr_coeff;
- /* maximum duration of a weight-raising period (jiffies) */
- unsigned int bfq_wr_max_time;
/* Maximum weight-raising duration for soft real-time processes */
unsigned int bfq_wr_rt_max_time;
--
2.30.0
Use helper macro RQ_BFQQ to get bfqq of request.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
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 698c5918ad10..6f38a0130034 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -6859,14 +6859,14 @@ static struct bfq_queue *bfq_init_rq(struct request *rq)
return NULL;
/*
- * Assuming that elv.priv[1] is set only if everything is set
+ * Assuming that RQ_BFQQ(rq) is set only if everything is set
* for this rq. This holds true, because this function is
* invoked only for insertion or merging, and, after such
* events, a request cannot be manipulated any longer before
* being removed from bfq.
*/
- if (rq->elv.priv[1])
- return rq->elv.priv[1];
+ if (RQ_BFQQ(rq))
+ return RQ_BFQQ(rq);
bic = icq_to_bic(rq->elv.icq);
--
2.30.0
Inject limit is updated or reset when time_is_before_eq_jiffies(
decrease_time_jif + several msecs) or think-time state changes.
decrease_time_jif is initialized to 0 and will be set to current jiffies
when inject limit is updated or reset. If the jiffies is slightly greater
than LONG_MAX, time_is_after_eq_jiffies(0) will keep for a long time, so as
time_is_after_eq_jiffies(decrease_time_jif + several msecs). If the
think-time state never chages, then the injection will not work as expected
for long time.
To be more specific:
Function bfq_update_inject_limit maybe triggered when jiffies pasts
decrease_time_jif + msecs_to_jiffies(10) in bfq_add_request by setting
bfqd->wait_dispatch to true.
Function bfq_reset_inject_limit are called in two conditions:
1. jiffies pasts bfqq->decrease_time_jif + msecs_to_jiffies(1000) in
function bfq_add_request.
2. jiffies pasts bfqq->decrease_time_jif + msecs_to_jiffies(100) or
bfq think-time state change from short to long.
Fix this by initializing bfqq->decrease_time_jif to current jiffies
to trigger service injection soon when service injection conditions
are met.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
block/bfq-iosched.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index f038b4d16d86..698c5918ad10 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5654,6 +5654,8 @@ static void bfq_init_bfqq(struct bfq_data *bfqd, struct bfq_queue *bfqq,
/* first request is almost certainly seeky */
bfqq->seek_history = 1;
+
+ bfqq->decrease_time_jif = jiffies;
}
static struct bfq_queue **bfq_async_queue_prio(struct bfq_data *bfqd,
--
2.30.0
Function bfq_choose_bfqq_for_injection may temporarily raise inject limit
to one request if current inject_limit is 0 before search of the source
queue for injection. However the search below will reset inject limit to
bfqd->in_service_queue which is zero for raised inject limit. Then the
temporarily raised inject limit never works as expected.
Assigment limit to bfqd->in_service_queue in search is needed as limit
maybe overwriten to min_t(unsigned int, 1, limit) for condition that
a large in-flight request is on non-rotational devices in found queue.
So we need to reset limit to bfqd->in_service_queue for normal case.
Actually, we have already make sure bfqd->rq_in_driver is < limit before
search, then
-Limit is >= 1 as bfqd->rq_in_driver is >= 0. Then min_t(unsigned int,
1, limit) is always 1. So we can simply check bfqd->rq_in_driver with
1 instead of result of min_t(unsigned int, 1, limit) for larget request in
non-rotational device case to avoid overwritting limit and the bug is gone.
-For normal case, we have already check bfqd->rq_in_driver is < limit,
so we can return found bfqq unconditionally to remove unncessary check.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
block/bfq-iosched.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 815b884d6c5a..8836221a2673 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4730,12 +4730,10 @@ bfq_choose_bfqq_for_injection(struct bfq_data *bfqd)
*/
if (blk_queue_nonrot(bfqd->queue) &&
blk_rq_sectors(bfqq->next_rq) >=
- BFQQ_SECT_THR_NONROT)
- limit = min_t(unsigned int, 1, limit);
- else
- limit = in_serv_bfqq->inject_limit;
-
- if (bfqd->tot_rq_in_driver < limit) {
+ BFQQ_SECT_THR_NONROT &&
+ bfqd->tot_rq_in_driver >= 1)
+ continue;
+ else {
bfqd->rqs_injected = true;
return bfqq;
}
--
2.30.0
We have already avoided a circular list in bfq_setup_merge (see comments
in bfq_setup_merge() for details), so bfq_queue will not appear in it's
new_bfqq list. Just remove this check.
Signed-off-by: Kemeng Shi <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
---
block/bfq-iosched.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 4a17b22327f1..dbee5c61830c 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5429,8 +5429,6 @@ void bfq_put_cooperator(struct bfq_queue *bfqq)
*/
__bfqq = bfqq->new_bfqq;
while (__bfqq) {
- if (__bfqq == bfqq)
- break;
next = __bfqq->new_bfqq;
bfq_put_queue(__bfqq);
__bfqq = next;
--
2.30.0
We jump to tag only for returning current rq. Return directly to
remove this tag.
Reviewed-by: Jan Kara <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
Signed-off-by: Kemeng Shi <[email protected]>
---
block/bfq-iosched.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index dbee5c61830c..0416dfe05983 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5120,7 +5120,7 @@ static struct request *bfq_dispatch_rq_from_bfqq(struct bfq_data *bfqd,
bfq_dispatch_remove(bfqd->queue, rq);
if (bfqq != bfqd->in_service_queue)
- goto return_rq;
+ return rq;
/*
* If weight raising has to terminate for bfqq, then next
@@ -5140,12 +5140,9 @@ static struct request *bfq_dispatch_rq_from_bfqq(struct bfq_data *bfqd,
* belongs to CLASS_IDLE and other queues are waiting for
* service.
*/
- if (!(bfq_tot_busy_queues(bfqd) > 1 && bfq_class_idle(bfqq)))
- goto return_rq;
+ if (bfq_tot_busy_queues(bfqd) > 1 && bfq_class_idle(bfqq))
+ bfq_bfqq_expire(bfqd, bfqq, false, BFQQE_BUDGET_EXHAUSTED);
- bfq_bfqq_expire(bfqd, bfqq, false, BFQQE_BUDGET_EXHAUSTED);
-
-return_rq:
return rq;
}
--
2.30.0
On Mon, 16 Jan 2023 17:51:45 +0800, Kemeng Shi wrote:
> mechanism and some random cleancode patches.
> Thanks!
>
Applied, thanks!
[1/8] block, bfq: correctly raise inject limit in bfq_choose_bfqq_for_injection
commit: 0c3e09e8854bcd3f7c45de85007ed283342b3464
[2/8] block, bfq: remove unsed parameter reason in bfq_bfqq_is_slow
commit: bebeb9e582e8040944b12942ccc56f4ebacaa9f8
[3/8] block, bfq: initialize bfqq->decrease_time_jif correctly
commit: 1c970450a7fd8be0298758c4e2c631e4a739292d
[4/8] block, bfq: use helper macro RQ_BFQQ to get bfqq of request
commit: 8ac2e43c3559f29513377df8aff7a22a8277fcf8
[5/8] block, bfq: remove unnecessary dereference to get async_bfqq
commit: 86f8382e6d3a74f783c23a3d773285e2637b8bc2
[6/8] block, bfq: remove redundant check in bfq_put_cooperator
commit: 433d4b03e722bdfb1b6a75563cb45e8dca6784e7
[7/8] block, bfq: remove unnecessary goto tag in bfq_dispatch_rq_from_bfqq
commit: 87c971de8157b90494490d7c869a21b7f2123305
[8/8] block, bfq: remove unused bfq_wr_max_time in struct bfq_data
commit: 323745a3aa9ba172582d4549689146298fb68405
Best regards,
--
Jens Axboe