block: elevator: avoid to load iosched module from this disk

PATCH In review Ming Lei 4 participants 20 messages Sep 7, 2024 – Sep 9, 2024 Block layer

lore

Share & tools

lore

Ming Lei

author

2024-09-07 01:43 UTC #0 lore raw

PATCH 1/1 block: elevator: avoid to load iosched module from this disk

When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.

This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.

Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.

Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.

Cc Richard Jones <[email protected]>

Cc Jeff Moyer <[email protected]>

Cc Jiri Jaburek <[email protected]>

Signed-off-by Ming Lei <[email protected]>

---
 block/elevator.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

block/elevator.c +14 -0

index f13d552a32c8..2b0432f4ac33 100644
@@ -676,6 +676,13 @@ void elevator_disable(struct request_queue *q)
 	blk_mq_unfreeze_queue(q);
 }
 
+static bool disk_is_claimed(struct gendisk *disk)
+{
+	if (disk->part0->bd_holder)
+		return true;
+	return false;
+}
+
 /*
  * Switch this queue to the given IO scheduler.
  */

@@ -699,6 +706,13 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 
 	e = elevator_find_get(q, elevator_name);
 	if (!e) {
+		/*
+		 * Try to avoid to load iosched module from FS behind our
+		 * disk, otherwise deadlock may be triggered
+		 */
+		if (disk_is_claimed(q->disk))
+			return -EDEADLK;
+
 		request_module("%s-iosched", elevator_name);
 		e = elevator_find_get(q, elevator_name);
 		if (!e)

Signature · 1 line

2.46.0

Richard W.M. Jones

Tested-by on v1 1/1

2024-09-07 07:35 UTC #1 lore raw

Quoted 9 lines · Ming Lei

When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.

Quoted 8 lines

Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>

I'd suggest also:

Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166

Reported-by Richard W.M. Jones <[email protected]>

Reported-by Jiri Jaburek <[email protected]>

Tested-by Richard W.M. Jones <[email protected]>

So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:

+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided

(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)

Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.

Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):

commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200

block: freeze the queue in queue_attr_store

queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.

Reverting this commit on top of git head also fixes the problem.

Why did this commit expose the problem?

Rich.

Quoted 38 lines

---
 block/elevator.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

block/elevator.c +14 -0

index f13d552a32c8..2b0432f4ac33 100644
@@ -676,6 +676,13 @@ void elevator_disable(struct request_queue *q)
 	blk_mq_unfreeze_queue(q);
 }
 
+static bool disk_is_claimed(struct gendisk *disk)
+{
+	if (disk->part0->bd_holder)
+		return true;
+	return false;
+}
+
 /*
  * Switch this queue to the given IO scheduler.
  */

@@ -699,6 +706,13 @@ static int elevator_change(struct request_queue *q, const char *elevator_name)
 
 	e = elevator_find_get(q, elevator_name);
 	if (!e) {
+		/*
+		 * Try to avoid to load iosched module from FS behind our
+		 * disk, otherwise deadlock may be triggered
+		 */
+		if (disk_is_claimed(q->disk))
+			return -EDEADLK;
+
 		request_module("%s-iosched", elevator_name);
 		e = elevator_find_get(q, elevator_name);
 		if (!e)

Signature · 1 line

2.46.0

Signature · 5 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Ming Lei

author

2024-09-07 07:59 UTC #2 lore raw

Quoted 29 lines · Richard W.M. Jones

Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:

Quoted 6 lines

+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)

Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.

Quoted 4 lines

Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.

As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.

Quoted 10 lines

Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store

Quoted 7 lines

queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?

That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.

Another candidate fix is to revert it, or at least not do it for storing elevator attribute.

Thanks,
Ming

Damien Le Moal

2024-09-07 09:05 UTC #3 lore raw

Quoted 64 lines · Ming Lei

Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 16 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.

Quoted 8 lines

Quoted 1 line
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.

I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).

block/blk-sysfs.c +16 -1

index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);
        ssize_t (*store)(struct gendisk *disk, const char *page, size_t count);
 };

@@ -413,6 +414,14 @@ static struct queue_sysfs_entry _prefix##_entry = {        \
        .store  = _prefix##_store,                      \
 };
 
