Re: [PATCH] block: elevator: avoid to load iosched module from this disk

["Richard W.M. Jones" <rjones@redhat.com>]

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@redhat.com>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: Jiri Jaburek <jjaburek@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

I'd suggest also:

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=219166
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Reported-by: Jiri Jaburek <jjaburek@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>

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@lst.de>
  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