diff mbox

[BUGFIX] block, bfq: reset in_service_entity if it becomes idle

Message ID 20170728194118.8249-1-paolo.valente@linaro.org
State Accepted
Commit 6ab1d8da972d4c4e318607e96c5ecb32101c80f4
Headers show

Commit Message

Paolo Valente July 28, 2017, 7:41 p.m. UTC 
BFQ implements hierarchical scheduling by representing each group of
queues with a generic parent entity. For each parent entity, BFQ
maintains an in_service_entity pointer: if one of the child entities
happens to be in service, in_service_entity points to it.  The
resetting of these pointers happens only on queue expirations: when
the in-service queue is expired, i.e., stops to be the queue in
service, BFQ resets all in_service_entity pointers along the
parent-entity path from this queue to the root entity.

Functions handling the scheduling of entities assume, naturally, that
in-service entities are active, i.e., have pending I/O requests (or,
as a special case, even if they have no pending requests, they are
expected to receive a new request very soon, with the scheduler idling
the storage device while waiting for such an event). Unfortunately,
the above resetting scheme of the in_service_entity pointers may cause
this assumption to be violated.  For example, the in-service queue may
happen to remain without requests because of a request merge. In this
case the queue does become idle, and all related data structures are
updated accordingly. But in_service_entity still points to the queue
in the parent entity. This inconsistency may even propagate to
higher-level parent entities, if they happen to become idle as well,
as a consequence of the leaf queue becoming idle. For this queue and
parent entities, scheduling functions have an undefined behaviour,
and, as reported, may easily lead to kernel crashes or hangs.

This commit addresses this issue by simply resetting the
in_service_entity field also when it is detected to point to an entity
becoming idle (regardless of why the entity becomes idle).

Reported-by: Laurentiu Nicola <lnicola@dend.ro>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>

Tested-by: Laurentiu Nicola <lnicola@dend.ro>

---
 block/bfq-wf2q.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.10.0

Comments

Jens Axboe July 29, 2017, 9:33 p.m. UTC | #1 
On 07/28/2017 01:41 PM, Paolo Valente wrote:
> BFQ implements hierarchical scheduling by representing each group of

> queues with a generic parent entity. For each parent entity, BFQ

> maintains an in_service_entity pointer: if one of the child entities

> happens to be in service, in_service_entity points to it.  The

> resetting of these pointers happens only on queue expirations: when

> the in-service queue is expired, i.e., stops to be the queue in

> service, BFQ resets all in_service_entity pointers along the

> parent-entity path from this queue to the root entity.

> 

> Functions handling the scheduling of entities assume, naturally, that

> in-service entities are active, i.e., have pending I/O requests (or,

> as a special case, even if they have no pending requests, they are

> expected to receive a new request very soon, with the scheduler idling

> the storage device while waiting for such an event). Unfortunately,

> the above resetting scheme of the in_service_entity pointers may cause

> this assumption to be violated.  For example, the in-service queue may

> happen to remain without requests because of a request merge. In this

> case the queue does become idle, and all related data structures are

> updated accordingly. But in_service_entity still points to the queue

> in the parent entity. This inconsistency may even propagate to

> higher-level parent entities, if they happen to become idle as well,

> as a consequence of the leaf queue becoming idle. For this queue and

> parent entities, scheduling functions have an undefined behaviour,

> and, as reported, may easily lead to kernel crashes or hangs.

> 

> This commit addresses this issue by simply resetting the

> in_service_entity field also when it is detected to point to an entity

> becoming idle (regardless of why the entity becomes idle).


Applied, thanks.

-- 
Jens Axboe
diff mbox

Patch

diff --git a/block/bfq-wf2q.c b/block/bfq-wf2q.c
index 979f8f2..881bbe5 100644
--- a/block/bfq-wf2q.c
+++ b/block/bfq-wf2q.c
@@ -1158,8 +1158,10 @@  bool __bfq_deactivate_entity(struct bfq_entity *entity, bool ins_into_idle_tree)
 	st = bfq_entity_service_tree(entity);
 	is_in_service = entity == sd->in_service_entity;
 
-	if (is_in_service)
+	if (is_in_service) {
 		bfq_calc_finish(entity, entity->service);
+		sd->in_service_entity = NULL;
+	}
 
 	if (entity->tree == &st->active)
 		bfq_active_extract(st, entity);