LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged
@ 2018-05-31 13:23 Paolo Valente
  2018-05-31 13:23 ` [PATCH BUGFIX 1/3] block, bfq: remove wrong lock in bfq_requests_merged Paolo Valente
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Paolo Valente @ 2018-05-31 13:23 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, broonie, linus.walleij,
	bfq-iosched, oleksandr, filippo.muzzini, Paolo Valente

Hi Jens,
this series fixes three bugs in bfq_requests_merged. In more detail:

- two linked bugs, with the first (critical: wrong lock) hidden by the
  second (less critical: wrong check that made the body of the
  function not be executed at all)
- a rather minor bug: superflous code

Thanks,
Paolo

Filippo Muzzini (2):
  block, bfq: remove wrong lock in bfq_requests_merged
  block, bfq: remove the removal of 'next' rq in bfq_requests_merged

Paolo Valente (1):
  block, bfq: remove wrong check in bfq_requests_merged

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

--
2.16.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH BUGFIX 1/3] block, bfq: remove wrong lock in bfq_requests_merged
  2018-05-31 13:23 [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Paolo Valente
@ 2018-05-31 13:23 ` Paolo Valente
  2018-05-31 13:23 ` [PATCH BUGFIX 2/3] block, bfq: remove wrong check " Paolo Valente
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Paolo Valente @ 2018-05-31 13:23 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, broonie, linus.walleij,
	bfq-iosched, oleksandr, filippo.muzzini, Paolo Valente

From: Filippo Muzzini <filippo.muzzini@outlook.it>

In bfq_requests_merged(), there is a deadlock because the lock on
bfqq->bfqd->lock is held by the calling function, but the code of
this function tries to grab the lock again.

This deadlock is currently hidden by another bug (fixed by next commit
for this source file), which causes the body of bfq_requests_merged()
to be never executed.

This commit removes the deadlock by removing the lock/unlock pair.

Signed-off-by: Filippo Muzzini <filippo.muzzini@outlook.it>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 771ae9730ac6..1f0951d36424 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1898,7 +1898,6 @@ static void bfq_requests_merged(struct request_queue *q, struct request *rq,
 
 	if (!RB_EMPTY_NODE(&rq->rb_node))
 		goto end;
-	spin_lock_irq(&bfqq->bfqd->lock);
 
 	/*
 	 * If next and rq belong to the same bfq_queue and next is older
@@ -1923,7 +1922,6 @@ static void bfq_requests_merged(struct request_queue *q, struct request *rq,
 	bfq_remove_request(q, next);
 	bfqg_stats_update_io_remove(bfqq_group(bfqq), next->cmd_flags);
 
-	spin_unlock_irq(&bfqq->bfqd->lock);
 end:
 	bfqg_stats_update_io_merged(bfqq_group(bfqq), next->cmd_flags);
 }
-- 
2.16.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH BUGFIX 2/3] block, bfq: remove wrong check in bfq_requests_merged
  2018-05-31 13:23 [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Paolo Valente
  2018-05-31 13:23 ` [PATCH BUGFIX 1/3] block, bfq: remove wrong lock in bfq_requests_merged Paolo Valente
