[PATCH v4 0/6] blk-wbt: simple improvment to enable wbt correctly

[PATCH v4 0/6] blk-wbt: simple improvment to enable wbt correctly

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

changes in v4:
 - remove patch 3 from v3
 - add patch 2,3 in v4

changes in v3:
 - instead of check elevator name, add a flag in elevator_queue, as
 suggested by Christoph.
 - add patch 3 and patch 5 to this patchset.

changes in v2:
 - define new api if wbt config is not enabled in patch 1.

Yu Kuai (6):
  elevator: remove redundant code in elv_unregister_queue()
  blk-wbt: remove unnecessary check in wbt_enable_default()
  blk-wbt: make enable_state more accurate
  blk-wbt: don't show valid wbt_lat_usec in sysfs while wbt is disabled
  elevator: add new field flags in struct elevator_queue
  blk-wbt: don't enable throttling if default elevator is bfq

 block/bfq-iosched.c |  2 ++
 block/blk-sysfs.c   |  6 +++++-
 block/blk-wbt.c     | 26 ++++++++++++++++++++++----
 block/blk-wbt.h     | 17 ++++++++++++-----
 block/elevator.c    |  8 ++------
 block/elevator.h    |  5 ++++-
 6 files changed, 47 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH v4 1/6] elevator: remove redundant code in elv_unregister_queue()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

"elevator_queue *e" is already declared and initialized in the beginning
of elv_unregister_queue().

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Eric Biggers <ebiggers@google.com>
---
 block/elevator.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index bd71f0fc4e4b..20e70fd3f77f 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -524,8 +524,6 @@ void elv_unregister_queue(struct request_queue *q)
 	lockdep_assert_held(&q->sysfs_lock);
 
 	if (e && e->registered) {
-		struct elevator_queue *e = q->elevator;
-
 		kobject_uevent(&e->kobj, KOBJ_REMOVE);
 		kobject_del(&e->kobj);
 
-- 
2.31.1

[PATCH v4 2/6] blk-wbt: remove unnecessary check in wbt_enable_default()

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

If CONFIG_BLK_WBT_MQ is disabled, wbt_init() won't do anything.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-wbt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 246467926253..4ed60dbd0756 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -651,7 +651,7 @@ void wbt_enable_default(struct request_queue *q)
 	if (!blk_queue_registered(q))
 		return;
 
-	if (queue_is_mq(q) && IS_ENABLED(CONFIG_BLK_WBT_MQ))
+	if (queue_is_mq(q))
 		wbt_init(q);
 }
 EXPORT_SYMBOL_GPL(wbt_enable_default);
-- 
2.31.1

Re: [PATCH v4 2/6] blk-wbt: remove unnecessary check in wbt_enable_default()

[Christoph Hellwig]

On Fri, Sep 30, 2022 at 11:19:02AM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> If CONFIG_BLK_WBT_MQ is disabled, wbt_init() won't do anything.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v4 3/6] blk-wbt: make enable_state more accurate

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, if user disable wbt through sysfs, 'enable_state' will be
'WBT_STATE_ON_MANUAL', which will be confusing. Add a new state
'WBT_STATE_OFF_MANUAL' to cover that case.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-wbt.c |  7 ++++++-
 block/blk-wbt.h | 12 +++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 4ed60dbd0756..9cac54569ff1 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -435,8 +435,13 @@ void wbt_set_min_lat(struct request_queue *q, u64 val)
 	struct rq_qos *rqos = wbt_rq_qos(q);
 	if (!rqos)
 		return;
+
 	RQWB(rqos)->min_lat_nsec = val;
-	RQWB(rqos)->enable_state = WBT_STATE_ON_MANUAL;
+	if (val)
+		RQWB(rqos)->enable_state = WBT_STATE_ON_MANUAL;
+	else
+		RQWB(rqos)->enable_state = WBT_STATE_OFF_MANUAL;
+
 	wbt_update_limits(RQWB(rqos));
 }
 
