[PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Bart Van Assche]

Freezing the request queue from inside sysfs store callbacks may cause a
deadlock in combination with the dm-multipath driver and the
queue_if_no_path option. Additionally, freezing the request queue slows
down system boot on systems where sysfs attributes are set synchronously.

Fix this by removing the blk_mq_freeze_queue() / blk_mq_unfreeze_queue()
calls from the store callbacks that do not strictly need these callbacks.
This patch may cause a small delay in applying the new settings.

This patch affects the following sysfs attributes:
* io_poll_delay
* io_timeout
* nomerges
* read_ahead_kb
* rq_affinity

Here is an example of a deadlock triggered by running test srp/002:

task:multipathd
Call Trace:
 <TASK>
 __schedule+0x8c1/0x1bf0
 schedule+0xdd/0x270
 schedule_preempt_disabled+0x1c/0x30
 __mutex_lock+0xb89/0x1650
 mutex_lock_nested+0x1f/0x30
 dm_table_set_restrictions+0x823/0xdf0
 __bind+0x166/0x590
 dm_swap_table+0x2a7/0x490
 do_resume+0x1b1/0x610
 dev_suspend+0x55/0x1a0
 ctl_ioctl+0x3a5/0x7e0
 dm_ctl_ioctl+0x12/0x20
 __x64_sys_ioctl+0x127/0x1a0
 x64_sys_call+0xe2b/0x17d0
 do_syscall_64+0x96/0x3a0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53
 </TASK>
task:(udev-worker)
Call Trace:
 <TASK>
 __schedule+0x8c1/0x1bf0
 schedule+0xdd/0x270
 blk_mq_freeze_queue_wait+0xf2/0x140
 blk_mq_freeze_queue_nomemsave+0x23/0x30
 queue_ra_store+0x14e/0x290
 queue_attr_store+0x23e/0x2c0
 sysfs_kf_write+0xde/0x140
 kernfs_fop_write_iter+0x3b2/0x630
 vfs_write+0x4fd/0x1390
 ksys_write+0xfd/0x230
 __x64_sys_write+0x76/0xc0
 x64_sys_call+0x276/0x17d0
 do_syscall_64+0x96/0x3a0
 entry_SYSCALL_64_after_hwframe+0x4b/0x53
 </TASK>

Cc: Christoph Hellwig <hch@lst.de>
Cc: Nilay Shroff <nilay@linux.ibm.com>
Cc: Martin Wilck <mwilck@suse.com>
Cc: Benjamin Marzinski <bmarzins@redhat.com>
Cc: stable@vger.kernel.org
Fixes: af2814149883 ("block: freeze the queue in queue_attr_store")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---

Changes compared to v2:
 - Dropped the controversial patch "block: Restrict the duration of sysfs
   attribute changes".

Changes compared to v1:
 - Added patch "block: Restrict the duration of sysfs attribute changes".
 - Remove queue freezing from more sysfs callbacks.

 block/blk-sysfs.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 76c47fe9b8d6..c698d83cbd29 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -143,7 +143,6 @@ queue_ra_store(struct gendisk *disk, const char *page, size_t count)
 {
 	unsigned long ra_kb;
 	ssize_t ret;
-	unsigned int memflags;
 	struct request_queue *q = disk->queue;
 
 	ret = queue_var_store(&ra_kb, page, count);
@@ -154,10 +153,8 @@ queue_ra_store(struct gendisk *disk, const char *page, size_t count)
 	 * calculated from the queue limits by queue_limits_commit_update.
 	 */
 	mutex_lock(&q->limits_lock);
-	memflags = blk_mq_freeze_queue(q);
 	disk->bdi->ra_pages = ra_kb >> (PAGE_SHIFT - 10);
 	mutex_unlock(&q->limits_lock);
-	blk_mq_unfreeze_queue(q, memflags);
 
 	return ret;
 }
@@ -375,21 +372,18 @@ static ssize_t queue_nomerges_store(struct gendisk *disk, const char *page,
 				    size_t count)
 {
 	unsigned long nm;
-	unsigned int memflags;
 	struct request_queue *q = disk->queue;
 	ssize_t ret = queue_var_store(&nm, page, count);
 
 	if (ret < 0)
 		return ret;
 
-	memflags = blk_mq_freeze_queue(q);
 	blk_queue_flag_clear(QUEUE_FLAG_NOMERGES, q);
 	blk_queue_flag_clear(QUEUE_FLAG_NOXMERGES, q);
 	if (nm == 2)
 		blk_queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	else if (nm)
 		blk_queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
-	blk_mq_unfreeze_queue(q, memflags);
 
 	return ret;
 }
@@ -409,7 +403,6 @@ queue_rq_affinity_store(struct gendisk *disk, const char *page, size_t count)
 #ifdef CONFIG_SMP
 	struct request_queue *q = disk->queue;
 	unsigned long val;
-	unsigned int memflags;
 
 	ret = queue_var_store(&val, page, count);
 	if (ret < 0)
@@ -421,7 +414,6 @@ queue_rq_affinity_store(struct gendisk *disk, const char *page, size_t count)
 	 * are accessed individually using atomic test_bit operation. So we
 	 * don't grab any lock while updating these flags.
 	 */
-	memflags = blk_mq_freeze_queue(q);
 	if (val == 2) {
 		blk_queue_flag_set(QUEUE_FLAG_SAME_COMP, q);
 		blk_queue_flag_set(QUEUE_FLAG_SAME_FORCE, q);
@@ -432,7 +424,6 @@ queue_rq_affinity_store(struct gendisk *disk, const char *page, size_t count)
 		blk_queue_flag_clear(QUEUE_FLAG_SAME_COMP, q);
 		blk_queue_flag_clear(QUEUE_FLAG_SAME_FORCE, q);
 	}
-	blk_mq_unfreeze_queue(q, memflags);
 #endif
 	return ret;
 }
@@ -446,11 +437,9 @@ static ssize_t queue_poll_delay_store(struct gendisk *disk, const char *page,
 static ssize_t queue_poll_store(struct gendisk *disk, const char *page,
 				size_t count)
 {
-	unsigned int memflags;
 	ssize_t ret = count;
 	struct request_queue *q = disk->queue;
 
-	memflags = blk_mq_freeze_queue(q);
 	if (!(q->limits.features & BLK_FEAT_POLL)) {
 		ret = -EINVAL;
 		goto out;
@@ -459,7 +448,6 @@ static ssize_t queue_poll_store(struct gendisk *disk, const char *page,
 	pr_info_ratelimited("writes to the poll attribute are ignored.\n");
 	pr_info_ratelimited("please use driver specific parameters instead.\n");
 out:
-	blk_mq_unfreeze_queue(q, memflags);
 	return ret;
 }
 
@@ -472,7 +460,7 @@ static ssize_t queue_io_timeout_show(struct gendisk *disk, char *page)
 static ssize_t queue_io_timeout_store(struct gendisk *disk, const char *page,
 				  size_t count)
 {
-	unsigned int val, memflags;
+	unsigned int val;
 	int err;
 	struct request_queue *q = disk->queue;
 
@@ -480,9 +468,7 @@ static ssize_t queue_io_timeout_store(struct gendisk *disk, const char *page,
 	if (err || val == 0)
 		return -EINVAL;
 
-	memflags = blk_mq_freeze_queue(q);
 	blk_queue_rq_timeout(q, msecs_to_jiffies(val));
-	blk_mq_unfreeze_queue(q, memflags);
 
 	return count;
 }

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Martin Wilck]

On Thu, 2025-10-30 at 10:24 -0700, Bart Van Assche wrote:
> Freezing the request queue from inside sysfs store callbacks may
> cause a
> deadlock in combination with the dm-multipath driver and the
> queue_if_no_path option. Additionally, freezing the request queue
> slows
> down system boot on systems where sysfs attributes are set
> synchronously.
> 
> Fix this by removing the blk_mq_freeze_queue() /
> blk_mq_unfreeze_queue()
> calls from the store callbacks that do not strictly need these
> callbacks.
> This patch may cause a small delay in applying the new settings.
> 
> This patch affects the following sysfs attributes:
> * io_poll_delay
> * io_timeout
> * nomerges
> * read_ahead_kb
> * rq_affinity
> 
> ...
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Nilay Shroff <nilay@linux.ibm.com>
> Cc: Martin Wilck <mwilck@suse.com>
> Cc: Benjamin Marzinski <bmarzins@redhat.com>
> Cc: stable@vger.kernel.org
> Fixes: af2814149883 ("block: freeze the queue in queue_attr_store")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> Changes compared to v2:
>  - Dropped the controversial patch "block: Restrict the duration of
> sysfs
>    attribute changes".

So the "deadlock" situation for the other sysfs attributes that you
handled with the timeout in v2 will remain now? Are you planning to
send a follow-up patch for these attributes?

Martin

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Bart Van Assche]

On 10/30/25 10:38 AM, Martin Wilck wrote:
> So the "deadlock" situation for the other sysfs attributes that you
> handled with the timeout in v2 will remain now? Are you planning to
> send a follow-up patch for these attributes?

Only if agreement can be reached about the approach that should be used
to fix the deadlock for the remaining request queue sysfs attributes.

Thanks,

Bart.

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Christoph Hellwig]

On Thu, Oct 30, 2025 at 10:24:17AM -0700, Bart Van Assche wrote:
> Freezing the request queue from inside sysfs store callbacks may cause a
> deadlock in combination with the dm-multipath driver and the
> queue_if_no_path option. Additionally, freezing the request queue slows
> down system boot on systems where sysfs attributes are set synchronously.
> 
> Fix this by removing the blk_mq_freeze_queue() / blk_mq_unfreeze_queue()
> calls from the store callbacks that do not strictly need these callbacks.
> This patch may cause a small delay in applying the new settings.

You'll also need data_race or READ_ONCE annotations to satisfy the
memory model.

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Nilay Shroff]