+#define QUEUE_RPW_ENTRY(_prefix, _name)                        \
+static struct queue_sysfs_entry _prefix##_entry = {    \
+       .attr   = { .name = _name, .mode = 0644 },      \
+       .show   = _prefix##_show,                       \
+       .pre_store = _prefix##_pre_store,               \
+       .store  = _prefix##_store,                      \
+};
+
 QUEUE_RW_ENTRY(queue_requests, "nr_requests");
 QUEUE_RW_ENTRY(queue_ra, "read_ahead_kb");
 QUEUE_RW_ENTRY(queue_max_sectors, "max_sectors_kb");

@@ -420,7 +429,7 @@ QUEUE_RO_ENTRY(queue_max_hw_sectors, "max_hw_sectors_kb");
 QUEUE_RO_ENTRY(queue_max_segments, "max_segments");
 QUEUE_RO_ENTRY(queue_max_integrity_segments, "max_integrity_segments");
 QUEUE_RO_ENTRY(queue_max_segment_size, "max_segment_size");
-QUEUE_RW_ENTRY(elv_iosched, "scheduler");
+QUEUE_RPW_ENTRY(elv_iosched, "scheduler");
 
 QUEUE_RO_ENTRY(queue_logical_block_size, "logical_block_size");
 QUEUE_RO_ENTRY(queue_physical_block_size, "physical_block_size");

@@ -670,6 +679,12 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
        if (!entry->store)
                return -EIO;
 
+       if (entry->pre_store) {
+               res = entry->pre_store(disk, page, length);
+               if (res)
+                       return res;
+       }
+
        blk_mq_freeze_queue(q);
        mutex_lock(&q->sysfs_lock);
        res = entry->store(disk, page, length);

block/elevator.c +0 -0

index f13d552a32c8..c338282d5148 100644
@@ -698,17 +698,26 @@ static int elevator_change(struct request_queue *q, const

char *elevator_name)

                return 0;

        e = elevator_find_get(q, elevator_name);
-       if (!e) {
-               request_module("%s-iosched", elevator_name);
-               e = elevator_find_get(q, elevator_name);
-               if (!e)
-                       return -EINVAL;
-       }
+       if (!e)
+               return -EINVAL;
        ret = elevator_switch(q, e);
        elevator_put(e);
        return ret;

}

+int elv_iosched_pre_store(struct gendisk *disk, const char *buf,
+ size_t count)
+{
+ char elevator_name[ELV_NAME_MAX];
+
+ if (!elv_support_iosched(disk->queue))
+ return -ENOTSUPP;
+
+ strscpy(elevator_name, buf, sizeof(elevator_name));
+
+ return request_module("%s-iosched", elevator_name);
+}
+
ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
size_t count)
{

block/elevator.h +1 -0

index 3fe18e1a8692..059172c0f93c 100644
@@ -148,6 +148,7 @@ extern void elv_unregister(struct elevator_type *);
  * io scheduler sysfs switching
  */
 ssize_t elv_iosched_show(struct gendisk *disk, char *page);
+int elv_iosched_pre_store(struct gendisk *disk, const char *page, size_t count);
 ssize_t elv_iosched_store(struct gendisk *disk, const char *page, size_t count);
 
 extern bool elv_bio_merge_ok(struct request *, struct bio *);

Signature · 2 lines

Damien Le Moal
Western Digital Research

Ming Lei

author

2024-09-07 09:49 UTC #4 lore raw

Quoted 67 lines · Damien Le Moal

Quoted 66 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 17 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?

Quoted 8 lines

Quoted 6 lines
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply

Right, I remember that the freezing starts to cover update of max_sectors_kb.

block/blk-sysfs.c line 26

Quoted 12 lines

be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).

block/blk-sysfs.c +1 -0

index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);

It seems over-kill to add one new callback, and another way is just to not freeze queue for storing elevator.

But if other attribute update needs to not freeze queue, 'pre_store' looks one reasonable solution.

block/blk-sysfs.c +11 -2

index 60116d13cb80..c418edf66f0c 100644
@@ -666,15 +666,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
 	struct request_queue *q = disk->queue;
 	ssize_t res;
+	bool need_freeze;
 
 	if (!entry->store)
 		return -EIO;
 
-	blk_mq_freeze_queue(q);
+	/*
+	 * storing scheduler freezes queue in its way, especially
+	 * loading scheduler module can't be done when queue is frozen
+	 */
+	need_freeze = (entry->store == elv_iosched_store);
+
+	if (need_freeze)
+		blk_mq_freeze_queue(q);
 	mutex_lock(&q->sysfs_lock);
 	res = entry->store(disk, page, length);
 	mutex_unlock(&q->sysfs_lock);
-	blk_mq_unfreeze_queue(q);
+	if (need_freeze)
+		blk_mq_unfreeze_queue(q);
 	return res;
 }

Thanks,
Ming

Richard W.M. Jones

2024-09-07 10:02 UTC #5 lore raw

Quoted 120 lines · Ming Lei

Quoted 67 lines · Damien Le Moal
Quoted 66 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 17 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
Quoted 8 lines
Quoted 6 lines
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply
Right, I remember that the freezing starts to cover update of max_sectors_kb.
block/blk-sysfs.c line 26
Quoted 12 lines
be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
block/blk-sysfs.c +1 -0
index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);
It seems over-kill to add one new callback, and another way is just to not freeze queue for storing elevator.
But if other attribute update needs to not freeze queue, 'pre_store' looks one reasonable solution.
block/blk-sysfs.c +8 -1
index 60116d13cb80..c418edf66f0c 100644
@@ -666,15 +666,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
 	struct request_queue *q = disk->queue;
 	ssize_t res;
+	bool need_freeze;
 
 	if (!entry->store)
 		return -EIO;
 
-	blk_mq_freeze_queue(q);
+	/*
+	 * storing scheduler freezes queue in its way, especially
+	 * loading scheduler module can't be done when queue is frozen
+	 */
+	need_freeze = (entry->store == elv_iosched_store);
+
+	if (need_freeze)

block/blk-sysfs.c queue_attr_store(struct kobject *kobj, struct attribute *attr, · line 689

Quoted 10 lines

block/blk-sysfs.c +3 -1

+		blk_mq_freeze_queue(q);
 	mutex_lock(&q->sysfs_lock);
 	res = entry->store(disk, page, length);
 	mutex_unlock(&q->sysfs_lock);
-	blk_mq_unfreeze_queue(q);
+	if (need_freeze)
+		blk_mq_unfreeze_queue(q);
 	return res;
 }

Unfortunately this doesn't fix the problem for me. The test still hangs occasionally in the same way as before.

Rich.

Signature · 4 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Ming Lei

author

2024-09-07 10:07 UTC #6 lore raw

Quoted 126 lines · Richard W.M. Jones

Quoted 125 lines · Ming Lei
Quoted 67 lines · Damien Le Moal
Quoted 66 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 17 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
Quoted 8 lines
Quoted 6 lines
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply
Right, I remember that the freezing starts to cover update of max_sectors_kb.
block/blk-sysfs.c line 26
Quoted 12 lines
be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
block/blk-sysfs.c +1 -0
index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);
It seems over-kill to add one new callback, and another way is just to not freeze queue for storing elevator.
But if other attribute update needs to not freeze queue, 'pre_store' looks one reasonable solution.
block/blk-sysfs.c +9 -2
index 60116d13cb80..c418edf66f0c 100644
@@ -666,15 +666,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
 	struct request_queue *q = disk->queue;
 	ssize_t res;
+	bool need_freeze;
 
 	if (!entry->store)
 		return -EIO;
 
-	blk_mq_freeze_queue(q);
+	/*
+	 * storing scheduler freezes queue in its way, especially
+	 * loading scheduler module can't be done when queue is frozen
+	 */
+	need_freeze = (entry->store == elv_iosched_store);
+
+	if (need_freeze)
+		blk_mq_freeze_queue(q);
 	mutex_lock(&q->sysfs_lock);
 	res = entry->store(disk, page, length);
 	mutex_unlock(&q->sysfs_lock);
-	blk_mq_unfreeze_queue(q);

Quoted 8 lines

