[PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance

[PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance

[Jan Kara]

Hello,

I was investigating why dbench performance (even for single dbench client) with
BFQ is significantly worse than it used to be CFQ. The culprit is the idling
logic in BFQ. The dbench workload is very fsync(2) heavy. In practice the time
to complete fsync(2) calls is what determines the overall performance. For
filesystems with a journal such as xfs or ext4 it is common that fsync(2)
involves writing data from the process runningg fsync(2) - dbench in this case
- and then waiting for the journalling machinery to flush out metadata from a
separate process (jbd2 process in ext4 case, worker thread in xfs case).
CFQ's heuristic was able to determine that it isn't worth to idle waiting for
either dbench or jbd2 IO. BFQ's heuristic is not able to determine this and
thus jbd2 process is often blocked waiting for idle timer of dbench queue to
trigger.

The first patch in the series is an obvious bugfix but is not enough to improve
performance. The second patch does improve dbench performance from ~80 MB/s
to ~200 MB/s on my test machine but I'm aware that it is probably way too
aggressive and probably a different solution is needed. So I just wrote that
patch to see the results and spark some discussion :). Any idea how to
improve the waker logic so that dbench performance doesn't drop so
dramatically?

								Honza

[PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected

[Jan Kara]

The check in bfq_select_queue() checking whether a waker queue should be
selected has a bug and is checking bfqq->next_rq instead of
bfqq->waker_bfqq->next_rq to verify whether the waker queue has a
request to dispatch. This often results in the condition being false
(most notably when the current queue is idling waiting for next request)
and thus the waker queue logic is ineffective. Fix the condition.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 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 78ba57efd16b..18f85d474c9c 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -4498,7 +4498,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd)
 			bfqq = bfqq->bic->bfqq[0];
 		else if (bfq_bfqq_has_waker(bfqq) &&
 			   bfq_bfqq_busy(bfqq->waker_bfqq) &&
-			   bfqq->next_rq &&
+			   bfqq->waker_bfqq->next_rq &&
 			   bfq_serv_to_charge(bfqq->waker_bfqq->next_rq,
 					      bfqq->waker_bfqq) <=
 			   bfq_bfqq_budget_left(bfqq->waker_bfqq)
-- 
2.16.4

[PATCH 2/2] bfq: Allow short_ttime queues to have waker

[Jan Kara]

Currently queues that have average think time shorter than slice_idle
cannot have waker. However this requirement is too strict. E.g. dbench
process always submits a one or two IOs (which is enough to pull its
average think time below slice_idle) and then blocks waiting for jbd2
thread to commit a transaction. Due to idling logic jbd2 thread is
often forced to wait for dbench's idle timer to trigger to be able to
submit its IO and this severely delays the overall benchmark progress.

E.g. on my test machine current dbench single-thread throughput is ~80
MB/s, with this patch it is ~200 MB/s.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 block/bfq-iosched.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 18f85d474c9c..416473ba80c8 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -1928,7 +1928,6 @@ static void bfq_add_request(struct request *rq)
 		 * I/O-plugging interval for bfqq.
 		 */
 		if (bfqd->last_completed_rq_bfqq &&
-		    !bfq_bfqq_has_short_ttime(bfqq) &&
 		    ktime_get_ns() - bfqd->last_completion <
 		    200 * NSEC_PER_USEC) {
 			if (bfqd->last_completed_rq_bfqq != bfqq &&
-- 
2.16.4

Re: [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance

[Paolo Valente]

Hi Jan,
I'm glad you're addressing these BFQ issues.  Sorry for the delay, but
I happen to be working on similar issues, for other sort of
corner-case workloads.  So I wanted to consolidate my changes before
replying.

Probably, the best first step for me, to check your proposal, is to
merge it with my current changes, and test the outcome.  In this
respect, my problem is that, after our last improvements for dbench,
we cannot reproduce regressions any longer.  So, we would need your
support to test both issues, i.e., to test throughput with dbench (on
your side/machines), and possible other regressions of your and my
changes (on our side/machines).

Would it be ok for you to participate in this little collaboration?
If it is, then I'll contact you privately to kick this off.

Thanks,
Paolo

> Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> 
> Hello,
> 
> I was investigating why dbench performance (even for single dbench client) with
> BFQ is significantly worse than it used to be CFQ. The culprit is the idling
> logic in BFQ. The dbench workload is very fsync(2) heavy. In practice the time
> to complete fsync(2) calls is what determines the overall performance. For
> filesystems with a journal such as xfs or ext4 it is common that fsync(2)
> involves writing data from the process runningg fsync(2) - dbench in this case
> - and then waiting for the journalling machinery to flush out metadata from a
> separate process (jbd2 process in ext4 case, worker thread in xfs case).
> CFQ's heuristic was able to determine that it isn't worth to idle waiting for
> either dbench or jbd2 IO. BFQ's heuristic is not able to determine this and
> thus jbd2 process is often blocked waiting for idle timer of dbench queue to
> trigger.
> 
> The first patch in the series is an obvious bugfix but is not enough to improve
> performance. The second patch does improve dbench performance from ~80 MB/s
> to ~200 MB/s on my test machine but I'm aware that it is probably way too
> aggressive and probably a different solution is needed. So I just wrote that
> patch to see the results and spark some discussion :). Any idea how to
> improve the waker logic so that dbench performance doesn't drop so
> dramatically?
> 
> 								Honza

Re: [PATCH 0/2 RFC] bfq: Waker logic tweaks for dbench performance

[Jan Kara]

Hi Paolo!

On Fri 17-04-20 09:47:07, Paolo Valente wrote:
> I'm glad you're addressing these BFQ issues.  Sorry for the delay, but
> I happen to be working on similar issues, for other sort of
> corner-case workloads.  So I wanted to consolidate my changes before
> replying.
> 
> Probably, the best first step for me, to check your proposal, is to
> merge it with my current changes, and test the outcome.  In this
> respect, my problem is that, after our last improvements for dbench,
> we cannot reproduce regressions any longer.  So, we would need your
> support to test both issues, i.e., to test throughput with dbench (on
> your side/machines), and possible other regressions of your and my
> changes (on our side/machines).
> 
> Would it be ok for you to participate in this little collaboration?
> If it is, then I'll contact you privately to kick this off.

Sure, I can test whatever patches you send me on our machines.

								Honza

> 
> Thanks,
> Paolo
> 
> > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> > 
> > Hello,
> > 
> > I was investigating why dbench performance (even for single dbench client) with
> > BFQ is significantly worse than it used to be CFQ. The culprit is the idling
> > logic in BFQ. The dbench workload is very fsync(2) heavy. In practice the time
> > to complete fsync(2) calls is what determines the overall performance. For
> > filesystems with a journal such as xfs or ext4 it is common that fsync(2)
> > involves writing data from the process runningg fsync(2) - dbench in this case
> > - and then waiting for the journalling machinery to flush out metadata from a
> > separate process (jbd2 process in ext4 case, worker thread in xfs case).
> > CFQ's heuristic was able to determine that it isn't worth to idle waiting for
> > either dbench or jbd2 IO. BFQ's heuristic is not able to determine this and
> > thus jbd2 process is often blocked waiting for idle timer of dbench queue to
> > trigger.
> > 
> > The first patch in the series is an obvious bugfix but is not enough to improve
> > performance. The second patch does improve dbench performance from ~80 MB/s
> > to ~200 MB/s on my test machine but I'm aware that it is probably way too
> > aggressive and probably a different solution is needed. So I just wrote that
> > patch to see the results and spark some discussion :). Any idea how to
> > improve the waker logic so that dbench performance doesn't drop so
> > dramatically?
> > 
> > 								Honza
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] bfq: Allow short_ttime queues to have waker

[Paolo Valente]

> Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> 
> Currently queues that have average think time shorter than slice_idle
> cannot have waker. However this requirement is too strict. E.g. dbench
> process always submits a one or two IOs (which is enough to pull its
> average think time below slice_idle) and then blocks waiting for jbd2
> thread to commit a transaction. Due to idling logic jbd2 thread is
> often forced to wait for dbench's idle timer to trigger to be able to
> submit its IO and this severely delays the overall benchmark progress.
> 
> E.g. on my test machine current dbench single-thread throughput is ~80
> MB/s, with this patch it is ~200 MB/s.
> 

Hi Jan,
I've modified this logic a little bit (in patches that I'm going to
submit).  And I don't see your boost in my tests.  So it's difficult
for me to validate this change.  If ok for you, you could test it on
top of the patches that I'll submit.  If you see a boost, and (as I
expect) I won't see any regression, this improvement is very welcome
for me.

