2023-02-08 03:24:26

by Yu Kuai

Subject: [PATCH -next] blk-iocost: fix sleeping in atomic warning for wbt_enable_default()

From: Yu Kuai <[email protected]>

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 wbt is initialized 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 <[email protected]>
Link: https://lore.kernel.org/lkml/Y+Ja5SRs886CEz7a@kadam/
Signed-off-by: Yu Kuai <[email protected]>
---
block/blk-iocost.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index 7a2dc9dc8e3b..03bfe1dda07c 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3279,11 +3279,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) {
@@ -3296,6 +3294,10 @@ 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.31.1

2023-02-08 19:06:09

by Tejun Heo

Subject: Re: [PATCH -next] blk-iocost: fix sleeping in atomic warning for wbt_enable_default()

On Wed, Feb 08, 2023 at 11:48:03AM +0800, Yu Kuai wrote:
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 7a2dc9dc8e3b..03bfe1dda07c 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -3279,11 +3279,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) {
> @@ -3296,6 +3294,10 @@ 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);

Wouldn't this allow two competiting config attempts to race each other and
leave wbt in an unexpected state?

task1 task2

ioc_qos_write() ioc_qos_write()
lock()
enable
unlock()
lock()
disable
unlock()
wbt_enable_default()
wbt_disable_default()

Thanks.

--
tejun