block/blk-sysfs.c queue_attr_store(struct kobject *kobj, struct attribute *attr, · line 689
Quoted 5 lines
block/blk-sysfs.c +2 -0
+	if (need_freeze)
+		blk_mq_unfreeze_queue(q);
 	return res;
 }
 
Unfortunately this doesn't fix the problem for me. The test still hangs occasionally in the same way as before.

'need_freeze' needs to be flipped by:

	need_freeze = (entry->store != elv_iosched_store);

thanks,
Ming

Richard W.M. Jones

2024-09-07 10:36 UTC #7 lore raw

Quoted 136 lines · Ming Lei

Quoted 126 lines · Richard W.M. Jones
Quoted 125 lines · Ming Lei
Quoted 67 lines · Damien Le Moal
Quoted 66 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 17 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
Quoted 8 lines
Quoted 6 lines
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply
Right, I remember that the freezing starts to cover update of max_sectors_kb.
block/blk-sysfs.c line 26
Quoted 12 lines
be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
block/blk-sysfs.c +1 -0
index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);
It seems over-kill to add one new callback, and another way is just to not freeze queue for storing elevator.
But if other attribute update needs to not freeze queue, 'pre_store' looks one reasonable solution.
block/blk-sysfs.c +9 -2
index 60116d13cb80..c418edf66f0c 100644
@@ -666,15 +666,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
 	struct request_queue *q = disk->queue;
 	ssize_t res;
+	bool need_freeze;
 
 	if (!entry->store)
 		return -EIO;
 
-	blk_mq_freeze_queue(q);
+	/*
+	 * storing scheduler freezes queue in its way, especially
+	 * loading scheduler module can't be done when queue is frozen
+	 */
+	need_freeze = (entry->store == elv_iosched_store);
+
+	if (need_freeze)
+		blk_mq_freeze_queue(q);
 	mutex_lock(&q->sysfs_lock);
 	res = entry->store(disk, page, length);
 	mutex_unlock(&q->sysfs_lock);
-	blk_mq_unfreeze_queue(q);
Quoted 8 lines
block/blk-sysfs.c queue_attr_store(struct kobject *kobj, struct attribute *attr, · line 689
Quoted 5 lines
block/blk-sysfs.c +2 -0
+	if (need_freeze)
+		blk_mq_unfreeze_queue(q);
 	return res;
 }
 
Unfortunately this doesn't fix the problem for me. The test still hangs occasionally in the same way as before.

Quoted 3 lines

'need_freeze' needs to be flipped by:
	need_freeze = (entry->store != elv_iosched_store);

I'm still running the test (takes 5,000 boot iterations before I can be "sure"), but so far it seems flipping this test fixes the bug.

This seems like the neatest (or shortest) fix so far, but doesn't it "mix up layers" by checking elv_iosched_store?

Rich.

Signature · 4 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Richard W.M. Jones

2024-09-07 11:01 UTC #8 lore raw

Quoted 2 lines · Richard W.M. Jones

I'm still running the test (takes 5,000 boot iterations before I can be "sure"), but so far it seems flipping this test fixes the bug.

This passed 5,000 iterations.

Rich.

Signature · 5 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Ming Lei

author

2024-09-07 11:02 UTC #9 lore raw

In reply to Richard W.M. Jones

Quoted 137 lines · Richard W.M. Jones

Quoted 136 lines · Ming Lei
Quoted 134 lines · Richard W.M. Jones
Quoted 120 lines · Ming Lei
Quoted 67 lines · Damien Le Moal
Quoted 66 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 17 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
Quoted 8 lines
Quoted 6 lines
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply
Right, I remember that the freezing starts to cover update of max_sectors_kb.
block/blk-sysfs.c line 26
Quoted 12 lines
be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
block/blk-sysfs.c +1 -0
index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);
It seems over-kill to add one new callback, and another way is just to not freeze queue for storing elevator.
But if other attribute update needs to not freeze queue, 'pre_store' looks one reasonable solution.
block/blk-sysfs.c +8 -1
index 60116d13cb80..c418edf66f0c 100644
@@ -666,15 +666,24 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 	struct gendisk *disk = container_of(kobj, struct gendisk, queue_kobj);
 	struct request_queue *q = disk->queue;
 	ssize_t res;
+	bool need_freeze;
 
 	if (!entry->store)
 		return -EIO;
 
-	blk_mq_freeze_queue(q);
+	/*
+	 * storing scheduler freezes queue in its way, especially
+	 * loading scheduler module can't be done when queue is frozen
+	 */
+	need_freeze = (entry->store == elv_iosched_store);
+
+	if (need_freeze)
block/blk-sysfs.c queue_attr_store(struct kobject *kobj, struct attribute *attr, · line 689
Quoted 10 lines
block/blk-sysfs.c +3 -1
+		blk_mq_freeze_queue(q);
 	mutex_lock(&q->sysfs_lock);
 	res = entry->store(disk, page, length);
 	mutex_unlock(&q->sysfs_lock);
-	blk_mq_unfreeze_queue(q);
+	if (need_freeze)
+		blk_mq_unfreeze_queue(q);
 	return res;
 }
 
Unfortunately this doesn't fix the problem for me. The test still hangs occasionally in the same way as before.

Quoted 6 lines

Quoted 3 lines
'need_freeze' needs to be flipped by:
	need_freeze = (entry->store != elv_iosched_store);
I'm still running the test (takes 5,000 boot iterations before I can be "sure"), but so far it seems flipping this test fixes the bug.

BTW, the issue can be reproduced 100% by:

echo "deadlock" > /sys/block/$ROOT_DISK/queue/scheduler

Quoted 3 lines

This seems like the neatest (or shortest) fix so far, but doesn't it "mix up layers" by checking elv_iosched_store?

It is just one exception for 'scheduler' sysfs attribute wrt. freezing queue for storing, and the check can be done via the attribute name("scheduler") too.

Thanks,
Ming

Richard W.M. Jones

2024-09-07 11:15 UTC #10 lore raw

Quoted 3 lines · Ming Lei

BTW, the issue can be reproduced 100% by:
echo "deadlock" > /sys/block/$ROOT_DISK/queue/scheduler

That doesn't reproduce it for me (reliably). Although I'm not surprised as this bug has been _very_ tricky to reproduce! Sometimes I think I have a definite reproducer, only for it to go away when some tiny detail changes.

Quoted 6 lines

Quoted 2 lines
This seems like the neatest (or shortest) fix so far, but doesn't it "mix up layers" by checking elv_iosched_store?
It is just one exception for 'scheduler' sysfs attribute wrt. freezing queue for storing, and the check can be done via the attribute name("scheduler") too.

Fair enough.

Rich.

Signature · 4 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Damien Le Moal

2024-09-08 00:02 UTC #11 lore raw

Quoted 4 lines · Richard W.M. Jones

Quoted 3 lines · Ming Lei
BTW, the issue can be reproduced 100% by:
echo "deadlock" > /sys/block/$ROOT_DISK/queue/scheduler

This probably should be:

echo "mq-deadline" > /sys/block/$ROOT_DISK/queue/scheduler

and make sure that:
1) mq-deadline is compiled as a module
2) mq-deadline is not already used by a device (so not loaded already)
3) The mq-deadline module file is stored on the target device of the scheduler change
4) The mq-deadline module file is not already cahced in the page cache.

For (4), you may want to do a "echo 3 > /proc/sys/vm/drop_caches" before trying to switch the scheduler.

Quoted 17 lines

That doesn't reproduce it for me (reliably). Although I'm not surprised as this bug has been _very_ tricky to reproduce! Sometimes I think I have a definite reproducer, only for it to go away when some tiny detail changes.
Quoted 6 lines
Quoted 2 lines
This seems like the neatest (or shortest) fix so far, but doesn't it "mix up layers" by checking elv_iosched_store?
It is just one exception for 'scheduler' sysfs attribute wrt. freezing queue for storing, and the check can be done via the attribute name("scheduler") too.
Fair enough.
Rich.

Signature · 2 lines

Damien Le Moal
Western Digital Research

Ming Lei

author

2024-09-09 01:00 UTC #12 lore raw

Quoted 43 lines · Damien Le Moal

Quoted 4 lines · Richard W.M. Jones
Quoted 3 lines · Ming Lei
BTW, the issue can be reproduced 100% by:
echo "deadlock" > /sys/block/$ROOT_DISK/queue/scheduler
This probably should be:
echo "mq-deadline" > /sys/block/$ROOT_DISK/queue/scheduler
and make sure that:
1) mq-deadline is compiled as a module
2) mq-deadline is not already used by a device (so not loaded already)
3) The mq-deadline module file is stored on the target device of the scheduler change
4) The mq-deadline module file is not already cahced in the page cache.
For (4), you may want to do a "echo 3 > /proc/sys/vm/drop_caches" before trying to switch the scheduler.
Quoted 9 lines
That doesn't reproduce it for me (reliably). Although I'm not surprised as this bug has been _very_ tricky to reproduce! Sometimes I think I have a definite reproducer, only for it to go away when some tiny detail changes.
Quoted 3 lines
Quoted 2 lines
This seems like the neatest (or shortest) fix so far, but doesn't it "mix up layers" by checking elv_iosched_store?
Quoted 8 lines
Quoted 3 lines
It is just one exception for 'scheduler' sysfs attribute wrt. freezing queue for storing, and the check can be done via the attribute name("scheduler") too.
Fair enough.
Rich.
--
Damien Le Moal
Western Digital Research

Ming Lei

author

2024-09-09 01:01 UTC #13 lore raw

In reply to Damien Le Moal

Quoted 10 lines · Damien Le Moal

Quoted 4 lines · Richard W.M. Jones
Quoted 3 lines · Ming Lei
BTW, the issue can be reproduced 100% by:
echo "deadlock" > /sys/block/$ROOT_DISK/queue/scheduler
This probably should be:
echo "mq-deadline" > /sys/block/$ROOT_DISK/queue/scheduler

No, it is deliberately something not existing.

If it can't work, dropping cache can be added before the switching.

Thanks,

Richard W.M. Jones

2024-09-07 09:53 UTC #14 lore raw

In reply to Damien Le Moal

Quoted 170 lines · Damien Le Moal

Quoted 64 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 16 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Quoted 8 lines
Quoted 1 line
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
block/blk-sysfs.c +16 -1
index 60116d13cb80..aef87f6b4a8a 100644
@@ -23,6 +23,7 @@
 struct queue_sysfs_entry {
        struct attribute attr;
        ssize_t (*show)(struct gendisk *disk, char *page);
+       int (*pre_store)(struct gendisk *disk, const char *page, size_t count);
        ssize_t (*store)(struct gendisk *disk, const char *page, size_t count);
 };
 
@@ -413,6 +414,14 @@ static struct queue_sysfs_entry _prefix##_entry = {        \
        .store  = _prefix##_store,                      \
 };
 
+#define QUEUE_RPW_ENTRY(_prefix, _name)                        \
+static struct queue_sysfs_entry _prefix##_entry = {    \
+       .attr   = { .name = _name, .mode = 0644 },      \
+       .show   = _prefix##_show,                       \
+       .pre_store = _prefix##_pre_store,               \
+       .store  = _prefix##_store,                      \
+};
+
 QUEUE_RW_ENTRY(queue_requests, "nr_requests");
 QUEUE_RW_ENTRY(queue_ra, "read_ahead_kb");
 QUEUE_RW_ENTRY(queue_max_sectors, "max_sectors_kb");
@@ -420,7 +429,7 @@ QUEUE_RO_ENTRY(queue_max_hw_sectors, "max_hw_sectors_kb");
 QUEUE_RO_ENTRY(queue_max_segments, "max_segments");
 QUEUE_RO_ENTRY(queue_max_integrity_segments, "max_integrity_segments");
 QUEUE_RO_ENTRY(queue_max_segment_size, "max_segment_size");
-QUEUE_RW_ENTRY(elv_iosched, "scheduler");
+QUEUE_RPW_ENTRY(elv_iosched, "scheduler");
 
 QUEUE_RO_ENTRY(queue_logical_block_size, "logical_block_size");
 QUEUE_RO_ENTRY(queue_physical_block_size, "physical_block_size");
@@ -670,6 +679,12 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
        if (!entry->store)
                return -EIO;
 
