Il giorno 3 feb 2021, alle ore 12:43, Jan Kara [off-list ref] ha scritto:

On Thu 28-01-21 18:54:05, Paolo Valente wrote:

quoted

quoted

Il giorno 26 gen 2021, alle ore 17:18, Jens Axboe [off-list ref] ha scritto:

On 1/26/21 3:50 AM, Paolo Valente wrote:

quoted

Consider a new I/O request that arrives for a bfq_queue bfqq. If, when
this happens, the only active bfq_queues are bfqq and either its waker
bfq_queue or one of its woken bfq_queues, then there is no point in
queueing this new I/O request in bfqq for service. In fact, the
in-service queue and bfqq agree on serving this new I/O request as
soon as possible. So this commit puts this new I/O request directly
into the dispatch list.

Tested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Paolo Valente <redacted>
---
block/bfq-iosched.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index a83149407336..e5b83910fbe0 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c

@@ -5640,7 +5640,22 @@ static void bfq_insert_request(struct blk_mq_hw_ctx *hctx, struct request *rq,

	spin_lock_irq(&bfqd->lock);
	bfqq = bfq_init_rq(rq);
-	if (!bfqq || at_head || blk_rq_is_passthrough(rq)) {
+
+	/*
+	 * Additional case for putting rq directly into the dispatch
+	 * queue: the only active bfq_queues are bfqq and either its
+	 * waker bfq_queue or one of its woken bfq_queues. In this
+	 * case, there is no point in queueing rq in bfqq for
+	 * service. In fact, the in-service queue and bfqq agree on
+	 * serving this new I/O request as soon as possible.
+	 */
+	if (!bfqq ||
+	    (bfqq != bfqd->in_service_queue &&
+	     bfqd->in_service_queue != NULL &&
+	     bfq_tot_busy_queues(bfqd) == 1 + bfq_bfqq_busy(bfqq) &&
+	     (bfqq->waker_bfqq == bfqd->in_service_queue ||
+	      bfqd->in_service_queue->waker_bfqq == bfqq)) ||
+	    at_head || blk_rq_is_passthrough(rq)) {
		if (at_head)
			list_add(&rq->queuelist, &bfqd->dispatch);
		else

This is unreadable... Just seems like you are piling heuristics in to
catch some case, and it's neither readable nor clean.

Yeah, these comments inappropriately assume that the reader knows the
waker mechanism in depth.  And they do not stress at all how important
this improvement is.

I'll do my best to improve these comments.

To try to do a better job, let me also explain the matter early here.
Maybe you or others can give me some early feedback (or just tell me
to proceed).

This change is one of the main improvements that boosted
throughput in Jan's tests.  Here is the rationale:
- consider a bfq_queue, say Q1, detected as a waker of another
 bfq_queue, say Q2
- by definition of a waker, Q1 blocks the I/O of Q2, i.e., some I/O of
 of Q1 needs to be completed for new I/O of Q1 to arrive.  A notable

					       ^^ Q2?

Yes, thank you!

(after this interaction, I'll fix and improve all this description,
according to your comments)

quoted

 example is journald
- so, Q1 and Q2 are in any respect two cooperating processes: if the
 service of Q1's I/O is delayed, Q2 can only suffer from it.
 Conversely, if Q2's I/O is delayed, the purpose of Q1 is just defeated.

What do you exactly mean by this last sentence?

By definition of waker, the purpose of Q1's I/O is doing what needs to
be done, so that new Q2's I/O can finally be issued.  Delaying Q2's I/O
is the opposite of this goal.

quoted

- as a consequence if some I/O of Q1/Q2 arrives while Q2/Q1 is the
 only queue in service, there is absolutely no point in delaying the
 service of such an I/O.  The only possible result is a throughput
 loss, detected by Jan's test

If we are idling at that moment waiting for more IO from in service queue,
I agree.

And I agree too, if the drive has no internal queueing, has no
parallelism or pipeline, or is at least one order of magnitude slower
than the CPU is processing I/O.  In all other cases, serving the I/O
of only one queue at a time means throwing away throughput.  For
example, on a consumer SSD, moving from one to two I/O threads served
in parallel usually means doubling the throughput.

So, the best thing to do, if all the above conditions are met, is to
have this new I/O dispatched as soon as possible.

The most efficient way to attain this goal is to just put the new I/O
directly into the dispatch list.

But that doesn't seem to be part of your condition above?

quoted

- so, when the above condition holds, the most effective and efficient
 action is to put the new I/O directly in the dispatch list
- as an additional restriction, Q1 and Q2 must be the only busy queues
 for this commit to put the I/O of Q2/Q1 in the dispatch list.  This is
 necessary, because, if also other queues are waiting for service, then
 putting new I/O directly in the dispatch list may evidently cause a
 violation of service guarantees for the other queues

This last restriction is not ideal for cases like jbd2 thread since it may
still lead to pointless idling but I understand that without some
restriction like this several waking threads could just starve other ones.

Yeah, the goal here is to reduce a little bit false positives.

So I guess it's fine for now.

Yes, hopefully experience will lead us to even improvements or even
better solutions.

Thanks,
Paolo

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR