On Sat, Sep 07, 2024 at 09:43:31AM +0800, Ming Lei wrote: > 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 <rjones@xxxxxxxxxx> > Cc: Jeff Moyer <jmoyer@xxxxxxxxxx> > Cc: Jiri Jaburek <jjaburek@xxxxxxxxxx> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx> I'd suggest also: Bug: https://bugzilla.kernel.org/show_bug.cgi?id=219166 Reported-by: Richard W.M. Jones <rjones@xxxxxxxxxx> Reported-by: Jiri Jaburek <jjaburek@xxxxxxxxxx> Tested-by: Richard W.M. Jones <rjones@xxxxxxxxxx> 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 <hch@xxxxxx> 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. > --- > block/elevator.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/block/elevator.c b/block/elevator.c > index f13d552a32c8..2b0432f4ac33 100644 > --- a/block/elevator.c > +++ b/block/elevator.c > @@ -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) > -- > 2.46.0 -- 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
