Hi, Holger

在 2022/09/14 0:13, Holger Hoffstätte 写道:

On 2022-09-13 16:20, Yu Kuai wrote:

quoted

在 2022/09/13 22:12, Holger Hoffstätte 写道:

quoted

On 2022-09-13 15:39, Yu Kuai wrote:

quoted

Hi, Holger

在 2022/08/18 1:12, Holger Hoffstätte 写道:

quoted

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

.