diff --git a/block/blk-wbt.h b/block/blk-wbt.h
index 7e44eccc676d..7fe98638fff5 100644
--- a/block/blk-wbt.h
+++ b/block/blk-wbt.h
@@ -28,13 +28,15 @@ enum {
 };
 
 /*
- * Enable states. Either off, or on by default (done at init time),
- * or on through manual setup in sysfs.
+ * If current state is WBT_STATE_ON/OFF_DEFAULT, it can be covered to any other
+ * state, if current state is WBT_STATE_ON/OFF_MANUAL, it can only be covered
+ * to WBT_STATE_OFF/ON_MANUAL.
  */
 enum {
-	WBT_STATE_ON_DEFAULT	= 1,
-	WBT_STATE_ON_MANUAL	= 2,
-	WBT_STATE_OFF_DEFAULT
+	WBT_STATE_ON_DEFAULT	= 1,	/* on by default */
+	WBT_STATE_ON_MANUAL	= 2,	/* on manually by sysfs */
+	WBT_STATE_OFF_DEFAULT	= 3,	/* off by default */
+	WBT_STATE_OFF_MANUAL	= 4,	/* off manually by sysfs */
 };
 
 struct rq_wb {
-- 
2.31.1

Re: [PATCH v4 3/6] blk-wbt: make enable_state more accurate

[Christoph Hellwig]

On Fri, Sep 30, 2022 at 11:19:03AM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Currently, if user disable wbt through sysfs, 'enable_state' will be
> 'WBT_STATE_ON_MANUAL', which will be confusing. Add a new state
> 'WBT_STATE_OFF_MANUAL' to cover that case.
> 
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v4 4/6] blk-wbt: don't show valid wbt_lat_usec in sysfs while wbt is disabled

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently, if wbt is initialized and then disabled by
wbt_disable_default(), sysfs will still show valid wbt_lat_usec, which
will confuse users that wbt is still enabled.

This patch shows wbt_lat_usec as zero if it's disabled.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reported-and-tested-by: Holger Hoffstätte <holger@applied-asynchrony.com>
---
 block/blk-sysfs.c | 6 +++++-
 block/blk-wbt.c   | 8 ++++++++
 block/blk-wbt.h   | 5 +++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index e71b3b43927c..5a6950b4ff69 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,
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 9cac54569ff1..1852ef223c68 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -422,6 +422,14 @@ 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 ||
+	       RQWB(rqos)->enable_state == WBT_STATE_OFF_MANUAL;
+}
+
 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 7fe98638fff5..e3ea6e7e2900 100644
--- a/block/blk-wbt.h
+++ b/block/blk-wbt.h
@@ -96,6 +96,7 @@ void wbt_enable_default(struct request_queue *);
 
 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);
 
@@ -127,6 +128,10 @@ static inline u64 wbt_default_latency_nsec(struct request_queue *q)
 {
 	return 0;
 }
+static inline bool wbt_disabled(struct request_queue *q)
+{
+	return true;
+}
 
 #endif /* CONFIG_BLK_WBT */
 
-- 
2.31.1

Re: [PATCH v4 4/6] blk-wbt: don't show valid wbt_lat_usec in sysfs while wbt is disabled

[Christoph Hellwig]

>  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);

	if (wbt_disabled(q))
		return sprintf(page, "0\n");
	return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000));

but otherwise the patch looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 4/6] blk-wbt: don't show valid wbt_lat_usec in sysfs while wbt is disabled

[Yu Kuai]

Hi,

在 2022/10/18 23:49, Christoph Hellwig 写道:
>>   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);
> 
> 	if (wbt_disabled(q))
> 		return sprintf(page, "0\n");
> 	return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000));
> 
> but otherwise the patch looks fine:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> .
> 

Thanks for the review, I'll send a new version.

Kuai

[PATCH v4 5/6] elevator: add new field flags in struct elevator_queue

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

There are only one flag to indicate that elevator is registered currently,
prepare to add a flag to disable wbt if default elevator is bfq.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/elevator.c | 6 ++----
 block/elevator.h | 4 +++-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/elevator.c b/block/elevator.c
index 20e70fd3f77f..9e12706e8d8c 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -512,7 +512,7 @@ int elv_register_queue(struct request_queue *q, bool uevent)
 		if (uevent)
 			kobject_uevent(&e->kobj, KOBJ_ADD);
 
-		e->registered = 1;
+		set_bit(ELEVATOR_FLAG_REGISTERED, &e->flags);
 	}
 	return error;
 }
@@ -523,11 +523,9 @@ void elv_unregister_queue(struct request_queue *q)
 
 	lockdep_assert_held(&q->sysfs_lock);
 
-	if (e && e->registered) {
+	if (e && test_and_clear_bit(ELEVATOR_FLAG_REGISTERED, &e->flags)) {
 		kobject_uevent(&e->kobj, KOBJ_REMOVE);
 		kobject_del(&e->kobj);
-
-		e->registered = 0;
 	}
 }
 
diff --git a/block/elevator.h b/block/elevator.h
index 3f0593b3bf9d..ed574bf3e629 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -100,10 +100,12 @@ struct elevator_queue
 	void *elevator_data;
 	struct kobject kobj;
 	struct mutex sysfs_lock;
-	unsigned int registered:1;
+	unsigned long flags;
 	DECLARE_HASHTABLE(hash, ELV_HASH_BITS);
 };
 
+#define ELEVATOR_FLAG_REGISTERED 0
+
 /*
  * block elevator interface
  */
-- 
2.31.1

Re: [PATCH v4 5/6] elevator: add new field flags in struct elevator_queue

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v4 6/6] blk-wbt: don't enable throttling if default elevator is bfq

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

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 adding a new flag ELEVATOR_FLAG_DISBALE_WBT, bfq
will set the flag in bfq_init_queue, and following wbt_enable_default()
won't enable wbt while the flag is set.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/bfq-iosched.c |  2 ++
 block/blk-wbt.c     | 11 ++++++++---
 block/elevator.h    |  3 ++-
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 7ea427817f7f..ccfadc6f71bf 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -7045,6 +7045,7 @@ static void bfq_exit_queue(struct elevator_queue *e)
 #endif
 
 	blk_stat_disable_accounting(bfqd->queue);
+	clear_bit(ELEVATOR_FLAG_DISABLE_WBT, &e->flags);
 	wbt_enable_default(bfqd->queue);
 
 	kfree(bfqd);
@@ -7190,6 +7191,7 @@ static int bfq_init_queue(struct request_queue *q, struct elevator_type *e)
 	/* We dispatch from request queue wide instead of hw queue */
 	blk_queue_flag_set(QUEUE_FLAG_SQ_SCHED, q);
 
+	set_bit(ELEVATOR_FLAG_DISABLE_WBT, &eq->flags);
 	wbt_disable_default(q);
 	blk_stat_enable_accounting(q);
 
diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 1852ef223c68..41f5aae659f0 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>
@@ -651,11 +652,15 @@ void wbt_set_write_cache(struct request_queue *q, bool write_cache_on)
  */
 void wbt_enable_default(struct request_queue *q)
 {
-	struct rq_qos *rqos = wbt_rq_qos(q);
+	struct rq_qos *rqos;
+	bool disable_flag = q->elevator &&
+		    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags);
 
 	/* Throttling already enabled? */