On 10/30/25 10:54 PM, Bart Van Assche wrote:
> Fix this by removing the blk_mq_freeze_queue() / blk_mq_unfreeze_queue()
> calls from the store callbacks that do not strictly need these callbacks.
> This patch may cause a small delay in applying the new settings.
> 
> This patch affects the following sysfs attributes:
> * io_poll_delay
> * io_timeout
> * nomerges
> * read_ahead_kb
> * rq_affinity

I see that io_timeout, nomerges and rq_affinity are all accessed
during I/O hotpath. So IMO for these attributes we still need to
freeze the queue before updating those parameters. The io_timeout
and nomerges are accessed during I/O submission and rq_affinity
is accessed during I/O completion.

Thanks,
--Nilay

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Bart Van Assche]

On 10/31/25 5:39 AM, Nilay Shroff wrote:
> On 10/30/25 10:54 PM, Bart Van Assche wrote:
>> Fix this by removing the blk_mq_freeze_queue() / blk_mq_unfreeze_queue()
>> calls from the store callbacks that do not strictly need these callbacks.
>> This patch may cause a small delay in applying the new settings.
>>
>> This patch affects the following sysfs attributes:
>> * io_poll_delay
>> * io_timeout
>> * nomerges
>> * read_ahead_kb
>> * rq_affinity
> 
> I see that io_timeout, nomerges and rq_affinity are all accessed
> during I/O hotpath. So IMO for these attributes we still need to
> freeze the queue before updating those parameters. The io_timeout
> and nomerges are accessed during I/O submission and rq_affinity
> is accessed during I/O completion.

Yes, several of the parameters affected by my patch are used in the
hot path. But changing these parameters while I/O is in progress can't
cause an I/O error. That's why I don't think that the queue needs to be
frozen while these parameters are modified.

Bart.

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Ming Lei]

On Fri, Oct 31, 2025 at 8:40 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>
>
>
> On 10/30/25 10:54 PM, Bart Van Assche wrote:
> > Fix this by removing the blk_mq_freeze_queue() / blk_mq_unfreeze_queue()
> > calls from the store callbacks that do not strictly need these callbacks.
> > This patch may cause a small delay in applying the new settings.
> >
> > This patch affects the following sysfs attributes:
> > * io_poll_delay
> > * io_timeout
> > * nomerges
> > * read_ahead_kb
> > * rq_affinity
>
> I see that io_timeout, nomerges and rq_affinity are all accessed
> during I/O hotpath. So IMO for these attributes we still need to
> freeze the queue before updating those parameters. The io_timeout
> and nomerges are accessed during I/O submission and rq_affinity
> is accessed during I/O completion.

Does freeze make any difference? Intermediate value isn't possible, and
either the old or new value should be just fine to take.

Thanks,

Re: [PATCH v3] block: Remove queue freezing from several sysfs store callbacks

[Nilay Shroff]

On 11/3/25 9:42 AM, Ming Lei wrote:
> On Fri, Oct 31, 2025 at 8:40 PM Nilay Shroff <nilay@linux.ibm.com> wrote:
>>
>>
>>
>> On 10/30/25 10:54 PM, Bart Van Assche wrote:
>>> Fix this by removing the blk_mq_freeze_queue() / blk_mq_unfreeze_queue()
>>> calls from the store callbacks that do not strictly need these callbacks.
>>> This patch may cause a small delay in applying the new settings.
>>>
>>> This patch affects the following sysfs attributes:
>>> * io_poll_delay
>>> * io_timeout
>>> * nomerges
>>> * read_ahead_kb
>>> * rq_affinity
>>
>> I see that io_timeout, nomerges and rq_affinity are all accessed
>> during I/O hotpath. So IMO for these attributes we still need to
>> freeze the queue before updating those parameters. The io_timeout
>> and nomerges are accessed during I/O submission and rq_affinity
>> is accessed during I/O completion.
> 
> Does freeze make any difference? Intermediate value isn't possible, and
> either the old or new value should be just fine to take.
> 
Yes it doesn't affect the I/O and so if we remove freeze/unfreeze 
calls for these attributes then we may still need to annotate 
it with data_race and/or READ_ONCE as Christoph mentioned in another 
thread. I saw that nomerges and rq_affinity already uses atomic
bitmap and so we may still need to annotate io_timeout.

Thanks,
--Nilay