[PATCH -next v2 0/6] blk-wbt: minor fix and cleanup

[PATCH -next v2 0/6] blk-wbt: minor fix and cleanup

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Changes in v2:
 - make the code more readable for patch 1
 - add a new attr_group that is only visible for rq based device
 - explain in detail for patch 4
 - add review tag for patch 2,3,5

Yu Kuai (6):
  blk-wbt: fix that wbt can't be disabled by default
  blk-wbt: don't create wbt sysfs entry if CONFIG_BLK_WBT is disabled
  blk-wbt: remove dead code to handle wbt enable/disable with io
    inflight
  blk-wbt: cleanup rwb_enabled() and wbt_disabled()
  blk-iocost: move wbt_enable/disable_default() out of spinlock
  blk-sysfs: add a new attr_group for blk_mq

 block/blk-iocost.c |   7 +-
 block/blk-sysfs.c  | 181 ++++++++++++++++++++++++++-------------------
 block/blk-wbt.c    |  33 +++------
 block/blk-wbt.h    |  19 -----
 4 files changed, 117 insertions(+), 123 deletions(-)

-- 
2.39.2

[PATCH -next v2 1/6] blk-wbt: fix that wbt can't be disabled by default

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

commit b11d31ae01e6 ("blk-wbt: remove unnecessary check in
wbt_enable_default()") removes the checking of CONFIG_BLK_WBT_MQ by
mistake, which is used to control enable or disable wbt by default.

Fix the problem by adding back the checking. This patch also do a litter
cleanup to make related code more readable.

Fixes: b11d31ae01e6 ("blk-wbt: remove unnecessary check in wbt_enable_default()")
Reported-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Link: https://lore.kernel.org/lkml/CAKXUXMzfKq_J9nKHGyr5P5rvUETY4B-fxoQD4sO+NYjFOfVtZA@mail.gmail.com/t/
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-wbt.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index e49a48684532..9ec2a2f1eda3 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -730,14 +730,16 @@ void wbt_enable_default(struct gendisk *disk)
 {
 	struct request_queue *q = disk->queue;
 	struct rq_qos *rqos;
-	bool disable_flag = q->elevator &&
-		    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags);
+	bool enable = IS_ENABLED(CONFIG_BLK_WBT_MQ);
+
+	if (q->elevator &&
+	    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags))
+		enable = false;
 
 	/* Throttling already enabled? */
 	rqos = wbt_rq_qos(q);
 	if (rqos) {
-		if (!disable_flag &&
-		    RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
+		if (enable && RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
 			RQWB(rqos)->enable_state = WBT_STATE_ON_DEFAULT;
 		return;
 	}
@@ -746,7 +748,7 @@ void wbt_enable_default(struct gendisk *disk)
 	if (!blk_queue_registered(q))
 		return;
 
-	if (queue_is_mq(q) && !disable_flag)
+	if (queue_is_mq(q) && enable)
 		wbt_init(disk);
 }
 EXPORT_SYMBOL_GPL(wbt_enable_default);
-- 
2.39.2

Re: [PATCH -next v2 1/6] blk-wbt: fix that wbt can't be disabled by default

[Chengming Zhou]

On 2023/5/12 17:35, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> commit b11d31ae01e6 ("blk-wbt: remove unnecessary check in
> wbt_enable_default()") removes the checking of CONFIG_BLK_WBT_MQ by
> mistake, which is used to control enable or disable wbt by default.
> 
> Fix the problem by adding back the checking. This patch also do a litter
> cleanup to make related code more readable.
> 
> Fixes: b11d31ae01e6 ("blk-wbt: remove unnecessary check in wbt_enable_default()")
> Reported-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Link: https://lore.kernel.org/lkml/CAKXUXMzfKq_J9nKHGyr5P5rvUETY4B-fxoQD4sO+NYjFOfVtZA@mail.gmail.com/t/
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  block/blk-wbt.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index e49a48684532..9ec2a2f1eda3 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -730,14 +730,16 @@ void wbt_enable_default(struct gendisk *disk)
>  {
>  	struct request_queue *q = disk->queue;
>  	struct rq_qos *rqos;
> -	bool disable_flag = q->elevator &&
> -		    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags);
> +	bool enable = IS_ENABLED(CONFIG_BLK_WBT_MQ);
> +
> +	if (q->elevator &&
> +	    test_bit(ELEVATOR_FLAG_DISABLE_WBT, &q->elevator->flags))
> +		enable = false;

Why not just early return, so "enable" is not needed at all?

I have another question that CONFIG_BLK_WBT_MQ is not much flexible, can we
just get rid of it? (I'm not sure when to disable it in the config)

Thanks.

>  
>  	/* Throttling already enabled? */
>  	rqos = wbt_rq_qos(q);
>  	if (rqos) {
> -		if (!disable_flag &&
> -		    RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
> +		if (enable && RQWB(rqos)->enable_state == WBT_STATE_OFF_DEFAULT)
>  			RQWB(rqos)->enable_state = WBT_STATE_ON_DEFAULT;
>  		return;
>  	}
> @@ -746,7 +748,7 @@ void wbt_enable_default(struct gendisk *disk)
>  	if (!blk_queue_registered(q))
>  		return;
>  
> -	if (queue_is_mq(q) && !disable_flag)
> +	if (queue_is_mq(q) && enable)
>  		wbt_init(disk);
>  }
>  EXPORT_SYMBOL_GPL(wbt_enable_default);

Re: [PATCH -next v2 1/6] blk-wbt: fix that wbt can't be disabled by default

[Christoph Hellwig]

Looks good:

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

[PATCH -next v2 2/6] blk-wbt: don't create wbt sysfs entry if CONFIG_BLK_WBT is disabled

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

sysfs entry /sys/block/[device]/queue/wbt_lat_usec will be created even
if CONFIG_BLK_WBT is disabled, while read and write will always fail.
It doesn't make sense to create a sysfs entry that can't be accessed,
so don't create such entry.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-sysfs.c | 143 ++++++++++++++++++++++++----------------------
 block/blk-wbt.h   |  19 ------
 2 files changed, 74 insertions(+), 88 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index a64208583853..6c1c4ba66bc0 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -47,19 +47,6 @@ queue_var_store(unsigned long *var, const char *page, size_t count)
 	return count;
 }
 
-static ssize_t queue_var_store64(s64 *var, const char *page)
-{
-	int err;
-	s64 v;
-
-	err = kstrtos64(page, 10, &v);
-	if (err < 0)
-		return err;
-
-	*var = v;
-	return 0;
-}
-
 static ssize_t queue_requests_show(struct request_queue *q, char *page)
 {
 	return queue_var_show(q->nr_requests, page);
@@ -451,61 +438,6 @@ static ssize_t queue_io_timeout_store(struct request_queue *q, const char *page,
 	return count;
 }
 
-static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
-{
-	if (!wbt_rq_qos(q))
-		return -EINVAL;
-
-	if (wbt_disabled(q))
-		return sprintf(page, "0\n");
-
-	return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000));
-}
-
-static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page,
-				  size_t count)
-{
-	struct rq_qos *rqos;
-	ssize_t ret;
-	s64 val;
-
-	ret = queue_var_store64(&val, page);
-	if (ret < 0)
-		return ret;
-	if (val < -1)
-		return -EINVAL;
-
-	rqos = wbt_rq_qos(q);
-	if (!rqos) {
-		ret = wbt_init(q->disk);
-		if (ret)
-			return ret;
-	}
-
-	if (val == -1)
-		val = wbt_default_latency_nsec(q);
-	else if (val >= 0)
-		val *= 1000ULL;
-
-	if (wbt_get_min_lat(q) == val)
-		return count;
-
-	/*
-	 * Ensure that the queue is idled, in case the latency update
-	 * ends up either enabling or disabling wbt completely. We can't
-	 * have IO inflight if that happens.
-	 */
-	blk_mq_freeze_queue(q);
-	blk_mq_quiesce_queue(q);
-
-	wbt_set_min_lat(q, val);
-
-	blk_mq_unquiesce_queue(q);
-	blk_mq_unfreeze_queue(q);
-
-	return count;
-}
-
 static ssize_t queue_wc_show(struct request_queue *q, char *page)
 {
 	if (test_bit(QUEUE_FLAG_WC, &q->queue_flags))
@@ -598,7 +530,6 @@ QUEUE_RW_ENTRY(queue_wc, "write_cache");
 QUEUE_RO_ENTRY(queue_fua, "fua");
 QUEUE_RO_ENTRY(queue_dax, "dax");
 QUEUE_RW_ENTRY(queue_io_timeout, "io_timeout");
-QUEUE_RW_ENTRY(queue_wb_lat, "wbt_lat_usec");
 QUEUE_RO_ENTRY(queue_virt_boundary_mask, "virt_boundary_mask");
 QUEUE_RO_ENTRY(queue_dma_alignment, "dma_alignment");
 
@@ -617,6 +548,78 @@ QUEUE_RW_ENTRY(queue_iostats, "iostats");
 QUEUE_RW_ENTRY(queue_random, "add_random");
 QUEUE_RW_ENTRY(queue_stable_writes, "stable_writes");
 
+#ifdef CONFIG_BLK_WBT
+static ssize_t queue_var_store64(s64 *var, const char *page)
+{
+	int err;
+	s64 v;
+
+	err = kstrtos64(page, 10, &v);
+	if (err < 0)
+		return err;
+
+	*var = v;
+	return 0;
+}
+
+static ssize_t queue_wb_lat_show(struct request_queue *q, char *page)
+{
+	if (!wbt_rq_qos(q))
+		return -EINVAL;
+
+	if (wbt_disabled(q))
+		return sprintf(page, "0\n");
+
+	return sprintf(page, "%llu\n", div_u64(wbt_get_min_lat(q), 1000));
+}
+
+static ssize_t queue_wb_lat_store(struct request_queue *q, const char *page,
+				  size_t count)
+{
+	struct rq_qos *rqos;
+	ssize_t ret;
+	s64 val;
+
+	ret = queue_var_store64(&val, page);
+	if (ret < 0)
+		return ret;
+	if (val < -1)
+		return -EINVAL;
+
+	rqos = wbt_rq_qos(q);
+	if (!rqos) {
+		ret = wbt_init(q->disk);
+		if (ret)
+			return ret;
+	}
+
+	if (val == -1)
+		val = wbt_default_latency_nsec(q);
+	else if (val >= 0)
+		val *= 1000ULL;
+
+	if (wbt_get_min_lat(q) == val)
+		return count;
+
+	/*
+	 * Ensure that the queue is idled, in case the latency update
+	 * ends up either enabling or disabling wbt completely. We can't
+	 * have IO inflight if that happens.
+	 */
+	blk_mq_freeze_queue(q);
+	blk_mq_quiesce_queue(q);
+
+	wbt_set_min_lat(q, val);
+
+	blk_mq_unquiesce_queue(q);
+	blk_mq_unfreeze_queue(q);
+
+	return count;
+}
+
+QUEUE_RW_ENTRY(queue_wb_lat, "wbt_lat_usec");
+#endif
+
 static struct attribute *queue_attrs[] = {
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
@@ -655,7 +658,9 @@ static struct attribute *queue_attrs[] = {
 	&queue_wc_entry.attr,
 	&queue_fua_entry.attr,
 	&queue_dax_entry.attr,
+#ifdef CONFIG_BLK_WBT
 	&queue_wb_lat_entry.attr,
+#endif
 	&queue_poll_delay_entry.attr,
 	&queue_io_timeout_entry.attr,
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
diff --git a/block/blk-wbt.h b/block/blk-wbt.h
index ba6cca5849a6..8a029e138f7a 100644
--- a/block/blk-wbt.h
+++ b/block/blk-wbt.h
@@ -18,10 +18,6 @@ u64 wbt_default_latency_nsec(struct request_queue *);
 
 #else
 
-static inline int wbt_init(struct gendisk *disk)
-{
-	return -EINVAL;
-}
 static inline void wbt_disable_default(struct gendisk *disk)
 {
 }
@@ -31,21 +27,6 @@ static inline void wbt_enable_default(struct gendisk *disk)
 static inline void wbt_set_write_cache(struct request_queue *q, bool wc)
 {
 }
-static inline u64 wbt_get_min_lat(struct request_queue *q)
-{
-	return 0;
-}
-static inline void wbt_set_min_lat(struct request_queue *q, u64 val)
-{
-}
-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.39.2

Re: [PATCH -next v2 2/6] blk-wbt: don't create wbt sysfs entry if CONFIG_BLK_WBT is disabled

[Christoph Hellwig]

Looks good:

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

[PATCH -next v2 3/6] blk-wbt: remove dead code to handle wbt enable/disable with io inflight

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

enable or disable wbt is always called with queue freezed, so that wbt
can never be enabled or disabled while io is still inflight, and this
behaviour should always hold to avoid io hang(There have been reported
several times).

Therefor, the code to handle wbt enable/diskble with io inflight is not
and never will be used, hence remove such dead code.

Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-wbt.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index 9ec2a2f1eda3..cb464c572840 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -200,15 +200,6 @@ static void wbt_rqw_done(struct rq_wb *rwb, struct rq_wait *rqw,
 
 	inflight = atomic_dec_return(&rqw->inflight);
 
-	/*
-	 * wbt got disabled with IO in flight. Wake up any potential
-	 * waiters, we don't have to do more than that.
-	 */
-	if (unlikely(!rwb_enabled(rwb))) {
-		rwb_wake_all(rwb);
-		return;
-	}
-
 	/*
 	 * For discards, our limit is always the background. For writes, if
 	 * the device does write back caching, drop further down before we
@@ -545,13 +536,6 @@ static inline unsigned int get_limit(struct rq_wb *rwb, blk_opf_t opf)
 {
 	unsigned int limit;
 
-	/*
-	 * If we got disabled, just return UINT_MAX. This ensures that
-	 * we'll properly inc a new IO, and dec+wakeup at the end.
-	 */
-	if (!rwb_enabled(rwb))
-		return UINT_MAX;
-
 	if ((opf & REQ_OP_MASK) == REQ_OP_DISCARD)
 		return rwb->wb_background;
 
-- 
2.39.2

[PATCH -next v2 4/6] blk-wbt: cleanup rwb_enabled() and wbt_disabled()

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

'wb_normal' will set to 0 if 'min_lat_nsec' is 0, and 'min_lat_nsec' can
only be set to 0 through sysfs configuration where 'WBT_STATE_OFF_MANUAL'
is set together, in the meantime, they can only be cleared together
through sysfs afterwards. Hence 'wb_normal != 0' is the same as
'rwb->enable_state != WBT_STATE_OFF_MANUAL'.

The code is redundan, hence replace the checking of 'wb_normal' to
'enable_state' in rwb_enabled() and reuse rwb_enabled() for
wbt_disabled().

Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 block/blk-wbt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/block/blk-wbt.c b/block/blk-wbt.c
index cb464c572840..06d400fa050d 100644
--- a/block/blk-wbt.c
+++ b/block/blk-wbt.c
@@ -146,7 +146,7 @@ enum {
 static inline bool rwb_enabled(struct rq_wb *rwb)
 {
 	return rwb && rwb->enable_state != WBT_STATE_OFF_DEFAULT &&
-		      rwb->wb_normal != 0;
+		      rwb->enable_state != WBT_STATE_OFF_MANUAL;
 }
 
 static void wb_timestamp(struct rq_wb *rwb, unsigned long *var)
@@ -494,8 +494,7 @@ 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;
+	return !rqos || !rwb_enabled(RQWB(rqos));
 }
 
 u64 wbt_get_min_lat(struct request_queue *q)
-- 
2.39.2

Re: [PATCH -next v2 4/6] blk-wbt: cleanup rwb_enabled() and wbt_disabled()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>

[PATCH -next v2 5/6] blk-iocost: move wbt_enable/disable_default() out of spinlock

[Yu Kuai]

From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

There are following smatch warning:

block/blk-wbt.c:843 wbt_init() warn: sleeping in atomic context
ioc_qos_write() <- disables preempt
-> wbt_enable_default()
   -> wbt_init()

wbt_init() will be called from wbt_enable_default() if wbt is not
initialized, currently this is only possible in blk_register_queue(), hence
wbt_init() will never be called from iocost and this warning is false
positive.

However, we might support rq_qos destruction dynamically in the future,
and it's better to prevent that, hence move wbt_enable_default() outside
'ioc->lock'. This is safe because queue is still freezed.

Reported-by: Dan Carpenter <error27-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Link: https://lore.kernel.org/lkml/Y+Ja5SRs886CEz7a@kadam/
Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 block/blk-iocost.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 285ced3467ab..eb57e7e4f2db 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3300,11 +3300,9 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 		blk_stat_enable_accounting(disk->queue);
 		blk_queue_flag_set(QUEUE_FLAG_RQ_ALLOC_TIME, disk->queue);
 		ioc->enabled = true;
-		wbt_disable_default(disk);
 	} else {
 		blk_queue_flag_clear(QUEUE_FLAG_RQ_ALLOC_TIME, disk->queue);
 		ioc->enabled = false;
-		wbt_enable_default(disk);
 	}
 
 	if (user) {
@@ -3317,6 +3315,11 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
 	ioc_refresh_params(ioc, true);
 	spin_unlock_irq(&ioc->lock);
 
+	if (enable)
+		wbt_disable_default(disk);
+	else
+		wbt_enable_default(disk);
+
 	blk_mq_unquiesce_queue(disk->queue);
 	blk_mq_unfreeze_queue(disk->queue);
 
-- 
2.39.2

Re: [PATCH -next v2 5/6] blk-iocost: move wbt_enable/disable_default() out of spinlock

[Christoph Hellwig]

Looks good:

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

[PATCH -next v2 6/6] blk-sysfs: add a new attr_group for blk_mq

[Yu Kuai]

From: Yu Kuai <yukuai3@huawei.com>

Currently wbt sysfs entry is created for bio based device, and wbt can
be enabled for such device through sysfs while it doesn't make sense
because wbt can only work for rq based device. In the meantime, there
are other similar sysfs entries.

Fix this by adding a new attr_group for blk_mq, and sysfs entries will
only be created when the device is rq based.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
---
 block/blk-sysfs.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 6c1c4ba66bc0..afc797fb0dfc 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -621,7 +621,6 @@ QUEUE_RW_ENTRY(queue_wb_lat, "wbt_lat_usec");
 #endif
 
 static struct attribute *queue_attrs[] = {
-	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
 	&queue_max_hw_sectors_entry.attr,
 	&queue_max_sectors_entry.attr,
@@ -629,7 +628,6 @@ static struct attribute *queue_attrs[] = {
 	&queue_max_discard_segments_entry.attr,
 	&queue_max_integrity_segments_entry.attr,
 	&queue_max_segment_size_entry.attr,
-	&elv_iosched_entry.attr,
 	&queue_hw_sector_size_entry.attr,
 	&queue_logical_block_size_entry.attr,
 	&queue_physical_block_size_entry.attr,
@@ -650,7 +648,6 @@ static struct attribute *queue_attrs[] = {
 	&queue_max_open_zones_entry.attr,
 	&queue_max_active_zones_entry.attr,
 	&queue_nomerges_entry.attr,
-	&queue_rq_affinity_entry.attr,
 	&queue_iostats_entry.attr,
 	&queue_stable_writes_entry.attr,
 	&queue_random_entry.attr,
@@ -658,11 +655,7 @@ static struct attribute *queue_attrs[] = {
 	&queue_wc_entry.attr,
 	&queue_fua_entry.attr,
 	&queue_dax_entry.attr,
-#ifdef CONFIG_BLK_WBT
-	&queue_wb_lat_entry.attr,
-#endif
 	&queue_poll_delay_entry.attr,
-	&queue_io_timeout_entry.attr,
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
 	&blk_throtl_sample_time_entry.attr,
 #endif
@@ -671,16 +664,23 @@ static struct attribute *queue_attrs[] = {
 	NULL,
 };
 
+static struct attribute *blk_mq_queue_attrs[] = {
+	&queue_requests_entry.attr,
+	&elv_iosched_entry.attr,
+	&queue_rq_affinity_entry.attr,
+	&queue_io_timeout_entry.attr,
+#ifdef CONFIG_BLK_WBT
+	&queue_wb_lat_entry.attr,
+#endif
+	NULL,
+};
+
 static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr,
 				int n)
 {
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
 	struct request_queue *q = disk->queue;
 
-	if (attr == &queue_io_timeout_entry.attr &&
-		(!q->mq_ops || !q->mq_ops->timeout))
-			return 0;
-
 	if ((attr == &queue_max_open_zones_entry.attr ||
 	     attr == &queue_max_active_zones_entry.attr) &&
 	    !blk_queue_is_zoned(q))
@@ -689,11 +689,30 @@ static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr,
 	return attr->mode;
 }
 
+static umode_t blk_mq_queue_attr_visible(struct kobject *kobj,
+					 struct attribute *attr, int n)
+{
+	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
+	struct request_queue *q = disk->queue;
+
+	if (!queue_is_mq(q))
+		return 0;
+
+	if (attr == &queue_io_timeout_entry.attr && !q->mq_ops->timeout)
+		return 0;
+
+	return attr->mode;
+}
+
 static struct attribute_group queue_attr_group = {
 	.attrs = queue_attrs,
 	.is_visible = queue_attr_visible,
 };
 
+static struct attribute_group blk_mq_queue_attr_group = {
+	.attrs = blk_mq_queue_attrs,
+	.is_visible = blk_mq_queue_attr_visible,
+};
 
 #define to_queue(atr) container_of((atr), struct queue_sysfs_entry, attr)
 
@@ -738,6 +757,7 @@ static const struct sysfs_ops queue_sysfs_ops = {
 
 static const struct attribute_group *blk_queue_attr_groups[] = {
 	&queue_attr_group,
+	&blk_mq_queue_attr_group,
 	NULL
 };
 
-- 
2.39.2

Re: [PATCH -next v2 6/6] blk-sysfs: add a new attr_group for blk_mq

[Yu Kuai]

Hi, Christoph

ÔÚ 2023/05/12 17:35, Yu Kuai дµÀ:
> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Currently wbt sysfs entry is created for bio based device, and wbt can
> be enabled for such device through sysfs while it doesn't make sense
> because wbt can only work for rq based device. In the meantime, there
> are other similar sysfs entries.
> 
> Fix this by adding a new attr_group for blk_mq, and sysfs entries will
> only be created when the device is rq based.
> 
> Suggested-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Signed-off-by: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Any comments about this patch?

Thanks,
Kuai
> ---
>   block/blk-sysfs.c | 42 +++++++++++++++++++++++++++++++-----------
>   1 file changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 6c1c4ba66bc0..afc797fb0dfc 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -621,7 +621,6 @@ QUEUE_RW_ENTRY(queue_wb_lat, "wbt_lat_usec");
>   #endif
>   
>   static struct attribute *queue_attrs[] = {
> -	&queue_requests_entry.attr,
>   	&queue_ra_entry.attr,
>   	&queue_max_hw_sectors_entry.attr,
>   	&queue_max_sectors_entry.attr,
> @@ -629,7 +628,6 @@ static struct attribute *queue_attrs[] = {
>   	&queue_max_discard_segments_entry.attr,
>   	&queue_max_integrity_segments_entry.attr,
>   	&queue_max_segment_size_entry.attr,
> -	&elv_iosched_entry.attr,
>   	&queue_hw_sector_size_entry.attr,
>   	&queue_logical_block_size_entry.attr,
>   	&queue_physical_block_size_entry.attr,
> @@ -650,7 +648,6 @@ static struct attribute *queue_attrs[] = {
>   	&queue_max_open_zones_entry.attr,
>   	&queue_max_active_zones_entry.attr,
>   	&queue_nomerges_entry.attr,
> -	&queue_rq_affinity_entry.attr,
>   	&queue_iostats_entry.attr,
>   	&queue_stable_writes_entry.attr,
>   	&queue_random_entry.attr,
> @@ -658,11 +655,7 @@ static struct attribute *queue_attrs[] = {
>   	&queue_wc_entry.attr,
>   	&queue_fua_entry.attr,
>   	&queue_dax_entry.attr,
> -#ifdef CONFIG_BLK_WBT
> -	&queue_wb_lat_entry.attr,
> -#endif
>   	&queue_poll_delay_entry.attr,
> -	&queue_io_timeout_entry.attr,
>   #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
>   	&blk_throtl_sample_time_entry.attr,
>   #endif
> @@ -671,16 +664,23 @@ static struct attribute *queue_attrs[] = {
>   	NULL,
>   };
>   
> +static struct attribute *blk_mq_queue_attrs[] = {
> +	&queue_requests_entry.attr,
> +	&elv_iosched_entry.attr,
> +	&queue_rq_affinity_entry.attr,
> +	&queue_io_timeout_entry.attr,
> +#ifdef CONFIG_BLK_WBT
> +	&queue_wb_lat_entry.attr,
> +#endif
> +	NULL,
> +};
> +
>   static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr,
>   				int n)
>   {
>   	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
>   	struct request_queue *q = disk->queue;
>   
> -	if (attr == &queue_io_timeout_entry.attr &&
> -		(!q->mq_ops || !q->mq_ops->timeout))
> -			return 0;
> -
>   	if ((attr == &queue_max_open_zones_entry.attr ||
>   	     attr == &queue_max_active_zones_entry.attr) &&
>   	    !blk_queue_is_zoned(q))
> @@ -689,11 +689,30 @@ static umode_t queue_attr_visible(struct kobject *kobj, struct attribute *attr,
>   	return attr->mode;
>   }
>   
> +static umode_t blk_mq_queue_attr_visible(struct kobject *kobj,
> +					 struct attribute *attr, int n)
> +{
> +	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
> +	struct request_queue *q = disk->queue;
> +
> +	if (!queue_is_mq(q))
> +		return 0;
> +
> +	if (attr == &queue_io_timeout_entry.attr && !q->mq_ops->timeout)
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
>   static struct attribute_group queue_attr_group = {
>   	.attrs = queue_attrs,
>   	.is_visible = queue_attr_visible,
>   };
>   
> +static struct attribute_group blk_mq_queue_attr_group = {
> +	.attrs = blk_mq_queue_attrs,
> +	.is_visible = blk_mq_queue_attr_visible,
> +};
>   
>   #define to_queue(atr) container_of((atr), struct queue_sysfs_entry, attr)
>   
> @@ -738,6 +757,7 @@ static const struct sysfs_ops queue_sysfs_ops = {
>   
>   static const struct attribute_group *blk_queue_attr_groups[] = {
>   	&queue_attr_group,
> +	&blk_mq_queue_attr_group,
>   	NULL
>   };
>   
> 

Re: [PATCH -next v2 6/6] blk-sysfs: add a new attr_group for blk_mq

[Christoph Hellwig]

This patch looks good to me:

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

Re: [PATCH -next v2 0/6] blk-wbt: minor fix and cleanup

[Jens Axboe]

On 5/12/23 3:35 AM, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@huawei.com>
> 
> Changes in v2:
>  - make the code more readable for patch 1
>  - add a new attr_group that is only visible for rq based device
>  - explain in detail for patch 4
>  - add review tag for patch 2,3,5
> 
> Yu Kuai (6):
>   blk-wbt: fix that wbt can't be disabled by default
>   blk-wbt: don't create wbt sysfs entry if CONFIG_BLK_WBT is disabled
>   blk-wbt: remove dead code to handle wbt enable/disable with io
>     inflight
>   blk-wbt: cleanup rwb_enabled() and wbt_disabled()
>   blk-iocost: move wbt_enable/disable_default() out of spinlock
>   blk-sysfs: add a new attr_group for blk_mq
> 
>  block/blk-iocost.c |   7 +-
>  block/blk-sysfs.c  | 181 ++++++++++++++++++++++++++-------------------
>  block/blk-wbt.c    |  33 +++------
>  block/blk-wbt.h    |  19 -----
>  4 files changed, 117 insertions(+), 123 deletions(-)

We need a 6.4 version of the fix to get rid of the regression. If you
want to do cleanups on top of that, then that's fine and that can go into
6.5. But don't mix them up.

-- 
Jens Axboe

Re: [PATCH -next v2 0/6] blk-wbt: minor fix and cleanup

[Linux regression tracking (Thorsten Leemhuis)]

On 12.05.23 16:58, Jens Axboe wrote:
> On 5/12/23 3:35 AM, Yu Kuai wrote:
>> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>
>> Changes in v2:
>>  - make the code more readable for patch 1
>>  - add a new attr_group that is only visible for rq based device
>>  - explain in detail for patch 4
>>  - add review tag for patch 2,3,5
>>
>> Yu Kuai (6):
>>   blk-wbt: fix that wbt can't be disabled by default
>>   blk-wbt: don't create wbt sysfs entry if CONFIG_BLK_WBT is disabled
>>   blk-wbt: remove dead code to handle wbt enable/disable with io
>>     inflight
>>   blk-wbt: cleanup rwb_enabled() and wbt_disabled()
>>   blk-iocost: move wbt_enable/disable_default() out of spinlock
>>   blk-sysfs: add a new attr_group for blk_mq
>>
>>  block/blk-iocost.c |   7 +-
>>  block/blk-sysfs.c  | 181 ++++++++++++++++++++++++++-------------------
>>  block/blk-wbt.c    |  33 +++------
>>  block/blk-wbt.h    |  19 -----
>>  4 files changed, 117 insertions(+), 123 deletions(-)
> 
> We need a 6.4 version of the fix to get rid of the regression. If you
> want to do cleanups on top of that, then that's fine and that can go into
> 6.5. But don't mix them up.

Hmm, it seems nothing has happened here since ten days to fix this
regression that likely is still present in 6.3. Yu Kuai, did it fall
through the cracks, or is what Jens asked for more complicated than it
sounds?

Or was progress made and I just missed it?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

Re: [PATCH -next v2 0/6] blk-wbt: minor fix and cleanup

[Yu Kuai]

Hi,

在 2023/05/22 20:00, Linux regression tracking (Thorsten Leemhuis) 写道:
> On 12.05.23 16:58, Jens Axboe wrote:
>> On 5/12/23 3:35 AM, Yu Kuai wrote:
>>> From: Yu Kuai <yukuai3-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>
>>> Changes in v2:
>>>   - make the code more readable for patch 1
>>>   - add a new attr_group that is only visible for rq based device
>>>   - explain in detail for patch 4
>>>   - add review tag for patch 2,3,5
>>>
>>> Yu Kuai (6):
>>>    blk-wbt: fix that wbt can't be disabled by default
>>>    blk-wbt: don't create wbt sysfs entry if CONFIG_BLK_WBT is disabled
>>>    blk-wbt: remove dead code to handle wbt enable/disable with io
>>>      inflight
>>>    blk-wbt: cleanup rwb_enabled() and wbt_disabled()
>>>    blk-iocost: move wbt_enable/disable_default() out of spinlock
>>>    blk-sysfs: add a new attr_group for blk_mq
>>>
>>>   block/blk-iocost.c |   7 +-
>>>   block/blk-sysfs.c  | 181 ++++++++++++++++++++++++++-------------------
>>>   block/blk-wbt.c    |  33 +++------
>>>   block/blk-wbt.h    |  19 -----
>>>   4 files changed, 117 insertions(+), 123 deletions(-)
>>
>> We need a 6.4 version of the fix to get rid of the regression. If you
>> want to do cleanups on top of that, then that's fine and that can go into
>> 6.5. But don't mix them up.
> 
> Hmm, it seems nothing has happened here since ten days to fix this
> regression that likely is still present in 6.3. Yu Kuai, did it fall
> through the cracks, or is what Jens asked for more complicated than it
> sounds?

Sorry for the delay, I was waiting for the last patch to be reviewed.
But the regression is only related to the first patch, I'll send it
seperately.

Thanks,
Kuai
> 
> Or was progress made and I just missed it?
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
> 
> #regzbot poke
> 
> .
>