+       if (entry->pre_store) {
+               res = entry->pre_store(disk, page, length);
+               if (res)
+                       return res;
+       }
+
        blk_mq_freeze_queue(q);
        mutex_lock(&q->sysfs_lock);
        res = entry->store(disk, page, length);
block/elevator.c +15 -6
index f13d552a32c8..c338282d5148 100644
@@ -698,17 +698,26 @@ static int elevator_change(struct request_queue *q, const
 char *elevator_name)
                return 0;
 
        e = elevator_find_get(q, elevator_name);
-       if (!e) {
-               request_module("%s-iosched", elevator_name);
-               e = elevator_find_get(q, elevator_name);
-               if (!e)
-                       return -EINVAL;
-       }
+       if (!e)
+               return -EINVAL;
        ret = elevator_switch(q, e);
        elevator_put(e);
        return ret;
 }
 
+int elv_iosched_pre_store(struct gendisk *disk, const char *buf,
+                          size_t count)
+{
+       char elevator_name[ELV_NAME_MAX];
+
+       if (!elv_support_iosched(disk->queue))
+               return -ENOTSUPP;
+
+       strscpy(elevator_name, buf, sizeof(elevator_name));
+
+       return request_module("%s-iosched", elevator_name);
+}
+
 ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
                          size_t count)
{
block/elevator.h +0 -0
index 3fe18e1a8692..059172c0f93c 100644

block/elevator.h extern void elv_unregister(struct elevator_type *); · line 154

Quoted 7 lines

block/elevator.h +1 -0

@@ -148,6 +148,7 @@ extern void elv_unregister(struct elevator_type *);
  * io scheduler sysfs switching
  */
 ssize_t elv_iosched_show(struct gendisk *disk, char *page);
+int elv_iosched_pre_store(struct gendisk *disk, const char *page, size_t count);
 ssize_t elv_iosched_store(struct gendisk *disk, const char *page, size_t count);
 
 extern bool elv_bio_merge_ok(struct request *, struct bio *);

I tested this on top of current git head and it fixes the problem for me.

Rich.

Signature · 4 lines

Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Jens Axboe

maintainer

2024-09-07 13:50 UTC #15 lore raw

In reply to Damien Le Moal

Quoted 71 lines · Damien Le Moal

Quoted 70 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 10 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
Quoted 7 lines
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.

Quoted 6 lines

Quoted 2 lines
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).

I like this approach, but let's please call it something descriptive like "load_module" or something like that.

Signature · 1 line

Jens Axboe

Ming Lei

author

2024-09-09 01:25 UTC #16 lore raw

Quoted 75 lines · Jens Axboe

Quoted 74 lines · Damien Le Moal
Quoted 72 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 10 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
Quoted 7 lines
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.

Quoted 6 lines

Quoted 3 lines
I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
I like this approach, but let's please call it something descriptive like "load_module" or something like that.

But 'load_module' is too specific as interface, and we just only have one case which need to load module exactly.

I guess there may be same risk in queue_wb_lat_store() which calls into GFP_KERNEL allocation which implies direct reclaim & IO.

Thanks,
Ming

Damien Le Moal

2024-09-09 01:56 UTC #17 lore raw

Quoted 80 lines · Ming Lei

Quoted 79 lines · Jens Axboe
Quoted 77 lines · Damien Le Moal
Quoted 64 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 16 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Quoted 8 lines
Quoted 1 line
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).

Quoted 8 lines

Quoted 2 lines
I like this approach, but let's please call it something descriptive like "load_module" or something like that.
But 'load_module' is too specific as interface, and we just only have one case which need to load module exactly.
I guess there may be same risk in queue_wb_lat_store() which calls into GFP_KERNEL allocation which implies direct reclaim & IO.

That needs to be changed to GFP_NOIO.

Quoted 4 lines

Thanks,
Ming

Signature · 2 lines

Damien Le Moal
Western Digital Research

Damien Le Moal

2024-09-09 01:59 UTC #18 lore raw

In reply to Ming Lei

Quoted 80 lines · Ming Lei

Quoted 79 lines · Jens Axboe
Quoted 77 lines · Damien Le Moal
Quoted 64 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 16 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Quoted 8 lines
Quoted 1 line
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).

