[PATCH 0/4] block, bfq: series of improvements and small fixes of the injection mechanism

[PATCH 0/4] block, bfq: series of improvements and small fixes of the injection mechanism

[Paolo Valente]

Hi Jens,
this patch series makes the injection mechanism better at preserving
control on I/O.

Thanks,
Paolo

Paolo Valente (4):
  block, bfq: update inject limit only after injection occurred
  block, bfq: reduce upper bound for inject limit to max_rq_in_driver+1
  block, bfq: increase update frequency of inject limit
  block, bfq: push up injection only after setting service time

 block/bfq-iosched.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

--
2.20.1

[PATCH 1/4] block, bfq: update inject limit only after injection occurred

[Paolo Valente]

BFQ updates the injection limit of each bfq_queue as a function of how
much the limit inflates the service times experienced by the I/O
requests of the queue. So only service times affected by injection
must be taken into account. Unfortunately, in the current
implementation of this update scheme, the service time of an I/O
request rq not affected by injection may happen to be considered in
the following case: there is no I/O request in service when rq
arrives.

This commit fixes this issue by making sure that only service times
affected by injection are considered for updating the injection
limit. In particular, the service time of an I/O request rq is now
considered only if at least one of the following two conditions holds:
- the destination bfq_queue for rq underwent injection before rq
arrival, and there is still I/O in service in the drive on rq arrival
(the service of such unfinished I/O may delay the service of rq);
- injection occurs between the arrival and the completion time of rq.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index b33be928d164..5a2bbd8613a8 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2025,7 +2025,21 @@ static void bfq_add_request(struct request *rq)
 			 * be set when rq will be dispatched.
 			 */
 			bfqd->wait_dispatch = true;
-			bfqd->rqs_injected = false;
+			/*
+			 * If there is no I/O in service in the drive,
+			 * then possible injection occurred before the
+			 * arrival of rq will not affect the total
+			 * service time of rq. So the injection limit
+			 * must not be updated as a function of such
+			 * total service time, unless new injection
+			 * occurs before rq is completed. To have the
+			 * injection limit updated only in the latter
+			 * case, reset rqs_injected here (rqs_injected
+			 * will be set in case injection is performed
+			 * on bfqq before rq is completed).
+			 */
+			if (bfqd->rq_in_driver == 0)
+				bfqd->rqs_injected = false;
 		}
 	}
 
