wbt_lat_usec still set despite wbt disabled by BFQ

wbt_lat_usec still set despite wbt disabled by BFQ

[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

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/block/bfq-iosched.c#n7195

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[Yu Kuai]

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 
> 
> 
> .
> 

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[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"

cheers
Holger

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[Yu Kuai]

在 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
> .
> 

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[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

Sorry :)

Holger

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[Yu Kuai]

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
> 
> .
> 

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[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.

cheers!
Holger

Re: wbt_lat_usec still set despite wbt disabled by BFQ

[Yu Kuai]

在 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
> 
> .
>