Quoted 5 lines

Quoted 2 lines
I like this approach, but let's please call it something descriptive like "load_module" or something like that.
But 'load_module' is too specific as interface, and we just only have one case which need to load module exactly.

If another attr needs to do some prep work before freezing the queue and calling attr->store(), we can rename the load_module attribute method to something like "prepare_store" to be more generic.

Quoted 7 lines

I guess there may be same risk in queue_wb_lat_store() which calls into GFP_KERNEL allocation which implies direct reclaim & IO.
Thanks,
Ming

Signature · 2 lines

Damien Le Moal
Western Digital Research

Ming Lei

author

2024-09-09 02:17 UTC #19 lore raw

Quoted 84 lines · Damien Le Moal

Quoted 83 lines · Ming Lei
Quoted 81 lines · Jens Axboe
Quoted 71 lines · Damien Le Moal
Quoted 70 lines · Ming Lei
Quoted 29 lines · Richard W.M. Jones
Quoted 9 lines · Ming Lei
When switching io scheduler via sysfs, 'request_module' may be called if the specified scheduler doesn't exist.
This was has deadlock risk because the module may be stored on FS behind our disk since request queue is frozen before switching its elevator.
Fix it by returning -EDEADLK in case that the disk is claimed, which can be thought as one signal that the disk is mounted.
Quoted 8 lines
Some distributions(Fedora) simulates the original kernel command line of 'elevator=foo' via 'echo foo > /sys/block/$DISK/queue/scheduler', and boot hang is triggered.
Cc Richard Jones <[email protected]>
Cc Jeff Moyer <[email protected]>
Cc Jiri Jaburek <[email protected]>
Signed-off-by Ming Lei <[email protected]>
I'd suggest also:
Bug https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by Richard W.M. Jones <[email protected]>
Reported-by Jiri Jaburek <[email protected]>
Tested-by Richard W.M. Jones <[email protected]>
So I have tested this patch and it does fix the issue, at the possible cost that now setting the scheduler can fail:
Quoted 6 lines
+ for f in /sys/block/{h,s,ub,v}d*/queue/scheduler
+ echo noop
/init: line 109: echo: write error: Resource deadlock avoided
(I know I'm setting it to an impossible value here, but this could also happen when setting it to a valid one.)
Actually in most of dist, io-schedulers are built-in, so request_module is just a nop, but meta IO must be started.
Quoted 4 lines
Since almost no one checks the result of 'echo foo > /sys/...' that would probably mean that sometimes a desired setting is silently not set.
As I mentioned, io-schedulers are built-in for most of dist, so request_module isn't called in case of one valid io-sched.
Quoted 10 lines
Also I bisected this bug yesterday and found it was caused by (or, more likely, exposed by):
commit af2814149883e2c1851866ea2afcd8eadc040f79
Author: Christoph Hellwig <[email protected]>
Date: Mon Jun 17 08:04:38 2024 +0200
block: freeze the queue in queue_attr_store
Quoted 7 lines
queue_attr_store updates attributes used to control generating I/O, and can cause malformed bios if changed with I/O in flight. Freeze the queue in common code instead of adding it to almost every attribute.
Reverting this commit on top of git head also fixes the problem.
Why did this commit expose the problem?
That is really the 1st bad commit which moves queue freezing before calling request_module(), originally we won't freeze queue until we have to do it.
Quoted 6 lines
Quoted 2 lines
Another candidate fix is to revert it, or at least not do it for storing elevator attribute.
I do not think that reverting is acceptable. Rather, a proper fix would simply be to do the request_module() before freezing the queue. Something like below should work (totally untested and that may be overkill).
I like this approach, but let's please call it something descriptive like "load_module" or something like that.

Quoted 6 lines

Quoted 2 lines
But 'load_module' is too specific as interface, and we just only have one case which need to load module exactly.
If another attr needs to do some prep work before freezing the queue and calling attr->store(), we can rename the load_module attribute method to something like "prepare_store" to be more generic.

'interface' is supposed to be generic from beginning, and I don't think we will have another 'load_module' case here.

Thanks,
Ming