@@ -5784,7 +5798,7 @@ static void bfq_update_inject_limit(struct bfq_data *bfqd,
 	u64 tot_time_ns = ktime_get_ns() - bfqd->last_empty_occupied_ns;
 	unsigned int old_limit = bfqq->inject_limit;
 
-	if (bfqq->last_serv_time_ns > 0) {
+	if (bfqq->last_serv_time_ns > 0 && bfqd->rqs_injected) {
 		u64 threshold = (bfqq->last_serv_time_ns * 3)>>1;
 
 		if (tot_time_ns >= threshold && old_limit > 0) {
@@ -5830,6 +5844,7 @@ static void bfq_update_inject_limit(struct bfq_data *bfqd,
 
 	/* update complete, not waiting for any request completion any longer */
 	bfqd->waited_rq = NULL;
+	bfqd->rqs_injected = false;
 }
 
 /*
-- 
2.20.1

[PATCH 2/4] block, bfq: reduce upper bound for inject limit to max_rq_in_driver+1

[Paolo Valente]

Upon an increment attempt of the injection limit, the latter is
constrained not to become higher than twice the maximum number
max_rq_in_driver of I/O requests that have happened to be in service
in the drive. This high bound allows the injection limit to grow
beyond max_rq_in_driver, which may then cause max_rq_in_driver itself
to grow.

However, since the limit is incremented by only one unit at a time,
there is no need for such a high bound, and just max_rq_in_driver+1 is
enough.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 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 5a2bbd8613a8..e114282204f6 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5805,7 +5805,7 @@ static void bfq_update_inject_limit(struct bfq_data *bfqd,
 			bfqq->inject_limit--;
 			bfqq->decrease_time_jif = jiffies;
 		} else if (tot_time_ns < threshold &&
-			   old_limit < bfqd->max_rq_in_driver<<1)
+			   old_limit <= bfqd->max_rq_in_driver)
 			bfqq->inject_limit++;
 	}
 
-- 
2.20.1

[PATCH 3/4] block, bfq: increase update frequency of inject limit

[Paolo Valente]

The update period of the injection limit has been tentatively set to
100 ms, to reduce fluctuations. This value however proved to cause,
occasionally, the limit to be decremented for some bfq_queue only
after the queue underwent excessive injection for a lot of time. This
commit reduces the period to 10 ms.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 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 e114282204f6..ddac93e910fa 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -2016,7 +2016,7 @@ static void bfq_add_request(struct request *rq)
 		     (bfqq->last_serv_time_ns > 0 &&
 		      bfqd->rqs_injected && bfqd->rq_in_driver > 0)) &&
 		    time_is_before_eq_jiffies(bfqq->decrease_time_jif +
-					      msecs_to_jiffies(100))) {
+					      msecs_to_jiffies(10))) {
 			bfqd->last_empty_occupied_ns = ktime_get_ns();
 			/*
 			 * Start the state machine for measuring the
-- 
2.20.1

[PATCH 4/4] block, bfq: push up injection only after setting service time

[Paolo Valente]

If equal to 0, the injection limit for a bfq_queue is pushed to 1
after a first sample of the total service time of the I/O requests of
the queue is computed (to allow injection to start). Yet, because of a
mistake in the branch that performs this action, the push may happen
also in some other case. This commit fixes this issue.

Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index ddac93e910fa..0319d6339822 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -5823,12 +5823,14 @@ static void bfq_update_inject_limit(struct bfq_data *bfqd,
 	 */
 	if ((bfqq->last_serv_time_ns == 0 && bfqd->rq_in_driver == 1) ||
 	    tot_time_ns < bfqq->last_serv_time_ns) {
+		if (bfqq->last_serv_time_ns == 0) {
+			/*
+			 * Now we certainly have a base value: make sure we
+			 * start trying injection.
+			 */
+			bfqq->inject_limit = max_t(unsigned int, 1, old_limit);
+		}
 		bfqq->last_serv_time_ns = tot_time_ns;
-		/*
-		 * Now we certainly have a base value: make sure we
-		 * start trying injection.
-		 */
-		bfqq->inject_limit = max_t(unsigned int, 1, old_limit);
 	} else if (!bfqd->rqs_injected && bfqd->rq_in_driver == 1)
 		/*
 		 * No I/O injected and no request still in service in
-- 
2.20.1

Re: [PATCH 0/4] block, bfq: series of improvements and small fixes of the injection mechanism

[Paolo Valente]

Hi Jens,
have you looked into this?

Thanks,
Paolo

> Il giorno 22 ago 2019, alle ore 17:20, Paolo Valente <paolo.valente@linaro.org> ha scritto:
> 
> Hi Jens,
> this patch series makes the injection mechanism better at preserving
> control on I/O.
> 
> Thanks,
> Paolo
> 
> Paolo Valente (4):
>  block, bfq: update inject limit only after injection occurred
>  block, bfq: reduce upper bound for inject limit to max_rq_in_driver+1
>  block, bfq: increase update frequency of inject limit
>  block, bfq: push up injection only after setting service time
> 
> block/bfq-iosched.c | 35 ++++++++++++++++++++++++++---------
> 1 file changed, 26 insertions(+), 9 deletions(-)
> 
> --
> 2.20.1

Re: [PATCH 0/4] block, bfq: series of improvements and small fixes of the injection mechanism

[Oleksandr Natalenko]

On 22.08.2019 17:20, Paolo Valente wrote:
> Hi Jens,
> this patch series makes the injection mechanism better at preserving
> control on I/O.
> 
> Thanks,
> Paolo
> 
> Paolo Valente (4):
>   block, bfq: update inject limit only after injection occurred
>   block, bfq: reduce upper bound for inject limit to max_rq_in_driver+1
>   block, bfq: increase update frequency of inject limit
>   block, bfq: push up injection only after setting service time
> 
>  block/bfq-iosched.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> --
> 2.20.1

FWIW, Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> (I run 
it for quite some time already).

-- 
   Oleksandr Natalenko (post-factum)

Re: [PATCH 0/4] block, bfq: series of improvements and small fixes of the injection mechanism

[Paolo Valente]

Hi Jens,
can these be considered for 5.4 too?

Thanks,
Paolo

> Il giorno 9 set 2019, alle ore 08:03, Oleksandr Natalenko <oleksandr@natalenko.name> ha scritto:
> 
> On 22.08.2019 17:20, Paolo Valente wrote:
>> Hi Jens,
>> this patch series makes the injection mechanism better at preserving
>> control on I/O.
>> Thanks,
>> Paolo
>> Paolo Valente (4):
>>  block, bfq: update inject limit only after injection occurred
>>  block, bfq: reduce upper bound for inject limit to max_rq_in_driver+1
>>  block, bfq: increase update frequency of inject limit
>>  block, bfq: push up injection only after setting service time
>> block/bfq-iosched.c | 35 ++++++++++++++++++++++++++---------
>> 1 file changed, 26 insertions(+), 9 deletions(-)
>> --
>> 2.20.1
> 
> FWIW, Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name> (I run it for quite some time already).
> 
> -- 
>  Oleksandr Natalenko (post-factum)

Re: [PATCH 0/4] block, bfq: series of improvements and small fixes of the injection mechanism

[Jens Axboe]

On 8/22/19 9:20 AM, Paolo Valente wrote:
> Hi Jens,
> this patch series makes the injection mechanism better at preserving
> control on I/O.

Applied, thanks.

-- 
Jens Axboe