Re: [PATCH 04/15] block: prevent elevator switch during updating nr_hw_queues

[Christoph Hellwig <hch@lst.de>]

On Thu, Apr 10, 2025 at 09:30:16PM +0800, Ming Lei wrote:
> updating nr_hw_queues is usually used for error handling code, when it

Capitalize the first word of each sentence, please.

> doesn't make sense to allow blk-mq elevator switching, since nr_hw_queues
> may change, and elevator tags depends on nr_hw_queues.

I don't think it's really updated from error handling

 - nbd does it when starting a device
 - nullb can do it through debugfs
 - xen-blkfront does it when resuming from a suspend
 - nvme does it when resetting a controller.  While error handling
   can escalate to it¸ it's basically probing and re-probing code

> Prevent elevator switch during updating nr_hw_queues by setting flag of
> BLK_MQ_F_UPDATE_HW_QUEUES, and use srcu to fail elevator switch during
> the period. Here elevator switch code is srcu reader of nr_hw_queues,
> and blk_mq_update_nr_hw_queues() is the writer.

That being said as we generally are in a setup path I think the general
idea is fine.  No devices should be life yet at this point and thus
no udev rules changing the scheduler should run yet.

> This way avoids lot of trouble.

Can you spell that out a bit?

> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Closes: https://lore.kernel.org/linux-block/mz4t4tlwiqjijw3zvqnjb7ovvvaegkqganegmmlc567tt5xj67@xal5ro544cnc/

Are we using Closes for bug reports now?  I haven't really seen that
anywhere.

>  out_cleanup_srcu:
>  	if (set->flags & BLK_MQ_F_BLOCKING)
>  		cleanup_srcu_struct(set->srcu);
> @@ -5081,7 +5087,18 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
>  void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
>  {
>  	mutex_lock(&set->tag_list_lock);
> +	/*
> +	 * Mark us in updating nr_hw_queues for preventing switching
> +	 * elevator
>
> +	 *
> +	 * Elevator switch code can _not_ acquire ->tag_list_lock

Please add a . at the end of a sentences.  Also this should probably
be something like "Mark us as in.." but I'll leave more nitpicking
to the native speakers.

>  	struct request_queue *q = disk->queue;
> +	struct blk_mq_tag_set *set = q->tag_set;
>  
>  	/*
>  	 * If the attribute needs to load a module, do it before freezing the
> @@ -732,6 +733,13 @@ ssize_t elv_iosched_store(struct gendisk *disk, const char *buf,
>  
>  	elv_iosched_load_module(name);
>  
> +	idx = srcu_read_lock(&set->update_nr_hwq_srcu);
> +
> +	if (set->flags & BLK_MQ_F_UPDATE_HW_QUEUES) {

What provides atomicity for field modifications vs reading of set->flags?
i.e. does this need to switch using test/set_bit?

> +	struct srcu_struct	update_nr_hwq_srcu;
>  };
>  
>  /**
> @@ -681,7 +682,14 @@ enum {
>  	 */
>  	BLK_MQ_F_NO_SCHED_BY_DEFAULT	= 1 << 6,
>  
> -	BLK_MQ_F_MAX = 1 << 7,
> +	/*
> +	 * True when updating nr_hw_queues is in-progress
> +	 *
> +	 * tag_set only flag, not usable for hctx
> +	 */
> +	BLK_MQ_F_UPDATE_HW_QUEUES	= 1 << 7,
> +
> +	BLK_MQ_F_MAX = 1 << 8,

Also mixing internal state with driver provided flags is always
a bad idea.  So this should probably be a new state field in the
tag_set and not reuse flags.