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

[Ming Lei <ming.lei@redhat.com>]

On Thu, Apr 10, 2025 at 04:36:22PM +0200, Christoph Hellwig wrote:
> 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

NVMe does use it in error handling. I can remove error handling words, but
the trouble doesn't change.

> 
>  - 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

reset is part of error handling.

> 
> > 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?

Closes: https://lore.kernel.org/linux-block/mz4t4tlwiqjijw3zvqnjb7ovvvaegkqganegmmlc567tt5xj67@xal5ro544cnc/

> 
> > 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.

The blktests block/039 isn't merged yet, and the patch is posted recently.

kernel panic and kasan is triggered in this test.

> 
> >  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.

OK.

> 
> >  	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?

WRITE is serialized via tag set lock with synchronize_srcu().

READ is covered by srcu read lock.

It is typical RCU usage, one writer vs. multiple writer.

> 
> > +	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.
 
That is fine, but BLK_MQ_F_TAG_QUEUE_SHARED is used in this way too.

thanks, 
Ming