[PATCH BUGFIX/IMPROVEMENT 0/4] bfq: two fixes and one improvement related to I/O control

[PATCH BUGFIX/IMPROVEMENT 0/4] bfq: two fixes and one improvement related to I/O control

[Paolo Valente]

Hi Jens,

while working a little bit on cgroups I/O control, I found two nasty
bugs in bfq. They break bandwidth control in simple configurations
with one-process groups. These bugs are fixed by the first two patches
in this series.

These fixes improved I/O control so much, that I could reduce the
write overcharge factor, used by bfq to counter write-induced
issues. This reduction is performed by the third patch.

The fourth patch contains a little code improvement I made in a
function that has to do with I/O control.

I hope we are still in time for 4.19.

Thanks,
Paolo

Paolo Valente (4):
  block, bfq: readd missing reset of parent-entity service
  block, bfq: always update the budget of an entity when needed
  block, bfq: reduce write overcharge
  block, bfq: improve code of bfq_bfqq_charge_time

 block/bfq-iosched.c | 54 +++++++++++++++++++++++++++++++++++++++--------------
 block/bfq-wf2q.c    | 22 +++++++++++-----------
 2 files changed, 51 insertions(+), 25 deletions(-)

--
2.16.1

[PATCH BUGFIX/IMPROVEMENT 1/4] block, bfq: readd missing reset of parent-entity service

[Paolo Valente]

The received-service counter needs to be equal to 0 when an entity is
set in service. Unfortunately, commit "block, bfq: fix service being
wrongly set to zero in case of preemption" mistakenly removed the
resetting of this counter for the parent entities of the bfq_queue
being set in service. This commit fixes this issue by resetting
service for parent entities, directly on the expiration of the
in-service bfq_queue.

Fixes: 9fae8dd59ff3 ("block, bfq: fix service being wrongly set to zero in case of preemption")
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 41d9036b1822..62efc1b97afb 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -3298,6 +3298,27 @@ void bfq_bfqq_expire(struct bfq_data *bfqd,
 		 */
 	} else
 		entity->service = 0;
+
+	/*
+	 * Reset the received-service counter for every parent entity.
+	 * Differently from what happens with bfqq->entity.service,
+	 * the resetting of this counter never needs to be postponed
+	 * for parent entities. In fact, in case bfqq may have a
+	 * chance to go on being served using the last, partially
+	 * consumed budget, bfqq->entity.service needs to be kept,
+	 * because if bfqq then actually goes on being served using
+	 * the same budget, the last value of bfqq->entity.service is
+	 * needed to properly decrement bfqq->entity.budget by the
+	 * portion already consumed. In contrast, it is not necessary
+	 * to keep entity->service for parent entities too, because
+	 * the bubble up of the new value of bfqq->entity.budget will
+	 * make sure that the budgets of parent entities are correct,
+	 * even in case bfqq and thus parent entities go on receiving
+	 * service with the same budget.
+	 */
+	entity = entity->parent;
+	for_each_entity(entity)
+		entity->service = 0;
 }
 
 /*
-- 
2.16.1

[PATCH BUGFIX/IMPROVEMENT 2/4] block, bfq: always update the budget of an entity when needed

[Paolo Valente]

When the next child entity to serve changes for a given parent entity,
the budget of that parent entity must be updated accordingly.
Unfortunately, this update is not performed, by mistake, for the
entities that happen to switch from having no child entity to serve,
to having one child entity to serve.

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

diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index dbc07b456059..d558fd26740c 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -130,10 +130,14 @@ static bool bfq_update_next_in_service(struct bfq_sched_data *sd,
 	if (!change_without_lookup) /* lookup needed */
 		next_in_service = bfq_lookup_next_entity(sd, expiration);
 
-	if (next_in_service)
-		parent_sched_may_change = !sd->next_in_service ||
+	if (next_in_service) {
+		bool new_budget_triggers_change =
 			bfq_update_parent_budget(next_in_service);
 
+		parent_sched_may_change = !sd->next_in_service ||
+			new_budget_triggers_change;
+	}
+
 	sd->next_in_service = next_in_service;
 
 	if (!next_in_service)
-- 
2.16.1

[PATCH BUGFIX/IMPROVEMENT 3/4] block, bfq: reduce write overcharge

[Paolo Valente]

When a sync request is dispatched, the queue that contains that
request, and all the ancestor entities of that queue, are charged with
the number of sectors of the request. In constrast, if the request is
async, then the queue and its ancestor entities are charged with the
number of sectors of the request, multiplied by an overcharge
factor. This throttles the bandwidth for async I/O, w.r.t. to sync
I/O, and it is done to counter the tendency of async writes to steal
I/O throughput to reads.

On the opposite end, the lower this parameter, the stabler I/O
control, in the following respect.  The lower this parameter is, the
less the bandwidth enjoyed by a group decreases
- when the group does writes, w.r.t. to when it does reads;
- when other groups do reads, w.r.t. to when they do writes.

