[PATCH 1/2] blk-stat: fix QUEUE_FLAG_STATS clear

[PATCH 1/2] blk-stat: fix QUEUE_FLAG_STATS clear

[chengming.zhou]

From: Chengming Zhou <zhouchengming@bytedance.com>

We need to set QUEUE_FLAG_STATS for two cases:
1. blk_stat_enable_accounting()
2. blk_stat_add_callback()

So we should clear it only when ((q->stats->accounting == 0) &&
list_empty(&q->stats->callbacks)).

blk_stat_disable_accounting() only check if q->stats->accounting
is 0 before clear the flag, this patch fix it.

Also add list_empty(&q->stats->callbacks)) check when enable, or
the flag is already set.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-stat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-stat.c b/block/blk-stat.c
index 74a1a8c32d86..bc7e0ed81642 100644
--- a/block/blk-stat.c
+++ b/block/blk-stat.c
@@ -190,7 +190,7 @@ void blk_stat_disable_accounting(struct request_queue *q)
 	unsigned long flags;
 
 	spin_lock_irqsave(&q->stats->lock, flags);
-	if (!--q->stats->accounting)
+	if (!--q->stats->accounting && list_empty(&q->stats->callbacks))
 		blk_queue_flag_clear(QUEUE_FLAG_STATS, q);
 	spin_unlock_irqrestore(&q->stats->lock, flags);
 }
@@ -201,7 +201,7 @@ void blk_stat_enable_accounting(struct request_queue *q)
 	unsigned long flags;
 
 	spin_lock_irqsave(&q->stats->lock, flags);
-	if (!q->stats->accounting++)
+	if (!q->stats->accounting++ && list_empty(&q->stats->callbacks))
 		blk_queue_flag_set(QUEUE_FLAG_STATS, q);
 	spin_unlock_irqrestore(&q->stats->lock, flags);
 }
-- 
2.39.2

[PATCH 2/2] blk-throttle: only enable blk-stat when BLK_DEV_THROTTLING_LOW

[chengming.zhou]

From: Chengming Zhou <zhouchengming@bytedance.com>

blk_throtl_register() will unconditionally enable blk-stat for gendisk
when register, even when we have no BLK_DEV_THROTTLING_LOW config.

Since the kernel always has only BLK_DEV_THROTTLING config and the
BLK_DEV_THROTTLING_LOW config is still in EXPERIMENTAL state, we can
just skip blk-stat when !BLK_DEV_THROTTLING_LOW.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
 block/blk-throttle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index 47e9d8be68f3..3c07c9cfa70a 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -2439,11 +2439,12 @@ void blk_throtl_register(struct gendisk *disk)
 #ifndef CONFIG_BLK_DEV_THROTTLING_LOW
 	/* if no low limit, use previous default */
 	td->throtl_slice = DFL_THROTL_SLICE_HD;
-#endif
 
+#else
 	td->track_bio_latency = !queue_is_mq(q);
 	if (!td->track_bio_latency)
 		blk_stat_enable_accounting(q);
+#endif
 }
 
 #ifdef CONFIG_BLK_DEV_THROTTLING_LOW
-- 
2.39.2

Re: [PATCH 2/2] blk-throttle: only enable blk-stat when BLK_DEV_THROTTLING_LOW

[Tejun Heo]

On Thu, Apr 13, 2023 at 12:07:54AM +0800, chengming.zhou@linux.dev wrote:
> From: Chengming Zhou <zhouchengming@bytedance.com>
> 
> blk_throtl_register() will unconditionally enable blk-stat for gendisk
> when register, even when we have no BLK_DEV_THROTTLING_LOW config.
> 
> Since the kernel always has only BLK_DEV_THROTTLING config and the
> BLK_DEV_THROTTLING_LOW config is still in EXPERIMENTAL state, we can
> just skip blk-stat when !BLK_DEV_THROTTLING_LOW.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 1/2] blk-stat: fix QUEUE_FLAG_STATS clear

[Tejun Heo]

On Thu, Apr 13, 2023 at 12:07:53AM +0800, chengming.zhou@linux.dev wrote:
> From: Chengming Zhou <zhouchengming@bytedance.com>
> 
> We need to set QUEUE_FLAG_STATS for two cases:
> 1. blk_stat_enable_accounting()
> 2. blk_stat_add_callback()
> 
> So we should clear it only when ((q->stats->accounting == 0) &&
> list_empty(&q->stats->callbacks)).
> 
> blk_stat_disable_accounting() only check if q->stats->accounting
> is 0 before clear the flag, this patch fix it.
> 
> Also add list_empty(&q->stats->callbacks)) check when enable, or
> the flag is already set.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>

Acked-by: Tejun Heo <tj@kernel.org>

It'd be useful to explicitly illustrate the buggy behavior in the
description (e.g. if you do X, Y and then Z, then X incorrectly loses
accounting). Can you also please add the appropriate stable cc?

Thanks.

-- 
tejun

Re: [PATCH 1/2] blk-stat: fix QUEUE_FLAG_STATS clear

[Chengming Zhou]

On 2023/4/13 01:12, Tejun Heo wrote:
> On Thu, Apr 13, 2023 at 12:07:53AM +0800, chengming.zhou@linux.dev wrote:
>> From: Chengming Zhou <zhouchengming@bytedance.com>
>>
>> We need to set QUEUE_FLAG_STATS for two cases:
>> 1. blk_stat_enable_accounting()
>> 2. blk_stat_add_callback()
>>
>> So we should clear it only when ((q->stats->accounting == 0) &&
>> list_empty(&q->stats->callbacks)).
>>
>> blk_stat_disable_accounting() only check if q->stats->accounting
>> is 0 before clear the flag, this patch fix it.
>>
>> Also add list_empty(&q->stats->callbacks)) check when enable, or
>> the flag is already set.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> It'd be useful to explicitly illustrate the buggy behavior in the
> description (e.g. if you do X, Y and then Z, then X incorrectly loses

Yes, I will add below buggy behavior in the next version:

This bug can be reproduced as below on kernel without BLK_DEV_THROTTLING
(since it will unconditionally enable accounting, see the second patch).

# cat /sys/block/sr0/queue/scheduler
none mq-deadline [bfq]

# cat /sys/kernel/debug/block/sr0/state
SAME_COMP|IO_STAT|INIT_DONE|STATS|REGISTERED|NOWAIT|30

# echo none > /sys/block/sr0/queue/scheduler

# cat /sys/kernel/debug/block/sr0/state
SAME_COMP|IO_STAT|INIT_DONE|REGISTERED|NOWAIT

# cat /sys/block/sr0/queue/wbt_lat_usec
75000

We can see that after changing elevator from "bfq" to "none", "STATS" flag
is lost even though WBT callback still need it.


> accounting). Can you also please add the appropriate stable cc?

Ok, will do.

Thanks.

> 
> Thanks.
>