Thanks,
Paolo

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> block/bfq-iosched.c | 1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 18f85d474c9c..416473ba80c8 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -1928,7 +1928,6 @@ static void bfq_add_request(struct request *rq)
> 		 * I/O-plugging interval for bfqq.
> 		 */
> 		if (bfqd->last_completed_rq_bfqq &&
> -		    !bfq_bfqq_has_short_ttime(bfqq) &&
> 		    ktime_get_ns() - bfqd->last_completion <
> 		    200 * NSEC_PER_USEC) {
> 			if (bfqd->last_completed_rq_bfqq != bfqq &&
> -- 
> 2.16.4
> 

Re: [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected

[Paolo Valente]

> Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> 
> The check in bfq_select_queue() checking whether a waker queue should be
> selected has a bug and is checking bfqq->next_rq instead of
> bfqq->waker_bfqq->next_rq to verify whether the waker queue has a
> request to dispatch. This often results in the condition being false
> (most notably when the current queue is idling waiting for next request)
> and thus the waker queue logic is ineffective. Fix the condition.
> 

Hi Jan,
my huge delay causes problems again, because a student of mine already
made this same fix a few months ago.  But I did not send it out yet,
for lack of time.  If ok for you, we could go for a common commit with
two authors (I seem to remember it is feasible).

Thanks.
Paolo

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> 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 78ba57efd16b..18f85d474c9c 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -4498,7 +4498,7 @@ static struct bfq_queue *bfq_select_queue(struct bfq_data *bfqd)
> 			bfqq = bfqq->bic->bfqq[0];
> 		else if (bfq_bfqq_has_waker(bfqq) &&
> 			   bfq_bfqq_busy(bfqq->waker_bfqq) &&
> -			   bfqq->next_rq &&
> +			   bfqq->waker_bfqq->next_rq &&
> 			   bfq_serv_to_charge(bfqq->waker_bfqq->next_rq,
> 					      bfqq->waker_bfqq) <=
> 			   bfq_bfqq_budget_left(bfqq->waker_bfqq)
> -- 
> 2.16.4
> 

Re: [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected

[Jan Kara]

On Sun 10-01-21 10:20:22, Paolo Valente wrote:
> 
> 
> > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> > 
> > The check in bfq_select_queue() checking whether a waker queue should be
> > selected has a bug and is checking bfqq->next_rq instead of
> > bfqq->waker_bfqq->next_rq to verify whether the waker queue has a
> > request to dispatch. This often results in the condition being false
> > (most notably when the current queue is idling waiting for next request)
> > and thus the waker queue logic is ineffective. Fix the condition.
> > 
> 
> Hi Jan,
> my huge delay causes problems again, because a student of mine already
> made this same fix a few months ago.  But I did not send it out yet,
> for lack of time.  If ok for you, we could go for a common commit with
> two authors (I seem to remember it is feasible).

No problem for me. Or just give the student a credit instead of me. A
commit in the kernel is likely more interesting for him than for me ;) Just
reply to the v2 series I've sent today (you should be on CC) so that Jens
knows the author should be changed.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] bfq: Fix check detecting whether waker queue should be selected

[Jan Kara]

On Wed 13-01-21 14:09:33, Jan Kara wrote:
> On Sun 10-01-21 10:20:22, Paolo Valente wrote:
> > 
> > 
> > > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> > > 
> > > The check in bfq_select_queue() checking whether a waker queue should be
> > > selected has a bug and is checking bfqq->next_rq instead of
> > > bfqq->waker_bfqq->next_rq to verify whether the waker queue has a
> > > request to dispatch. This often results in the condition being false
> > > (most notably when the current queue is idling waiting for next request)
> > > and thus the waker queue logic is ineffective. Fix the condition.
> > > 
> > 
> > Hi Jan,
> > my huge delay causes problems again, because a student of mine already
> > made this same fix a few months ago.  But I did not send it out yet,
> > for lack of time.  If ok for you, we could go for a common commit with
> > two authors (I seem to remember it is feasible).
> 
> No problem for me. Or just give the student a credit instead of me. A
> commit in the kernel is likely more interesting for him than for me ;) Just
> reply to the v2 series I've sent today (you should be on CC) so that Jens
> knows the author should be changed.

Oh, this is a different patch than I thought. I didn't resubmit this one.
Nevertheless "I don't care" still holds :). Just submit whatever you find fit.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 2/2] bfq: Allow short_ttime queues to have waker