+	rqos = wbt_rq_qos(q);
 	if (rqos) {
-		if (RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
+		if (!disable_flag &&
+		    RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
 			RQWB(rqos)->enable_state = WBT_STATE_ON_DEFAULT;
 		return;
 	}
@@ -664,7 +669,7 @@ void wbt_enable_default(struct request_queue *q)
 	if (!blk_queue_registered(q))
 		return;
 
-	if (queue_is_mq(q))
+	if (queue_is_mq(q) && !disable_flag)
 		wbt_init(q);
 }
 EXPORT_SYMBOL_GPL(wbt_enable_default);
diff --git a/block/elevator.h b/block/elevator.h
index ed574bf3e629..75382471222d 100644
--- a/block/elevator.h
+++ b/block/elevator.h
@@ -104,7 +104,8 @@ struct elevator_queue
 	DECLARE_HASHTABLE(hash, ELV_HASH_BITS);
 };
 
-#define ELEVATOR_FLAG_REGISTERED 0
+#define ELEVATOR_FLAG_REGISTERED	0
+#define ELEVATOR_FLAG_DISABLE_WBT	1
 
 /*
  * block elevator interface
-- 
2.31.1

Re: [PATCH v4 6/6] blk-wbt: don't enable throttling if default elevator is bfq

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v4 0/6] blk-wbt: simple improvment to enable wbt correctly

[Yu Kuai]

friendly ping ...

在 2022/09/30 11:19, Yu Kuai 写道:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> changes in v4:
>   - remove patch 3 from v3
>   - add patch 2,3 in v4
> 
> changes in v3:
>   - instead of check elevator name, add a flag in elevator_queue, as
>   suggested by Christoph.
>   - add patch 3 and patch 5 to this patchset.
> 
> changes in v2:
>   - define new api if wbt config is not enabled in patch 1.
> 
> Yu Kuai (6):
>    elevator: remove redundant code in elv_unregister_queue()
>    blk-wbt: remove unnecessary check in wbt_enable_default()
>    blk-wbt: make enable_state more accurate
>    blk-wbt: don't show valid wbt_lat_usec in sysfs while wbt is disabled
>    elevator: add new field flags in struct elevator_queue
>    blk-wbt: don't enable throttling if default elevator is bfq
> 
>   block/bfq-iosched.c |  2 ++
>   block/blk-sysfs.c   |  6 +++++-
>   block/blk-wbt.c     | 26 ++++++++++++++++++++++----
>   block/blk-wbt.h     | 17 ++++++++++++-----
>   block/elevator.c    |  8 ++------
>   block/elevator.h    |  5 ++++-
>   6 files changed, 47 insertions(+), 17 deletions(-)
> 

Re: [PATCH v4 0/6] blk-wbt: simple improvment to enable wbt correctly

[Yu Kuai]

friendly ping ...

在 2022/10/11 16:06, Yu Kuai 写道:
> friendly ping ...
> 
> 在 2022/09/30 11:19, Yu Kuai 写道:
>> From: Yu Kuai <yukuai3@huawei.com>
>>
>> changes in v4:
>>   - remove patch 3 from v3
>>   - add patch 2,3 in v4
>>
>> changes in v3:
>>   - instead of check elevator name, add a flag in elevator_queue, as
>>   suggested by Christoph.
>>   - add patch 3 and patch 5 to this patchset.
>>
>> changes in v2:
>>   - define new api if wbt config is not enabled in patch 1.
>>
>> Yu Kuai (6):
>>    elevator: remove redundant code in elv_unregister_queue()
>>    blk-wbt: remove unnecessary check in wbt_enable_default()
>>    blk-wbt: make enable_state more accurate
>>    blk-wbt: don't show valid wbt_lat_usec in sysfs while wbt is disabled
>>    elevator: add new field flags in struct elevator_queue
>>    blk-wbt: don't enable throttling if default elevator is bfq
>>
>>   block/bfq-iosched.c |  2 ++
>>   block/blk-sysfs.c   |  6 +++++-
>>   block/blk-wbt.c     | 26 ++++++++++++++++++++++----
>>   block/blk-wbt.h     | 17 ++++++++++++-----
>>   block/elevator.c    |  8 ++------
>>   block/elevator.h    |  5 ++++-
>>   6 files changed, 47 insertions(+), 17 deletions(-)
>>
> 
> .
>