@ 2018-05-31 13:23 ` Paolo Valente
  2018-05-31 13:23 ` [PATCH BUGFIX 3/3] block, bfq: remove the removal of 'next' rq " Paolo Valente
  2018-05-31 14:48 ` [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Jens Axboe
  3 siblings, 0 replies; 6+ messages in thread
From: Paolo Valente @ 2018-05-31 13:23 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, broonie, linus.walleij,
	bfq-iosched, oleksandr, filippo.muzzini, Paolo Valente

The request rq passed to the function bfq_requests_merged is always in
a bfq_queue, so the check !RB_EMPTY_NODE(&rq->rb_node) at the
beginning of bfq_requests_merged always succeeds, and the control
flow systematically skips to the end of the function.  This implies
that the body of the function is never executed, i.e., the
repositioning of rq is never performed.

On the opposite end, a control is missing in the body of the function:
'next' must be removed only if it is inside a bfq_queue.

This commit removes the wrong check on rq, and adds the missing check
on 'next'. In addition, this commit adds comments on
bfq_requests_merged.

Signed-off-by: Filippo Muzzini <filippo.muzzini@outlook.it>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 1f0951d36424..df2a9633cf4a 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1891,14 +1891,27 @@ static void bfq_request_merged(struct request_queue *q, struct request *req,
 	}
 }
 
+/*
+ * This function is called to notify the scheduler that the requests
+ * rq and 'next' have been merged, with 'next' going away.  BFQ
+ * exploits this hook to address the following issue: if 'next' has a
+ * fifo_time lower that rq, then the fifo_time of rq must be set to
+ * the value of 'next', to not forget the greater age of 'next'.
+ * Moreover 'next' may be in a bfq_queue, in this case it must be
+ * removed.
+ *
+ * NOTE: in this function we assume that rq is in a bfq_queue, basing
+ * on that rq is picked from the hash table q->elevator->hash, which,
+ * in its turn, is filled only with I/O requests present in
+ * bfq_queues, while BFQ is in use for the request queue q. In fact,
+ * the function that fills this hash table (elv_rqhash_add) is called
+ * only by bfq_insert_request.
+ */
 static void bfq_requests_merged(struct request_queue *q, struct request *rq,
 				struct request *next)
 {
 	struct bfq_queue *bfqq = RQ_BFQQ(rq), *next_bfqq = RQ_BFQQ(next);
 
-	if (!RB_EMPTY_NODE(&rq->rb_node))
-		goto end;
-
 	/*
 	 * If next and rq belong to the same bfq_queue and next is older
 	 * than rq, then reposition rq in the fifo (by substituting next
@@ -1919,10 +1932,11 @@ static void bfq_requests_merged(struct request_queue *q, struct request *rq,
 	if (bfqq->next_rq == next)
 		bfqq->next_rq = rq;
 
-	bfq_remove_request(q, next);
-	bfqg_stats_update_io_remove(bfqq_group(bfqq), next->cmd_flags);
+	if (!RB_EMPTY_NODE(&next->rb_node)) {
+		bfq_remove_request(q, next);
+		bfqg_stats_update_io_remove(bfqq_group(bfqq), next->cmd_flags);
+	}
 
-end:
 	bfqg_stats_update_io_merged(bfqq_group(bfqq), next->cmd_flags);
 }
 
-- 
2.16.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH BUGFIX 3/3] block, bfq: remove the removal of 'next' rq in bfq_requests_merged
  2018-05-31 13:23 [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Paolo Valente
  2018-05-31 13:23 ` [PATCH BUGFIX 1/3] block, bfq: remove wrong lock in bfq_requests_merged Paolo Valente
  2018-05-31 13:23 ` [PATCH BUGFIX 2/3] block, bfq: remove wrong check " Paolo Valente
@ 2018-05-31 13:23 ` Paolo Valente
  2018-05-31 14:48 ` [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Jens Axboe
  3 siblings, 0 replies; 6+ messages in thread
From: Paolo Valente @ 2018-05-31 13:23 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, linux-kernel, ulf.hansson, broonie, linus.walleij,
	bfq-iosched, oleksandr, filippo.muzzini, Paolo Valente

From: Filippo Muzzini <filippo.muzzini@outlook.it>

Since bfq_finish_request() is always called on the request 'next',
after bfq_requests_merged() is finished, and bfq_finish_request()
removes 'next' from its bfq_queue if needed, it isn't necessary to do
such a removal in advance in bfq_merged_requests().

This commit removes such a useless 'next' removal.

Signed-off-by: Filippo Muzzini <filippo.muzzini@outlook.it>
Signed-off-by: Paolo Valente <paolo.valente@linaro.org>
---
 block/bfq-iosched.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index df2a9633cf4a..f71a5846b629 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1897,8 +1897,6 @@ static void bfq_request_merged(struct request_queue *q, struct request *req,
  * exploits this hook to address the following issue: if 'next' has a
  * fifo_time lower that rq, then the fifo_time of rq must be set to
  * the value of 'next', to not forget the greater age of 'next'.
- * Moreover 'next' may be in a bfq_queue, in this case it must be
- * removed.
  *
  * NOTE: in this function we assume that rq is in a bfq_queue, basing
  * on that rq is picked from the hash table q->elevator->hash, which,
@@ -1932,11 +1930,6 @@ static void bfq_requests_merged(struct request_queue *q, struct request *rq,
 	if (bfqq->next_rq == next)
 		bfqq->next_rq = rq;
 
-	if (!RB_EMPTY_NODE(&next->rb_node)) {
-		bfq_remove_request(q, next);
-		bfqg_stats_update_io_remove(bfqq_group(bfqq), next->cmd_flags);
-	}
-
 	bfqg_stats_update_io_merged(bfqq_group(bfqq), next->cmd_flags);
 }
 
-- 
2.16.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged
  2018-05-31 13:23 [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Paolo Valente
                   ` (2 preceding siblings ...)
  2018-05-31 13:23 ` [PATCH BUGFIX 3/3] block, bfq: remove the removal of 'next' rq " Paolo Valente
@ 2018-05-31 14:48 ` Jens Axboe
  2018-05-31 14:50   ` Paolo Valente
  3 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2018-05-31 14:48 UTC (permalink / raw)
  To: Paolo Valente
  Cc: linux-block, linux-kernel, ulf.hansson, broonie, linus.walleij,
	bfq-iosched, oleksandr, filippo.muzzini

On 5/31/18 7:23 AM, Paolo Valente wrote:
> Hi Jens,
> this series fixes three bugs in bfq_requests_merged. In more detail:
> 
> - two linked bugs, with the first (critical: wrong lock) hidden by the
>   second (less critical: wrong check that made the body of the
>   function not be executed at all)
> - a rather minor bug: superflous code

Patch #2 doesn't apply on for-4.18/block. I hand applied it, all
3 are queued up now.

-- 
Jens Axboe

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged
  2018-05-31 14:48 ` [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Jens Axboe
@ 2018-05-31 14:50   ` Paolo Valente
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Valente @ 2018-05-31 14:50 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, LKML, Ulf Hansson, Mark Brown, Linus Walleij,
	'Paolo Valente' via bfq-iosched, oleksandr,
	filippo.muzzini



> Il giorno 31 mag 2018, alle ore 16:48, Jens Axboe <axboe@kernel.dk> ha scritto:
> 
> On 5/31/18 7:23 AM, Paolo Valente wrote:
>> Hi Jens,
>> this series fixes three bugs in bfq_requests_merged. In more detail:
>> 
>> - two linked bugs, with the first (critical: wrong lock) hidden by the
>>  second (less critical: wrong check that made the body of the
>>  function not be executed at all)
>> - a rather minor bug: superflous code
> 
> Patch #2 doesn't apply on for-4.18/block.

Yep, it gave some problems to me too, because we happened to move this
patch around by email.

> I hand applied it, all
> 3 are queued up now.
> 

Thanks for the patience and for queueing these fixes.

Paolo

> -- 
> Jens Axboe
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-31 14:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-31 13:23 [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Paolo Valente
2018-05-31 13:23 ` [PATCH BUGFIX 1/3] block, bfq: remove wrong lock in bfq_requests_merged Paolo Valente
2018-05-31 13:23 ` [PATCH BUGFIX 2/3] block, bfq: remove wrong check " Paolo Valente
2018-05-31 13:23 ` [PATCH BUGFIX 3/3] block, bfq: remove the removal of 'next' rq " Paolo Valente
2018-05-31 14:48 ` [PATCH BUGFIX 0/3] three fixes for the function bfq_requests_merged Jens Axboe
2018-05-31 14:50   ` Paolo Valente

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).