[Jan Kara]

On Sun 10-01-21 10:20:19, Paolo Valente wrote:
> 
> 
> > Il giorno 9 apr 2020, alle ore 19:09, Jan Kara <jack@suse.cz> ha scritto:
> > 
> > Currently queues that have average think time shorter than slice_idle
> > cannot have waker. However this requirement is too strict. E.g. dbench
> > process always submits a one or two IOs (which is enough to pull its
> > average think time below slice_idle) and then blocks waiting for jbd2
> > thread to commit a transaction. Due to idling logic jbd2 thread is
> > often forced to wait for dbench's idle timer to trigger to be able to
> > submit its IO and this severely delays the overall benchmark progress.
> > 
> > E.g. on my test machine current dbench single-thread throughput is ~80
> > MB/s, with this patch it is ~200 MB/s.
> > 
> 
> Hi Jan,
> I've modified this logic a little bit (in patches that I'm going to
> submit).  And I don't see your boost in my tests.  So it's difficult
> for me to validate this change.  If ok for you, you could test it on
> top of the patches that I'll submit.  If you see a boost, and (as I
> expect) I won't see any regression, this improvement is very welcome
> for me.

As I wrote in the cover letter, I'm not really convinced that patch
conceptually makes sence. What you later implemented should be definitely a
more sophisticated approach to the problem. So I'd just discard this patch.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR