[PATCH] bfq-mq, bfq-sq: Disable writeback throttling

[Luca Miccio]

Similarly to CFQ, BFQ has its write-throttling heuristics, and it
is better not to combine them with further write-throttling
heuristics of a different nature.
So this commit disables write-back throttling for a device if BFQ
is used as I/O scheduler for that device.

Signed-off-by: Luca Miccio <lucm...@gmail.com>
Signed-off-by: Paolo Valente <paolo....@linaro.org>
---
block/bfq-mq-iosched.c | 2 ++
block/bfq-sq-iosched.c | 7 +++++++
2 files changed, 9 insertions(+)

diff --git a/block/bfq-mq-iosched.c b/block/bfq-mq-iosched.c
index 8c5dce7716cb..f378519b6d33 100644
--- a/block/bfq-mq-iosched.c
+++ b/block/bfq-mq-iosched.c
@@ -89,6 +89,7 @@
#include "blk-mq-tag.h"
#include "blk-mq-sched.h"
#include "bfq-mq.h"
+#include "blk-wbt.h"

/* Expiration time of sync (0) and async (1) requests, in ns. */
static const u64 bfq_fifo_expire[2] = { NSEC_PER_SEC / 4, NSEC_PER_SEC / 8 };
@@ -5272,6 +5273,7 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
bfq_init_root_group(bfqd->root_group, bfqd);
bfq_init_entity(&bfqd->oom_bfqq.entity, bfqd->root_group);

+ wbt_disable_default(q);
return 0;

out_free:
diff --git a/block/bfq-sq-iosched.c b/block/bfq-sq-iosched.c
index 5b456e48e0cc..f4654436cd55 100644
--- a/block/bfq-sq-iosched.c
+++ b/block/bfq-sq-iosched.c
@@ -83,6 +83,7 @@
#include <linux/ioprio.h>
#include "blk.h"
#include "bfq.h"
+#include "blk-wbt.h"

/* Expiration time of sync (0) and async (1) requests, in ns. */
static const u64 bfq_fifo_expire[2] = { NSEC_PER_SEC / 4, NSEC_PER_SEC / 8 };
@@ -4988,6 +4989,11 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
return -ENOMEM;
}

+static void bfq_registered_queue(struct request_queue *q)
+{
+ wbt_disable_default(q);
+}
+
static void bfq_slab_kill(void)
{
kmem_cache_destroy(bfq_pool);
@@ -5297,6 +5303,7 @@ static struct elevator_type iosched_bfq = {
.elevator_may_queue_fn = bfq_may_queue,
.elevator_init_fn = bfq_init_queue,
.elevator_exit_fn = bfq_exit_queue,
+ .elevator_registered_fn = bfq_registered_queue,
},
.icq_size = sizeof(struct bfq_io_cq),
.icq_align = __alignof__(struct bfq_io_cq),
--
2.11.0

[post-factum]

Feel free to add:

Tested-by: Oleksandr Natalenko <olek...@natalenko.name>

since it builds and runs okay for me [1].

[1] https://github.com/pfactum/pf-kernel/commit/bff4494a133d6c8c55f28a3a0afd8457bbf3a94b

середа, 13 вересня 2017 р. 12:03:59 UTC+2 користувач Luca Miccio написав:

[Luca Miccio]

2017-09-13 13:55 GMT+02:00 post-factum <pfa...@gmail.com>:

Should this be considered instead of what you've already submitted [1]?

[1] https://marc.info/?l=linux-block&m=150486424501778&w=2

This is just the port of the patch that you have mentioned, which is for mainline, for 

the bfq-mq tree on Github.

Thanks,

Luca. 

середа, 13 вересня 2017 р. 12:03:59 UTC+2 користувач Luca Miccio написав:

Similarly to CFQ, BFQ has its write-throttling heuristics, and it

--
You received this message because you are subscribed to the Google Groups "bfq-iosched" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Paolo Valente]

> Il giorno 13 set 2017, alle ore 13:55, post-factum <pfa...@gmail.com> ha scritto:
>
> Should this be considered instead of what you've already submitted [1]?
>

This patch by Luca is just the port for bfq-mq and bfq-sq of [1], i.e., of what he has proposed for mainline.

Paolo

> [1] https://marc.info/?l=linux-block&m=150486424501778&w=2

>
> середа, 13 вересня 2017 р. 12:03:59 UTC+2 користувач Luca Miccio написав:

> --
> You received this message because you are subscribed to the Google Groups "bfq-iosched" group.

> To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched...@googlegroups.com.

> For more options, visit https://groups.google.com/d/optout.

.

[post-factum]

Yup, I've realized that after posting, but still removed my comment immediately ;).

середа, 13 вересня 2017 р. 14:37:44 UTC+2 користувач Luca Miccio написав:

To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched...@googlegroups.com.

[Paolo Valente]

> Il giorno 13 set 2017, alle ore 13:59, post-factum <pfa...@gmail.com> ha scritto:
>
> Feel free to add:
>
> Tested-by: Oleksandr Natalenko <olek...@natalenko.name>
>

Great! If you can, please send this same reply for the mainline patch as well, so as to help the patch being considered by Jens.

Thanks,
Paolo

> --
> You received this message because you are subscribed to the Google Groups "bfq-iosched" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

.

[Paolo Valente]

> Il giorno 13 set 2017, alle ore 12:03, Luca Miccio <lucm...@gmail.com> ha scritto:
>
> Similarly to CFQ, BFQ has its write-throttling heuristics, and it
> is better not to combine them with further write-throttling
> heuristics of a different nature.
> So this commit disables write-back throttling for a device if BFQ
> is used as I/O scheduler for that device.
>

Applied! Thank you very much Luca, and sorry for the delay.

Paolo

[Lee Tibbert]

Luca,

Thank you for taking the time to create this patch. 

Paolo had asked that I add a tested-by: to the linux-block list.

I am trying to figure out how to successfully send to that list (Rats!)
but let me get this info to you whilst I thrash that out.

I had been running this patch for about three weeks with no problem.
That showed me that it appeared to introduce no problems.
So that I felt confident to add a tested-by I needed to convince myself
that the patch did what it said it did.

The short story is that, with one small issue, the patch is fine on my
system.  The small issue is that block/blk-wbt.c line 657 says:
   
    Disable wbt, if enabled by default. Only called from CFQ.

As of this patch, that comment is no longer true.  Adding "& BFQ" is
the obvious edit, but simply deleting the "Only" sentence is probably
more robust.

Longer story:

      I tested a number of paths both  CONFIG_BLK_WBT enabled &
      disabled, etc.

      In particular, I tested that when CONFIB)BLK_WBT is enabled
     sys/block/sda/queue/wbt_lat_usec is cleared when the elevator
     is set to BFQ(BFQ-MQ). Patch does what it purports.

     I also tested that wbt_lat_usec gets set back to its default when
     an elevator other than BFQ is selected after BFQ is active. (Yeah,
     the new elevator probably does this. There is no code in BFQ to do it).

     This patch makes it easier to get write back settings right when using
     cgroups.  With cgroup, on wants  CONFIG_BLK_DEV_THROTTLING on
     but     CONFIG_BLK_WBT off. This patch painlessly assures the latter
     and reduces, at least my, confusion.

Thank you.

Lee

[Lee Tibbert]

Let me ask here, where it is relatively safe & friendly before
I embarass myself on linux-block.

What is the current thinking about comment only patches?
If allowed, do they get sent to only linux-block or to both
linux-block & Jens (who probably reads linux-block).

There is a sporting chance that I could create some
cycles to patch the comment in  block/blk-wbt.c at line 657
I noted above.  Deleting the "Only called from CFQ." sentence
would leave blk-wbt.c correct both with & without Luca's
patch.

My thought is not to create extra work for anybody but to
both correct a small pending error and, perhaps, refresh
interest in Luca's patch.

Please advise.  I am well aware that time is precious for
everyone.

Lee

[Luca Miccio]

Hi Lee,

sorry if i reply to you too late but i am very busy with my degree. Don't worry, i have read your comments about

this patch and tomorrow i will push the V2 with also the tested-by. 

After the V2 for the linux-block i will also push the V2 for the bfq-mq branch.

I take the opportunity to thank you once again for your help and also for testing every single change on BFQ.

I really appreciate it.

Thanks again,

Luca.

--
You received this message because you are subscribed to the Google Groups "bfq-iosched" group.

To unsubscribe from this group and stop receiving emails from it, send an email to bfq-iosched+unsubscribe@googlegroups.com.

[Lee Tibbert]

On Tuesday, October 3, 2017 at 1:44:35 PM UTC-4, Luca Miccio wrote:

Hi Lee,

sorry if i reply to you too late but i am very busy with my degree. Don't worry, i have read your comments about

this patch and tomorrow i will push the V2 with also the tested-by. 

After the V2 for the linux-block i will also push the V2 for the bfq-mq branch.

I take the opportunity to thank you once again for your help and also for testing every single change on BFQ.

I really appreciate it.

Thanks again,

Luca.

Understood! Yes, I know all to well how hard the push to a degree is.

More power to you.

I was  not trying to push the river. Sorry if I increased pressure.

We had a saying where I used to work:   He/she who proposes, disposes...

That cut down on _lots_ of bikeshedding, especially around

deadline time.

I was feeling guilty having found something I could easily fix

and not having volunteered to fix it.

Yell if I can be useful, otherwise I have plenty to keep me busy...

Lee

[Luca Miccio]

This is the v2 of the previous patch[1]. There are
no functional changes. The only difference from v1 to
v2 is the refactor of the wbt_disable_default's comment,
thanks to Lee Tibbert who pointed me out that mistake.

[1]: https://github.com/Algodev-github/bfq-mq/commit/ff43ee1bf38a0c33d052f4dd054a5e2364e02c80

Luca Miccio (1):
bfq-mq, bfq-sq: Disable writeback throttling

block/bfq-mq-iosched.c | 2 ++
block/bfq-sq-iosched.c | 7 +++++++

block/blk-wbt.c | 2 +-
3 files changed, 10 insertions(+), 1 deletion(-)

--
2.11.0

[Luca Miccio]

Similarly to CFQ, BFQ has its write-throttling heuristics, and it
is better not to combine them with further write-throttling
heuristics of a different nature.
So this commit disables write-back throttling for a device if BFQ
is used as I/O scheduler for that device.

Signed-off-by: Luca Miccio <lucm...@gmail.com>
Signed-off-by: Paolo Valente <paolo....@linaro.org>

Tested-by: Lee Tibbert <lee.t...@gmail.com>
Tested-by: Oleksandr Natalenko <olek...@natalenko.name>
---

block/bfq-mq-iosched.c | 2 ++
block/bfq-sq-iosched.c | 7 +++++++
block/blk-wbt.c | 2 +-
3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/block/bfq-mq-iosched.c b/block/bfq-mq-iosched.c
index b5c848650375..7d27d5b3befb 100644

--- a/block/bfq-mq-iosched.c
+++ b/block/bfq-mq-iosched.c
@@ -89,6 +89,7 @@
#include "blk-mq-tag.h"
#include "blk-mq-sched.h"
#include "bfq-mq.h"
+#include "blk-wbt.h"

/* Expiration time of sync (0) and async (1) requests, in ns. */
static const u64 bfq_fifo_expire[2] = { NSEC_PER_SEC / 4, NSEC_PER_SEC / 8 };

@@ -5260,6 +5261,7 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)

bfq_init_root_group(bfqd->root_group, bfqd);
bfq_init_entity(&bfqd->oom_bfqq.entity, bfqd->root_group);

+ wbt_disable_default(q);
return 0;

out_free:
diff --git a/block/bfq-sq-iosched.c b/block/bfq-sq-iosched.c

index 42393ab889a9..6fdc3b1d5bb8 100644

--- a/block/bfq-sq-iosched.c
+++ b/block/bfq-sq-iosched.c
@@ -83,6 +83,7 @@
#include <linux/ioprio.h>
#include "blk.h"
#include "bfq.h"
+#include "blk-wbt.h"

/* Expiration time of sync (0) and async (1) requests, in ns. */
static const u64 bfq_fifo_expire[2] = { NSEC_PER_SEC / 4, NSEC_PER_SEC / 8 };

@@ -4976,6 +4977,11 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)

return -ENOMEM;
}

+static void bfq_registered_queue(struct request_queue *q)
+{
+ wbt_disable_default(q);
+}
+
static void bfq_slab_kill(void)
{
kmem_cache_destroy(bfq_pool);

@@ -5285,6 +5291,7 @@ static struct elevator_type iosched_bfq = {

.elevator_may_queue_fn = bfq_may_queue,
.elevator_init_fn = bfq_init_queue,
.elevator_exit_fn = bfq_exit_queue,
+ .elevator_registered_fn = bfq_registered_queue,
},
.icq_size = sizeof(struct bfq_io_cq),
.icq_align = __alignof__(struct bfq_io_cq),

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 6a9a0f03a67b..e59d59c11ebb 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -654,7 +654,7 @@ void wbt_set_write_cache(struct rq_wb *rwb, bool write_cache_on)
}

/*
- * Disable wbt, if enabled by default. Only called from CFQ.
+ * Disable wbt, if enabled by default.
*/
void wbt_disable_default(struct request_queue *q)
{
--
2.11.0