* wbt_lat_usec still set despite wbt disabled by BFQ
@ 2022-08-17 17:12 Holger Hoffstätte
2022-09-13 13:39 ` Yu Kuai
0 siblings, 1 reply; 8+ messages in thread
From: Holger Hoffstätte @ 2022-08-17 17:12 UTC (permalink / raw)
To: linux-block
I just noticed that my device configured with BFQ still shows wbt_lat_usec
as configured, despite the fact that BFQ disables WBT in bfq_init_queue [1]:
$cat /sys/block/sdc/queue/scheduler
mq-deadline [bfq] none
$cat /sys/block/sdc/queue/wbt_lat_usec
75000
Is this supposed to be 0 (since it's disabled) or is sysfs confused?
Thanks,
Holger
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bfq-iosched.c#n7195
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-08-17 17:12 wbt_lat_usec still set despite wbt disabled by BFQ Holger Hoffstätte
@ 2022-09-13 13:39 ` Yu Kuai
2022-09-13 14:12 ` Holger Hoffstätte
0 siblings, 1 reply; 8+ messages in thread
From: Yu Kuai @ 2022-09-13 13:39 UTC (permalink / raw)
To: Holger Hoffstätte, linux-block, Yu Kuai, yukuai (C), zhangyi (F)
Hi, Holger
在 2022/08/18 1:12, Holger Hoffstätte 写道:
>
> I just noticed that my device configured with BFQ still shows wbt_lat_usec
> as configured, despite the fact that BFQ disables WBT in bfq_init_queue
> [1]:
>
> $cat /sys/block/sdc/queue/scheduler
> mq-deadline [bfq] none
> $cat /sys/block/sdc/queue/wbt_lat_usec
> 75000
>
> Is this supposed to be 0 (since it's disabled) or is sysfs confused?
>
> Thanks,
> Holger
I'm reviewing wbt codes recently, and I found that this problem will
happen if the default elevator is bfq. I'll try to fix this, do you mind
if I add reported-by tag?
Thanks,
Kuai
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bfq-iosched.c#n7195
>
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-09-13 13:39 ` Yu Kuai
@ 2022-09-13 14:12 ` Holger Hoffstätte
2022-09-13 14:20 ` Yu Kuai
0 siblings, 1 reply; 8+ messages in thread
From: Holger Hoffstätte @ 2022-09-13 14:12 UTC (permalink / raw)
To: Yu Kuai, linux-block, yukuai (C), zhangyi (F)
On 2022-09-13 15:39, Yu Kuai wrote:
> Hi, Holger
>
> 在 2022/08/18 1:12, Holger Hoffstätte 写道:
>>
>> I just noticed that my device configured with BFQ still shows wbt_lat_usec
>> as configured, despite the fact that BFQ disables WBT in bfq_init_queue [1]:
>>
>> $cat /sys/block/sdc/queue/scheduler
>> mq-deadline [bfq] none
>> $cat /sys/block/sdc/queue/wbt_lat_usec
>> 75000
>>
>> Is this supposed to be 0 (since it's disabled) or is sysfs confused?
>>
>> Thanks,
>> Holger
>
> I'm reviewing wbt codes recently, and I found that this problem will
> happen if the default elevator is bfq. I'll try to fix this, do you mind
> if I add reported-by tag?
Do not mind at all - thank you for looking into it!
Let me know if I can test a patch or help in some other way.
Btw not sure what "default scheduler" means here, I set my schedulers via
udev rules. In this case:
ACTION=="add", KERNEL=="sd[a-z]", ATTR{queue/rotational}=="1", ATTR{queue/scheduler}="bfq"
cheers
Holger
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-09-13 14:12 ` Holger Hoffstätte
@ 2022-09-13 14:20 ` Yu Kuai
2022-09-13 16:13 ` Holger Hoffstätte
0 siblings, 1 reply; 8+ messages in thread
From: Yu Kuai @ 2022-09-13 14:20 UTC (permalink / raw)
To: Holger Hoffstätte, Yu Kuai, linux-block, zhangyi (F), yukuai (C)
在 2022/09/13 22:12, Holger Hoffstätte 写道:
> On 2022-09-13 15:39, Yu Kuai wrote:
>> Hi, Holger
>>
>> 在 2022/08/18 1:12, Holger Hoffstätte 写道:
>>>
>>> I just noticed that my device configured with BFQ still shows
>>> wbt_lat_usec
>>> as configured, despite the fact that BFQ disables WBT in
>>> bfq_init_queue [1]:
>>>
>>> $cat /sys/block/sdc/queue/scheduler
>>> mq-deadline [bfq] none
>>> $cat /sys/block/sdc/queue/wbt_lat_usec
>>> 75000
>>>
>>> Is this supposed to be 0 (since it's disabled) or is sysfs confused?
>>>
>>> Thanks,
>>> Holger
>>
>> I'm reviewing wbt codes recently, and I found that this problem will
>> happen if the default elevator is bfq. I'll try to fix this, do you mind
>> if I add reported-by tag?
>
> Do not mind at all - thank you for looking into it!
> Let me know if I can test a patch or help in some other way.
>
> Btw not sure what "default scheduler" means here, I set my schedulers via
> udev rules. In this case:
>
> ACTION=="add", KERNEL=="sd[a-z]", ATTR{queue/rotational}=="1",
> ATTR{queue/scheduler}="bfq"
>
Default means the elevator is bfq when device is created.
Perhaps can you try the following patch?
blk-wbt: don't enable throttling if default elevator is bfq
Commit b5dc5d4d1f4f ("block,bfq: Disable writeback throttling")
tries to
disable wbt for bfq, it's done by calling wbt_disable_default() in
bfq_init_queue(). However, wbt is still enabled if default elevator is
bfq:
device_add_disk
elevator_init_mq
bfq_init_queue
wbt_disable_default -> done nothing
blk_register_queue
wbt_enable_default -> wbt is enabled
Fix the problem by checking elevator name if wbt_enable_default() is
called from blk_register_queue().
diff --git a/block/elevator.h b/block/elevator.h
index 3f0593b3bf9d..ccded343cf27 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -104,6 +104,11 @@ struct elevator_queue
DECLARE_HASHTABLE(hash, ELV_HASH_BITS);
};
+static inline bool check_elevator_name(struct elevator_queue *elevator,
+ const char *name)
+{
+ return !strcmp(elevator->type->elevator_name, name);
+}
/*
* block elevator interface
*/
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 7ea427817f7f..f769c90744fd 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -7045,7 +7045,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
#endif
blk_stat_disable_accounting(bfqd->queue);
- wbt_enable_default(bfqd->queue);
+ wbt_enable_default(bfqd->queue, false);
kfree(bfqd);
}
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e1f009aba6fd..a630d657c054 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -843,7 +843,7 @@ int blk_register_queue(struct gendisk *disk)
goto put_dev;
blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q);
- wbt_enable_default(q);
+ wbt_enable_default(q, true);
blk_throtl_register_queue(q);
/* Now everything is ready and send out KOBJ_ADD uevent */
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 246467926253..26ee6ca66a93 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -27,6 +27,7 @@
#include "blk-wbt.h"
#include "blk-rq-qos.h"
+#include "elevator.h"
#define CREATE_TRACE_POINTS
#include <trace/events/wbt.h>
@@ -636,10 +637,14 @@ void wbt_set_write_cache(struct request_queue *q,
bool write_cache_on)
/*
* Enable wbt if defaults are configured that way
*/
-void wbt_enable_default(struct request_queue *q)
+void wbt_enable_default(struct request_queue *q, bool check_elevator)
{
struct rq_qos *rqos = wbt_rq_qos(q);
+ if (check_elevator && q->elevator &&
+ check_elevator_name(q->elevator, "bfq"))
+ return;
+
/* Throttling already enabled? */
if (rqos) {
if (RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
diff --git a/block/blk-wbt.h b/block/blk-wbt.h
index 7e44eccc676d..1a49b6ac397c 100644
--- a/block/blk-wbt.h
+++ b/block/blk-wbt.h
@@ -90,7 +90,7 @@ static inline unsigned int wbt_inflight(struct rq_wb *rwb)
int wbt_init(struct request_queue *);
void wbt_disable_default(struct request_queue *);
-void wbt_enable_default(struct request_queue *);
+void wbt_enable_default(struct request_queue *, bool);
u64 wbt_get_min_lat(struct request_queue *q);
void wbt_set_min_lat(struct request_queue *q, u64 val);
@@ -108,7 +108,8 @@ static inline int wbt_init(struct request_queue *q)
static inline void wbt_disable_default(struct request_queue *q)
{
}
-static inline void wbt_enable_default(struct request_queue *q)
+static inline void wbt_enable_default(struct request_queue *q,
+ bool check_elevator)
{
}
static inline void wbt_set_write_cache(struct request_queue *q, bool wc)
> cheers
> Holger
> .
>
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-09-13 14:20 ` Yu Kuai
@ 2022-09-13 16:13 ` Holger Hoffstätte
2022-09-14 1:35 ` Yu Kuai
0 siblings, 1 reply; 8+ messages in thread
From: Holger Hoffstätte @ 2022-09-13 16:13 UTC (permalink / raw)
To: Yu Kuai, linux-block, zhangyi (F), yukuai (C)
On 2022-09-13 16:20, Yu Kuai wrote:
>
>
> 在 2022/09/13 22:12, Holger Hoffstätte 写道:
>> On 2022-09-13 15:39, Yu Kuai wrote:
>>> Hi, Holger
>>>
>>> 在 2022/08/18 1:12, Holger Hoffstätte 写道:
>>>>
>>>> I just noticed that my device configured with BFQ still shows wbt_lat_usec
>>>> as configured, despite the fact that BFQ disables WBT in bfq_init_queue [1]:
>>>>
>>>> $cat /sys/block/sdc/queue/scheduler
>>>> mq-deadline [bfq] none
>>>> $cat /sys/block/sdc/queue/wbt_lat_usec
>>>> 75000
>>>>
>>>> Is this supposed to be 0 (since it's disabled) or is sysfs confused?
>>>>
>>>> Thanks,
>>>> Holger
>>>
>>> I'm reviewing wbt codes recently, and I found that this problem will
>>> happen if the default elevator is bfq. I'll try to fix this, do you mind
>>> if I add reported-by tag?
>>
>> Do not mind at all - thank you for looking into it!
>> Let me know if I can test a patch or help in some other way.
>>
>> Btw not sure what "default scheduler" means here, I set my schedulers via
>> udev rules. In this case:
>>
>> ACTION=="add", KERNEL=="sd[a-z]", ATTR{queue/rotational}=="1", ATTR{queue/scheduler}="bfq"
>>
>
> Default means the elevator is bfq when device is created.
>
> Perhaps can you try the following patch?
>
> blk-wbt: don't enable throttling if default elevator is bfq
>
> Commit b5dc5d4d1f4f ("block,bfq: Disable writeback throttling") tries to
> disable wbt for bfq, it's done by calling wbt_disable_default() in
> bfq_init_queue(). However, wbt is still enabled if default elevator is
> bfq:
>
> device_add_disk
> elevator_init_mq
> bfq_init_queue
> wbt_disable_default -> done nothing
>
> blk_register_queue
> wbt_enable_default -> wbt is enabled
>
> Fix the problem by checking elevator name if wbt_enable_default() is
> called from blk_register_queue().
>
>
> diff --git a/block/elevator.h b/block/elevator.h
> index 3f0593b3bf9d..ccded343cf27 100644
> --- a/block/elevator.h
> +++ b/block/elevator.h
> @@ -104,6 +104,11 @@ struct elevator_queue
> DECLARE_HASHTABLE(hash, ELV_HASH_BITS);
> };
>
> +static inline bool check_elevator_name(struct elevator_queue *elevator,
> + const char *name)
> +{
> + return !strcmp(elevator->type->elevator_name, name);
> +}
> /*
> * block elevator interface
> */
>
> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
> index 7ea427817f7f..f769c90744fd 100644
> --- a/block/bfq-iosched.c
> +++ b/block/bfq-iosched.c
> @@ -7045,7 +7045,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
> #endif
>
> blk_stat_disable_accounting(bfqd->queue);
> - wbt_enable_default(bfqd->queue);
> + wbt_enable_default(bfqd->queue, false);
>
> kfree(bfqd);
> }
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index e1f009aba6fd..a630d657c054 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -843,7 +843,7 @@ int blk_register_queue(struct gendisk *disk)
> goto put_dev;
>
> blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q);
> - wbt_enable_default(q);
> + wbt_enable_default(q, true);
> blk_throtl_register_queue(q);
>
> /* Now everything is ready and send out KOBJ_ADD uevent */
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index 246467926253..26ee6ca66a93 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -27,6 +27,7 @@
>
> #include "blk-wbt.h"
> #include "blk-rq-qos.h"
> +#include "elevator.h"
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/wbt.h>
> @@ -636,10 +637,14 @@ void wbt_set_write_cache(struct request_queue *q, bool write_cache_on)
> /*
> * Enable wbt if defaults are configured that way
> */
> -void wbt_enable_default(struct request_queue *q)
> +void wbt_enable_default(struct request_queue *q, bool check_elevator)
> {
> struct rq_qos *rqos = wbt_rq_qos(q);
>
> + if (check_elevator && q->elevator &&
> + check_elevator_name(q->elevator, "bfq"))
> + return;
> +
> /* Throttling already enabled? */
> if (rqos) {
> if (RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
> diff --git a/block/blk-wbt.h b/block/blk-wbt.h
> index 7e44eccc676d..1a49b6ac397c 100644
> --- a/block/blk-wbt.h
> +++ b/block/blk-wbt.h
> @@ -90,7 +90,7 @@ static inline unsigned int wbt_inflight(struct rq_wb *rwb)
>
> int wbt_init(struct request_queue *);
> void wbt_disable_default(struct request_queue *);
> -void wbt_enable_default(struct request_queue *);
> +void wbt_enable_default(struct request_queue *, bool);
>
> u64 wbt_get_min_lat(struct request_queue *q);
> void wbt_set_min_lat(struct request_queue *q, u64 val);
> @@ -108,7 +108,8 @@ static inline int wbt_init(struct request_queue *q)
> static inline void wbt_disable_default(struct request_queue *q)
> {
> }
> -static inline void wbt_enable_default(struct request_queue *q)
> +static inline void wbt_enable_default(struct request_queue *q,
> + bool check_elevator)
> {
> }
> static inline void wbt_set_write_cache(struct request_queue *q, bool wc)
>
So that didn't help, unfortunately.
Directly after boot (with the above udev rule):
$cat /sys/block/sdc/queue/scheduler
mq-deadline [bfq] none
$cat /sys/block/sdc/queue/wbt_lat_usec
75000
Changing the scheduler back and forth also does not help:
$echo mq-deadline > /sys/block/sdc/queue/scheduler
$echo bfq > /sys/block/sdc/queue/scheduler
$cat /sys/block/sdc/queue/wbt_lat_usec
75000
Sorry :)
Holger
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-09-13 16:13 ` Holger Hoffstätte
@ 2022-09-14 1:35 ` Yu Kuai
2022-09-14 6:05 ` Holger Hoffstätte
0 siblings, 1 reply; 8+ messages in thread
From: Yu Kuai @ 2022-09-14 1:35 UTC (permalink / raw)
To: Holger Hoffstätte, Yu Kuai, linux-block, zhangyi (F), yukuai (C)
Hi, Holger
在 2022/09/14 0:13, Holger Hoffstätte 写道:
> On 2022-09-13 16:20, Yu Kuai wrote:
>>
>>
>> 在 2022/09/13 22:12, Holger Hoffstätte 写道:
>>> On 2022-09-13 15:39, Yu Kuai wrote:
>>>> Hi, Holger
>>>>
>>>> 在 2022/08/18 1:12, Holger Hoffstätte 写道:
>>>>>
>>>>> I just noticed that my device configured with BFQ still shows
>>>>> wbt_lat_usec
>>>>> as configured, despite the fact that BFQ disables WBT in
>>>>> bfq_init_queue [1]:
>>>>>
>>>>> $cat /sys/block/sdc/queue/scheduler
>>>>> mq-deadline [bfq] none
>>>>> $cat /sys/block/sdc/queue/wbt_lat_usec
>>>>> 75000
>>>>>
>>>>> Is this supposed to be 0 (since it's disabled) or is sysfs confused?
>>>>>
>>>>> Thanks,
>>>>> Holger
>>>>
>>>> I'm reviewing wbt codes recently, and I found that this problem will
>>>> happen if the default elevator is bfq. I'll try to fix this, do you
>>>> mind
>>>> if I add reported-by tag?
>>>
>>> Do not mind at all - thank you for looking into it!
>>> Let me know if I can test a patch or help in some other way.
>>>
>>> Btw not sure what "default scheduler" means here, I set my schedulers
>>> via
>>> udev rules. In this case:
>>>
>>> ACTION=="add", KERNEL=="sd[a-z]", ATTR{queue/rotational}=="1",
>>> ATTR{queue/scheduler}="bfq"
>>>
>>
>> Default means the elevator is bfq when device is created.
>>
>> Perhaps can you try the following patch?
>>
>> blk-wbt: don't enable throttling if default elevator is bfq
>>
>> Commit b5dc5d4d1f4f ("block,bfq: Disable writeback throttling")
>> tries to
>> disable wbt for bfq, it's done by calling wbt_disable_default() in
>> bfq_init_queue(). However, wbt is still enabled if default
>> elevator is
>> bfq:
>>
>> device_add_disk
>> elevator_init_mq
>> bfq_init_queue
>> wbt_disable_default -> done nothing
>>
>> blk_register_queue
>> wbt_enable_default -> wbt is enabled
>>
>> Fix the problem by checking elevator name if wbt_enable_default() is
>> called from blk_register_queue().
>>
>>
>> diff --git a/block/elevator.h b/block/elevator.h
>> index 3f0593b3bf9d..ccded343cf27 100644
>> --- a/block/elevator.h
>> +++ b/block/elevator.h
>> @@ -104,6 +104,11 @@ struct elevator_queue
>> DECLARE_HASHTABLE(hash, ELV_HASH_BITS);
>> };
>>
>> +static inline bool check_elevator_name(struct elevator_queue *elevator,
>> + const char *name)
>> +{
>> + return !strcmp(elevator->type->elevator_name, name);
>> +}
>> /*
>> * block elevator interface
>> */
>>
>> diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
>> index 7ea427817f7f..f769c90744fd 100644
>> --- a/block/bfq-iosched.c
>> +++ b/block/bfq-iosched.c
>> @@ -7045,7 +7045,7 @@ static void bfq_exit_queue(struct elevator_queue
>> *e)
>> #endif
>>
>> blk_stat_disable_accounting(bfqd->queue);
>> - wbt_enable_default(bfqd->queue);
>> + wbt_enable_default(bfqd->queue, false);
>>
>> kfree(bfqd);
>> }
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index e1f009aba6fd..a630d657c054 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -843,7 +843,7 @@ int blk_register_queue(struct gendisk *disk)
>> goto put_dev;
>>
>> blk_queue_flag_set(QUEUE_FLAG_REGISTERED, q);
>> - wbt_enable_default(q);
>> + wbt_enable_default(q, true);
>> blk_throtl_register_queue(q);
>>
>> /* Now everything is ready and send out KOBJ_ADD uevent */
>> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
>> index 246467926253..26ee6ca66a93 100644
>> --- a/block/blk-wbt.c
>> +++ b/block/blk-wbt.c
>> @@ -27,6 +27,7 @@
>>
>> #include "blk-wbt.h"
>> #include "blk-rq-qos.h"
>> +#include "elevator.h"
>>
>> #define CREATE_TRACE_POINTS
>> #include <trace/events/wbt.h>
>> @@ -636,10 +637,14 @@ void wbt_set_write_cache(struct request_queue
>> *q, bool write_cache_on)
>> /*
>> * Enable wbt if defaults are configured that way
>> */
>> -void wbt_enable_default(struct request_queue *q)
>> +void wbt_enable_default(struct request_queue *q, bool check_elevator)
>> {
>> struct rq_qos *rqos = wbt_rq_qos(q);
>>
>> + if (check_elevator && q->elevator &&
>> + check_elevator_name(q->elevator, "bfq"))
>> + return;
>> +
>> /* Throttling already enabled? */
>> if (rqos) {
>> if (RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
>> diff --git a/block/blk-wbt.h b/block/blk-wbt.h
>> index 7e44eccc676d..1a49b6ac397c 100644
>> --- a/block/blk-wbt.h
>> +++ b/block/blk-wbt.h
>> @@ -90,7 +90,7 @@ static inline unsigned int wbt_inflight(struct rq_wb
>> *rwb)
>>
>> int wbt_init(struct request_queue *);
>> void wbt_disable_default(struct request_queue *);
>> -void wbt_enable_default(struct request_queue *);
>> +void wbt_enable_default(struct request_queue *, bool);
>>
>> u64 wbt_get_min_lat(struct request_queue *q);
>> void wbt_set_min_lat(struct request_queue *q, u64 val);
>> @@ -108,7 +108,8 @@ static inline int wbt_init(struct request_queue *q)
>> static inline void wbt_disable_default(struct request_queue *q)
>> {
>> }
>> -static inline void wbt_enable_default(struct request_queue *q)
>> +static inline void wbt_enable_default(struct request_queue *q,
>> + bool check_elevator)
>> {
>> }
>> static inline void wbt_set_write_cache(struct request_queue *q, bool
>> wc)
>>
>
> So that didn't help, unfortunately.
>
> Directly after boot (with the above udev rule):
>
> $cat /sys/block/sdc/queue/scheduler
> mq-deadline [bfq] none
> $cat /sys/block/sdc/queue/wbt_lat_usec
> 75000
>
> Changing the scheduler back and forth also does not help:
>
> $echo mq-deadline > /sys/block/sdc/queue/scheduler
> $echo bfq > /sys/block/sdc/queue/scheduler
> $cat /sys/block/sdc/queue/wbt_lat_usec
> 75000
Thanks for the test, it turns out this way doesn't select bfq as default
as I expected...
wbt can show min_lat_nsec despite that wbt can be disabled by
wbt_disable_default(), I do miss that previously...
Can you try the following patch again?
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index a630d657c054..3e8adb95ff02 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct
request_queue *q, const char *page,
static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
{
+ u64 lat;
+
if (!wbt_rq_qos(q))
return -EINVAL;
- return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000));
+ lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000);
+
+ return sprintf(page, "%llu\n", lat);
}
static ssize_t queue_wb_lat_store(struct request_queue *q, const char
*page,
@@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct
request_queue *q, const char *page,
return ret;
}
+ if (wbt_disabled(q))
+ return -EINVAL;
+
if (val == -1)
val = wbt_default_latency_nsec(q);
else if (val >= 0)
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 26ee6ca66a93..55d1015ef117 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -423,6 +423,13 @@ static void wbt_update_limits(struct rq_wb *rwb)
rwb_wake_all(rwb);
}
+bool wbt_disabled(struct request_queue *q)
+{
+ struct rq_qos *rqos = wbt_rq_qos(q);
+
+ return !rqos || RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT;
+}
+
u64 wbt_get_min_lat(struct request_queue *q)
{
struct rq_qos *rqos = wbt_rq_qos(q);
diff --git a/block/blk-wbt.h b/block/blk-wbt.h
index 1a49b6ac397c..4252b8077257 100644
--- a/block/blk-wbt.h
+++ b/block/blk-wbt.h
@@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *, bool);
u64 wbt_get_min_lat(struct request_queue *q);
void wbt_set_min_lat(struct request_queue *q, u64 val);
+bool wbt_disabled(struct request_queue *);
void wbt_set_write_cache(struct request_queue *, bool);
>
> Sorry :)
>
> Holger
>
> .
>
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-09-14 1:35 ` Yu Kuai
@ 2022-09-14 6:05 ` Holger Hoffstätte
2022-09-14 9:23 ` Yu Kuai
0 siblings, 1 reply; 8+ messages in thread
From: Holger Hoffstätte @ 2022-09-14 6:05 UTC (permalink / raw)
To: Yu Kuai, linux-block, zhangyi (F), yukuai (C)
On 2022-09-14 03:35, Yu Kuai wrote:
[snip]
> Thanks for the test, it turns out this way doesn't select bfq as default
> as I expected...
>
> wbt can show min_lat_nsec despite that wbt can be disabled by
> wbt_disable_default(), I do miss that previously...
>
> Can you try the following patch again?
>
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index a630d657c054..3e8adb95ff02 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page,
>
> static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
> {
> + u64 lat;
> +
> if (!wbt_rq_qos(q))
> return -EINVAL;
>
> - return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000));
> + lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000);
> +
> + return sprintf(page, "%llu\n", lat);
> }
>
> static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page,
> @@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page,
> return ret;
> }
>
> + if (wbt_disabled(q))
> + return -EINVAL;
> +
> if (val == -1)
> val = wbt_default_latency_nsec(q);
> else if (val >= 0)
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index 26ee6ca66a93..55d1015ef117 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -423,6 +423,13 @@ static void wbt_update_limits(struct rq_wb *rwb)
> rwb_wake_all(rwb);
> }
>
> +bool wbt_disabled(struct request_queue *q)
> +{
> + struct rq_qos *rqos = wbt_rq_qos(q);
> +
> + return !rqos || RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT;
> +}
> +
> u64 wbt_get_min_lat(struct request_queue *q)
> {
> struct rq_qos *rqos = wbt_rq_qos(q);
> diff --git a/block/blk-wbt.h b/block/blk-wbt.h
> index 1a49b6ac397c..4252b8077257 100644
> --- a/block/blk-wbt.h
> +++ b/block/blk-wbt.h
> @@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *, bool);
>
> u64 wbt_get_min_lat(struct request_queue *q);
> void wbt_set_min_lat(struct request_queue *q, u64 val);
> +bool wbt_disabled(struct request_queue *);
>
> void wbt_set_write_cache(struct request_queue *, bool);
>
This one works! :)
After boot:
$cat /sys/block/sdc/queue/scheduler
mq-deadline [bfq] none
$cat /sys/block/sdc/queue/wbt_lat_usec
0
Changing schedulers back and forth - here on a device with different
default - also works:
$cat /sys/block/sda/queue/scheduler
[mq-deadline] bfq none
$cat /sys/block/sda/queue/wbt_lat_usec
2000
$echo bfq > /sys/block/sda/queue/scheduler
$cat /sys/block/sda/queue/wbt_lat_usec
0
$echo deadline > /sys/block/sda/queue/scheduler
$cat /sys/block/sda/queue/wbt_lat_usec
2000
Feel free to add my Reported-by and Tested-by.
cheers!
Holger
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: wbt_lat_usec still set despite wbt disabled by BFQ
2022-09-14 6:05 ` Holger Hoffstätte
@ 2022-09-14 9:23 ` Yu Kuai
0 siblings, 0 replies; 8+ messages in thread
From: Yu Kuai @ 2022-09-14 9:23 UTC (permalink / raw)
To: Holger Hoffstätte, Yu Kuai, linux-block, zhangyi (F), yukuai (C)
在 2022/09/14 14:05, Holger Hoffstätte 写道:
> On 2022-09-14 03:35, Yu Kuai wrote:
> [snip]
>> Thanks for the test, it turns out this way doesn't select bfq as default
>> as I expected...
>>
>> wbt can show min_lat_nsec despite that wbt can be disabled by
>> wbt_disable_default(), I do miss that previously...
>>
>> Can you try the following patch again?
>>
>> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
>> index a630d657c054..3e8adb95ff02 100644
>> --- a/block/blk-sysfs.c
>> +++ b/block/blk-sysfs.c
>> @@ -467,10 +467,14 @@ static ssize_t queue_io_timeout_store(struct
>> request_queue *q, const char *page,
>>
>> static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
>> {
>> + u64 lat;
>> +
>> if (!wbt_rq_qos(q))
>> return -EINVAL;
>>
>> - return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q),
>> 1000));
>> + lat = wbt_disabled(q) ? 0 : div_u64(wbt_get_min_lat(q), 1000);
>> +
>> + return sprintf(page, "%llu\n", lat);
>> }
>>
>> static ssize_t queue_wb_lat_store(struct request_queue *q, const
>> char *page,
>> @@ -493,6 +497,9 @@ static ssize_t queue_wb_lat_store(struct
>> request_queue *q, const char *page,
>> return ret;
>> }
>>
>> + if (wbt_disabled(q))
>> + return -EINVAL;
>> +
>> if (val == -1)
>> val = wbt_default_latency_nsec(q);
>> else if (val >= 0)
>> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
>> index 26ee6ca66a93..55d1015ef117 100644
>> --- a/block/blk-wbt.c
>> +++ b/block/blk-wbt.c
>> @@ -423,6 +423,13 @@ static void wbt_update_limits(struct rq_wb *rwb)
>> rwb_wake_all(rwb);
>> }
>>
>> +bool wbt_disabled(struct request_queue *q)
>> +{
>> + struct rq_qos *rqos = wbt_rq_qos(q);
>> +
>> + return !rqos || RQWB(rqos)->enable_state ==
>> WBT_STATE_OFF_DEFAULT;
>> +}
>> +
>> u64 wbt_get_min_lat(struct request_queue *q)
>> {
>> struct rq_qos *rqos = wbt_rq_qos(q);
>> diff --git a/block/blk-wbt.h b/block/blk-wbt.h
>> index 1a49b6ac397c..4252b8077257 100644
>> --- a/block/blk-wbt.h
>> +++ b/block/blk-wbt.h
>> @@ -94,6 +94,7 @@ void wbt_enable_default(struct request_queue *, bool);
>>
>> u64 wbt_get_min_lat(struct request_queue *q);
>> void wbt_set_min_lat(struct request_queue *q, u64 val);
>> +bool wbt_disabled(struct request_queue *);
>>
>> void wbt_set_write_cache(struct request_queue *, bool);
>>
>
> This one works! :)
>
> After boot:
>
> $cat /sys/block/sdc/queue/scheduler
> mq-deadline [bfq] none
> $cat /sys/block/sdc/queue/wbt_lat_usec
> 0
>
> Changing schedulers back and forth - here on a device with different
> default - also works:
>
> $cat /sys/block/sda/queue/scheduler
> [mq-deadline] bfq none
> $cat /sys/block/sda/queue/wbt_lat_usec
> 2000
> $echo bfq > /sys/block/sda/queue/scheduler
> $cat /sys/block/sda/queue/wbt_lat_usec
> 0
> $echo deadline > /sys/block/sda/queue/scheduler
> $cat /sys/block/sda/queue/wbt_lat_usec
> 2000
>
> Feel free to add my Reported-by and Tested-by.
>
I'll send a patch soon. Thanks again for the test.
Kuai
> cheers!
> Holger
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-14 9:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-17 17:12 wbt_lat_usec still set despite wbt disabled by BFQ Holger Hoffstätte
2022-09-13 13:39 ` Yu Kuai
2022-09-13 14:12 ` Holger Hoffstätte
2022-09-13 14:20 ` Yu Kuai
2022-09-13 16:13 ` Holger Hoffstätte
2022-09-14 1:35 ` Yu Kuai
2022-09-14 6:05 ` Holger Hoffstätte
2022-09-14 9:23 ` Yu Kuai
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).