block: Prevent deadlocks when switching elevators
PATCH 1/1 block: Prevent deadlocks when switching elevators
Commit af2814149883 ("block: freeze the queue in queue_attr_store") changed queue_attr_store() to always freeze a sysfs attribute queue before calling the attribute store() method, to ensure that no IOs are in-flight when an attribute value is being updated.
However, this change created a potential deadlock situation for the scheduler queue attribute as changing the queue elevator with elv_iosched_store() can result in a call to request_module() if the user requested module is not already registered. If the file of the requested module is stored on the block device of the frozen queue, a deadlock will happen as the read operations triggered by request_module() will wait for the queue freeze to end.
Solve this issue by introducing the load_module method in struct queue_sysfs_entry, and to calling this method function in queue_attr_store() before freezing the attribute queue.
The macro definition QUEUE_RW_LOAD_MODULE_ENTRY() is added to define a queue sysfs attribute that needs loading a module.
The definition of the scheduler atrribute is changed to using QUEUE_RW_LOAD_MODULE_ENTRY(), with the function elv_iosched_load_module() defined as the load_module method. elv_iosched_store() can then be simplified to remove the call to request_module().
--- block/blk-sysfs.c | 22 +++++++++++++++++++++- block/elevator.c | 21 +++++++++++++++------ block/elevator.h | 2 ++ 3 files changed, 38 insertions(+), 7 deletions(-)
block/blk-sysfs.c +21 -1
@@ -23,6 +23,7 @@struct queue_sysfs_entry {struct attribute attr;ssize_t (*show)(struct gendisk *disk,char *page); +int (*load_module)(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_RW_LOAD_MODULE_ENTRY (_prefix, _name) \ +static struct queue_sysfs_entry _prefix##_entry = { \ + .attr = { .name = _name, .mode = 0644 }, \ + .show = _prefix##_show, \ + .load_module = _prefix##_load_module, \ + .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_RW_LOAD_MODULE_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,17 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,if (!entry->store)return -EIO ; +/* +* If the attribute needs to load a module, do it before freezing the +* queue to ensure that the module file can be read when the request +* queue is the one for the device storing the module file. +*/ +if (entry->load_module) { + res = entry->load_module (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
@@ -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_load_module (struct gendisk *disk,const char *buf, +size_t count) +{ +char elevator_name[ELV_NAME_MAX ]; + +if (!elv_support_iosched (disk->queue)) +return -EOPNOTSUPP ; + +strscpy (elevator_name, buf,sizeof (elevator_name)); + +return request_module ("%s-iosched" ,strstrip (elevator_name)); +} +ssize_t elv_iosched_store (struct gendisk *disk,const char *buf,size_t count) {
block/elevator.h +2 -0
@@ -148,6 +148,8 @@ extern void elv_unregister(struct elevator_type *);* io scheduler sysfs switching */ ssize_t elv_iosched_show (struct gendisk *disk,char *page); +int elv_iosched_load_module (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 · 1 line
2.46.0
Quoted 25 lines · Damien Le Moal
Commit
af2814149883("block: freeze the queue inqueue_attr_store") changedqueue_attr_store()to always freeze a sysfs attribute queue before calling the attributestore()method, to ensure that no IOs are in-flight when an attribute value is being updated.However, this change created a potential deadlock situation for the scheduler queue attribute as changing the queue elevator with
elv_iosched_store()can result in a call torequest_module()if the user requested module is not already registered. If the file of the requested module is stored on the block device of the frozen queue, a deadlock will happen as the read operations triggered byrequest_module()will wait for the queue freeze to end.Solve this issue by introducing the
load_modulemethod in structqueue_sysfs_entry, and to calling this method function inqueue_attr_store()before freezing the attribute queue.
The macro definitionQUEUE_RW_LOAD_MODULE_ENTRY()is added to define a queue sysfs attribute that needs loading a module.The definition of the scheduler atrribute is changed to using
QUEUE_RW_LOAD_MODULE_ENTRY(), with the functionelv_iosched_load_module()defined as theload_modulemethod.elv_iosched_store()can then be simplified to remove the call torequest_module().
Quoted 6 lines
Reported-by Richard W.M. Jones <[email protected]>Reported-by Jiri Jaburek <[email protected]>Fixes af2814149883 ("block: freeze the queue in queue_attr_store")Signed-off-by Damien Le Moal <[email protected]>
I tested this (using my "unreliable" test!), and it passed 5,000 iterations, so:
Thanks,
Rich.
Quoted 112 lines
--- block/blk-sysfs.c | 22 +++++++++++++++++++++- block/elevator.c | 21 +++++++++++++++------ block/elevator.h | 2 ++ 3 files changed, 38 insertions(+), 7 deletions(-)block/blk-sysfs.c +21
-1@@ -23,6 +23,7 @@struct queue_sysfs_entry {struct attribute attr;ssize_t (*show)(struct gendisk *disk,char *page); +int (*load_module)(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_RW_LOAD_MODULE_ENTRY (_prefix, _name) \ +static struct queue_sysfs_entry _prefix##_entry = { \ + .attr = { .name = _name, .mode = 0644 }, \ + .show = _prefix##_show, \ + .load_module = _prefix##_load_module, \ + .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_RW_LOAD_MODULE_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,17 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,if (!entry->store)return -EIO ; +/* +* If the attribute needs to load a module, do it before freezing the +* queue to ensure that the module file can be read when the request +* queue is the one for the device storing the module file. +*/ +if (entry->load_module) { + res = entry->load_module (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@@ -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_load_module (struct gendisk *disk,const char *buf, +size_t count) +{ +char elevator_name[ELV_NAME_MAX ]; + +if (!elv_support_iosched (disk->queue)) +return -EOPNOTSUPP ; + +strscpy (elevator_name, buf,sizeof (elevator_name)); + +return request_module ("%s-iosched" ,strstrip (elevator_name)); +} +ssize_t elv_iosched_store (struct gendisk *disk,const char *buf,size_t count) {block/elevator.h +2
-0@@ -148,6 +148,8 @@ extern void elv_unregister(struct elevator_type *);* io scheduler sysfs switching */ ssize_t elv_iosched_show (struct gendisk *disk,char *page); +int elv_iosched_load_module (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 · 1 line
2.46.0
Signature · 4 lines
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Quoted 13 lines · Damien Le Moal
Commit
af2814149883("block: freeze the queue inqueue_attr_store") changedqueue_attr_store()to always freeze a sysfs attribute queue before calling the attributestore()method, to ensure that no IOs are in-flight when an attribute value is being updated.However, this change created a potential deadlock situation for the scheduler queue attribute as changing the queue elevator with
elv_iosched_store()can result in a call torequest_module()if the user requested module is not already registered. If the file of the requested module is stored on the block device of the frozen queue, a deadlock will happen as the read operations triggered byrequest_module()will wait for the queue freeze to end.
Quoted 1 line
[...]
Applied, thanks!
[1/1] block: Prevent deadlocks when switching elevators
commit: 3c031b721c0ee1d6237719a6a9d7487ef757487b
Best regards,
Signature · 1 line
Jens Axboe
Quoted 19 lines · Damien Le Moal
Commit
af2814149883("block: freeze the queue inqueue_attr_store") changedqueue_attr_store()to always freeze a sysfs attribute queue before calling the attributestore()method, to ensure that no IOs are in-flight when an attribute value is being updated.However, this change created a potential deadlock situation for the scheduler queue attribute as changing the queue elevator with
elv_iosched_store()can result in a call torequest_module()if the user requested module is not already registered. If the file of the requested module is stored on the block device of the frozen queue, a deadlock will happen as the read operations triggered byrequest_module()will wait for the queue freeze to end.Solve this issue by introducing the
load_modulemethod in structqueue_sysfs_entry, and to calling this method function inqueue_attr_store()before freezing the attribute queue.
The macro definitionQUEUE_RW_LOAD_MODULE_ENTRY()is added to define a queue sysfs attribute that needs loading a module.
Quoted 5 lines
The definition of the scheduler atrribute is changed to using
QUEUE_RW_LOAD_MODULE_ENTRY(), with the functionelv_iosched_load_module()defined as theload_modulemethod.elv_iosched_store()can then be simplified to remove the call torequest_module().
Hi,
this broke udev rules for loop in 6.11:
Quoted 8 lines
loop1:
/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop1/queue/scheduler}="none", ignoring: No such file or directory
loop0:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop0/queue/scheduler}="none", ignoring: No such file or directory
loop5:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop5/queue/scheduler}="none", ignoring: No such file or directory
loop3:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop3/queue/scheduler}="none", ignoring: No such file or directory
loop2:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop2/queue/scheduler}="none", ignoring: No such file or directory
loop7:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop7/queue/scheduler}="none", ignoring: No such file or directory
loop4:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop4/queue/scheduler}="none", ignoring: No such file or directory
loop6:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop6/queue/scheduler}="none", ignoring: No such file or directory
60-io-scheduler.rules:
Quoted 6 lines
1 # Set optimal IO schedulers for HDD and SSD
2 # Copyright (c) 2021 SUSE LLC
3
4 # ## DO NOT EDIT. ##
5 # To modify the rules, copy this file to /etc/udev/rules.d/60-io-scheduler.rules
6 # and edit the copy.Quoted 8 lines
7 # Please read the section "Tuning I/O performance" in the System Analysis and Tuning Guide
8 # from the SUSE Documentation.
9
10 # --- DO NOT EDIT THIS PART ----
11 SUBSYSTEM!="block", GOTO="scheduler_end"
12 ACTION!="add|change", GOTO="scheduler_end"
13 ENV{DEVTYPE}!="disk", GOTO="scheduler_end"
14 TEST!="%S%p/queue/scheduler", GOTO="scheduler_end"It apparently exists here ^^^.
Quoted 11 lines
15
16 # For dm devices, the relevant events are "change" events, see 10-dm.rules
17 ACTION!="change", KERNEL=="dm-*", GOTO="scheduler_end"
18 # "none" with no brackets means scheduler isn't configurable
19 ATTR{queue/scheduler}=="none", GOTO="scheduler_end"
20 # keep our hands off zoned devices, the kernel auto-configures them
21 ATTR{queue/zoned}!="none", GOTO="scheduler_end"
22 # Enforce "none" for multipath components.
23 ENV{DM_MULTIPATH_DEVICE_PATH}=="1", ATTR{queue/scheduler}="none", GOTO="scheduler_end"
24 # Enforce "none" for loop devices
25 KERNEL=="loop[0-9]*", ATTR{queue/scheduler}="none", GOTO="scheduler_end"But cannot be written to ^^^ because it does not exist. What the heck?
Quoted 21 lines
26
27 # --- EDIT BELOW HERE after copying to /etc/udev/rules.d ---
28
29 # Uncomment these if you want to force virtual devices to use no scheduler
30 # KERNEL=="vd[a-z]*", ATTR{queue/scheduler}="none", GOTO="scheduler_end"
31 # KERNEL=="xvd[a-z]*", ATTR{queue/scheduler}="none", GOTO="scheduler_end"
32
33 # Leave virtual devices untouched
34 KERNEL=="vd[a-z]*", GOTO="scheduler_end"
35 KERNEL=="xvd[a-z]*", GOTO="scheduler_end"
36
37 # 1. BFQ scheduler for single-queue HDD
38 ATTR{queue/rotational}!="0", TEST!="%S%p/mq/1", ATTR{queue/scheduler}="bfq", GOTO="scheduler_end"
39
40 # 2. BFQ scheduler for every HDD, including "real" multiqueue
41 # ATTR{queue/rotational}!="0", ATTR{queue/scheduler}="bfq", GOTO="scheduler_end"
42
43 # 3. For "real" multiqueue devices, the kernel defaults to no IO scheduling
44 # Uncomment this (and select your scheduler) if you need an IO scheduler for them
45 # TEST=="%S%p/mq/1", ATTR{queue/scheduler}="kyber", GOTO="scheduler_end"
46Quoted 8 lines
47 # 4. BFQ scheduler for every device (uncomment if you need ionice or blk-cgroup features)
48 # ATTR{queue/scheduler}="bfq", GOTO="scheduler_end"
49
50 # 5. mq-deadline is the kernel default for devices with just one hardware queue
51 # ATTR{queue/scheduler}="mq-deadline"
52
53 # --- EDIT ABOVE HERE after copying to /etc/udev/rules.d ---
54 LABEL="scheduler_end"Any ideas?
Quoted 108 lines
Reported-by Richard W.M. Jones <[email protected]>Reported-by Jiri Jaburek <[email protected]>Fixes af2814149883 ("block: freeze the queue in queue_attr_store")Signed-off-by Damien Le Moal <[email protected]>--- block/blk-sysfs.c | 22 +++++++++++++++++++++- block/elevator.c | 21 +++++++++++++++------ block/elevator.h | 2 ++ 3 files changed, 38 insertions(+), 7 deletions(-)block/blk-sysfs.c +21
-1@@ -23,6 +23,7 @@struct queue_sysfs_entry {struct attribute attr;ssize_t (*show)(struct gendisk *disk,char *page); +int (*load_module)(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_RW_LOAD_MODULE_ENTRY (_prefix, _name) \ +static struct queue_sysfs_entry _prefix##_entry = { \ + .attr = { .name = _name, .mode = 0644 }, \ + .show = _prefix##_show, \ + .load_module = _prefix##_load_module, \ + .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_RW_LOAD_MODULE_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,17 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,if (!entry->store)return -EIO ; +/* +* If the attribute needs to load a module, do it before freezing the +* queue to ensure that the module file can be read when the request +* queue is the one for the device storing the module file. +*/ +if (entry->load_module) { + res = entry->load_module (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@@ -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_load_module (struct gendisk *disk,const char *buf, +size_t count) +{ +char elevator_name[ELV_NAME_MAX ]; + +if (!elv_support_iosched (disk->queue)) +return -EOPNOTSUPP ; + +strscpy (elevator_name, buf,sizeof (elevator_name)); + +return request_module ("%s-iosched" ,strstrip (elevator_name)); +} +ssize_t elv_iosched_store (struct gendisk *disk,const char *buf,size_t count) {block/elevator.h +0
-0
block/elevator.h extern void elv_unregister(struct elevator_type *); · line 155
Quoted 8 lines
block/elevator.h +2
-0@@ -148,6 +148,8 @@ extern void elv_unregister(struct elevator_type *);* io scheduler sysfs switching */ ssize_t elv_iosched_show (struct gendisk *disk,char *page); +int elv_iosched_load_module (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 *);
thanks,
Signature · 2 lines
js suse labs
Quoted 2 lines · Jiri Slaby
this broke udev rules for loop in 6.11:
Quoted 1 line
loop1:
/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop1/queue/scheduler}="none", ignoring: No such file or directory
(etc.)
Patch here but it's not in mainline yet:
https://lore.kernel.org/linux-block/[email protected]/
cheers
Holger
Jens Axboe
maintainerQuoted 9 lines · Holger Hoffstätte
Quoted 2 lines · Jiri Slaby
this broke udev rules for loop in 6.11:
> loop1:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop1/queue/scheduler}="none", ignoring: No such file or directory(etc.)
Patch here but it's not in mainline yet:
It'll go upstream later this week.
Signature · 1 line
Jens Axboe
Quoted 5 lines · Jens Axboe
Quoted 4 lines · Holger Hoffstätte
Quoted 2 lines · Jiri Slaby
this broke udev rules for loop in 6.11:
> loop1:/usr/lib/udev/rules.d/60-io-scheduler.rules:25Failed to write ATTR{/sys/devices/virtual/block/loop1/queue/scheduler}="none", ignoring: No such file or directory
Quoted 7 lines
Quoted 5 lines
(etc.)
Patch here but it's not in mainline yet:
It'll go upstream later this week.
FWIW works for me.
thanks,
Signature · 1 line
js