From: Yu Kuai <[email protected]>
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
From: Yu Kuai <[email protected]>
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 <[email protected]>
Signed-off-by: Yu Kuai <[email protected]>
---
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
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 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 <[email protected]>
Link: https://lore.kernel.org/lkml/Y+Ja5SRs886CEz7a@kadam/
Signed-off-by: Yu Kuai <[email protected]>
---
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
Looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On 5/12/23 3:35 AM, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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
On 12.05.23 16:58, Jens Axboe wrote:
> On 5/12/23 3:35 AM, Yu Kuai wrote:
>> From: Yu Kuai <[email protected]>
>>
>> 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)
--
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
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 <[email protected]>
>>>
>>> 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
>
> .
>
Hi, Christoph
?? 2023/05/12 17:35, Yu Kuai д??:
> From: Yu Kuai <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Yu Kuai <[email protected]>
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
> };
>
>
This patch looks good to me:
Reviewed-by: Christoph Hellwig <[email protected]>