The fixes "block, bfq: always update the budget of an entity when
needed" and "block, bfq: readd missing reset of parent-entity service"
improved I/O control in bfq to such an extent that it has been
possible to revise this overcharge factor downwards.  This commit
introduces the resulting, new value.

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

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 62efc1b97afb..653100fb719e 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -187,11 +187,25 @@ static const int bfq_stats_min_budgets = 194;
 static const int bfq_default_max_budget = 16 * 1024;
 
 /*
- * Async to sync throughput distribution is controlled as follows:
- * when an async request is served, the entity is charged the number
- * of sectors of the request, multiplied by the factor below
+ * When a sync request is dispatched, the queue that contains that
+ * request, and all the ancestor entities of that queue, are charged
+ * with the number of sectors of the request. In constrast, if the
+ * request is async, then the queue and its ancestor entities are
+ * charged with the number of sectors of the request, multiplied by
+ * the factor below. This throttles the bandwidth for async I/O,
+ * w.r.t. to sync I/O, and it is done to counter the tendency of async
+ * writes to steal I/O throughput to reads.
+ *
+ * The current value of this parameter is the result of a tuning with
+ * several hardware and software configurations. We tried to find the
+ * lowest value for which writes do not cause noticeable problems to
+ * reads. In fact, the lower this parameter, the stabler I/O control,
+ * in the following respect.  The lower this parameter is, the less
+ * the bandwidth enjoyed by a group decreases
+ * - when the group does writes, w.r.t. to when it does reads;
+ * - when other groups do reads, w.r.t. to when they do writes.
  */
-static const int bfq_async_charge_factor = 10;
+static const int bfq_async_charge_factor = 3;
 
 /* Default timeout values, in jiffies, approximating CFQ defaults. */
 const int bfq_timeout = HZ / 8;
@@ -853,16 +867,7 @@ static unsigned long bfq_serv_to_charge(struct request *rq,
 	if (bfq_bfqq_sync(bfqq) || bfqq->wr_coeff > 1)
 		return blk_rq_sectors(rq);
 
-	/*
-	 * If there are no weight-raised queues, then amplify service
-	 * by just the async charge factor; otherwise amplify service
-	 * by twice the async charge factor, to further reduce latency
-	 * for weight-raised queues.
-	 */
-	if (bfqq->bfqd->wr_busy_queues == 0)
-		return blk_rq_sectors(rq) * bfq_async_charge_factor;
-
-	return blk_rq_sectors(rq) * 2 * bfq_async_charge_factor;
+	return blk_rq_sectors(rq) * bfq_async_charge_factor;
 }
 
 /**
-- 
2.16.1

[PATCH BUGFIX/IMPROVEMENT 4/4] block, bfq: improve code of bfq_bfqq_charge_time

[Paolo Valente]

bfq_bfqq_charge_time contains some lengthy and redundant code. This
commit trims and condenses that code.

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

diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index d558fd26740c..ae52bff43ce4 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -881,15 +881,11 @@ void bfq_bfqq_charge_time(struct bfq_data *bfqd, struct bfq_queue *bfqq,
 			  unsigned long time_ms)
 {
 	struct bfq_entity *entity = &bfqq->entity;
-	int tot_serv_to_charge = entity->service;
-	unsigned int timeout_ms = jiffies_to_msecs(bfq_timeout);
-
-	if (time_ms > 0 && time_ms < timeout_ms)
-		tot_serv_to_charge =
-			(bfqd->bfq_max_budget * time_ms) / timeout_ms;
-
-	if (tot_serv_to_charge < entity->service)
-		tot_serv_to_charge = entity->service;
+	unsigned long timeout_ms = jiffies_to_msecs(bfq_timeout);
+	unsigned long bounded_time_ms = min(time_ms, timeout_ms);
+	int serv_to_charge_for_time =
+		(bfqd->bfq_max_budget * bounded_time_ms) / timeout_ms;
+	int tot_serv_to_charge = max(serv_to_charge_for_time, entity->service);
 
 	/* Increase budget to avoid inconsistencies */
 	if (tot_serv_to_charge > entity->budget)
-- 
2.16.1

Re: [PATCH BUGFIX/IMPROVEMENT 0/4] bfq: two fixes and one improvement related to I/O control

[Jens Axboe]

On 8/16/18 10:51 AM, Paolo Valente wrote:
> Hi Jens,
> 
> while working a little bit on cgroups I/O control, I found two nasty
> bugs in bfq. They break bandwidth control in simple configurations
> with one-process groups. These bugs are fixed by the first two patches
> in this series.
> 
> These fixes improved I/O control so much, that I could reduce the
> write overcharge factor, used by bfq to counter write-induced
> issues. This reduction is performed by the third patch.
> 
> The fourth patch contains a little code improvement I made in a
> function that has to do with I/O control.
> 
> I hope we are still in time for 4.19.

Patches look fine to me. You are in fact several weeks late for 4.19, I
need to have anything that's going in to 4.19 by -rc6 or -rc7 time
(depending on whether we have an -rc8 or not). This is to ensure that it
gets plenty of time in linux-next as well. I've queued up patches this
time (since they were small), but please send them in in due time next
time.

-- 
Jens Axboe

Re: [PATCH BUGFIX/IMPROVEMENT 0/4] bfq: two fixes and one improvement related to I/O control

[Paolo Valente]

> Il giorno 16 ago 2018, alle ore 21:10, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 8/16/18 10:51 AM, Paolo Valente wrote:
>> Hi Jens,
>> 
>> while working a little bit on cgroups I/O control, I found two nasty
>> bugs in bfq. They break bandwidth control in simple configurations
>> with one-process groups. These bugs are fixed by the first two patches
>> in this series.
>> 
>> These fixes improved I/O control so much, that I could reduce the
>> write overcharge factor, used by bfq to counter write-induced
>> issues. This reduction is performed by the third patch.
>> 
>> The fourth patch contains a little code improvement I made in a
>> function that has to do with I/O control.
>> 
>> I hope we are still in time for 4.19.
> 
> Patches look fine to me. You are in fact several weeks late for 4.19, I
> need to have anything that's going in to 4.19 by -rc6 or -rc7 time
> (depending on whether we have an -rc8 or not). This is to ensure that it
> gets plenty of time in linux-next as well. I've queued up patches this
> time (since they were small), but please send them in in due time next
> time.
> 

Thanks for your patience. Next time I'll respect due time.

Paolo

> -- 
> Jens Axboe
>