On 12/31/24 04:59, Damien Le Moal wrote: > On 2024/12/30 18:02, Ming Lei wrote: >>> 1. Callers which need acquiring limits-lock while starting the update; and freezing >>> queue only when committing the update: >>> - sd_revalidate_disk >> >> sd_revalidate_disk() should be the most strange one, in which >> passthrough io command is required, so dependency on queue freeze lock >> can't be added, such as, q->limits_lock >> >> Actually the current queue limits structure aren't well-organized, otherwise >> limit lock isn't needed for reading queue limits from hardware, since >> sd_revalidate_disk() just overwrites partial limits. Or it can be >> done by refactoring sd_revalidate_disk(). However, the change might >> be a little big, and I guess that is the reason why Damien don't like >> it. > > That was not the reason, but yes, modifying sd_revalidate_disk() is not without > risks of introducing regressions. The reason I proposed to simply move the queue > freeze around or inside queue_limits_commit_update() is that: > > 1) It is the right thing to do as that is the only place where it is actually > needed to avoid losing concurrent limits changes. > > 2) It clarifies the locking order between queue freeze and the limits lock. > > 3) The current issues should mostly all be solved with some refactoring of the > ->store() calls in blk-sysfs.c, resolving the current ABBA deadlocks between > queue freeze and limits lock. > > With that, we should be able to fix the issue for all block drivers with changes > to the block layer sysfs code only. But... I have not looked into the details of > all limits commit calls in all block drivers. So there may be some bad apples in > there that will also need some tweaking. Yes, I think, we have places other than blk-sysfs, like nvme, where we need fixing. > >>> - nvme_init_identify >>> - loop_clear_limits >>> - few more... >>> >>> 2. Callers which need both freezing the queue and acquiring limits-lock while starting >>> the update: >>> - nvme_update_ns_info_block >>> - nvme_update_ns_info_generic >>> - few more... > > The queue freeze should not be necessary anywhere when starting the update. The > queue freeze is only needed when applying the limits so that IOs that are in > flight are not affected by the limits change while still being processed. Hmm, but as mentioned for nvme limits update this is not always true. So this need to be tweaked. >>> >>> 3. Callers which neither need acquiring limits-lock nor require freezing queue as for >>> these set of callers in the call stack limits-lock is already acquired and queue is >>> already frozen: >>> - __blk_mq_update_nr_hw_queues >>> - queue_xxx_store and helpers >> >> I think it isn't correct. >> >> The queue limits are applied on fast IO path, in theory anywhere >> updating q->limits need to drain IOs in submission path at least >> after gendisk is added. > > Yes ! > OK, I think it'd be tricky to fix __blk_mq_update_nr_hw_queues and queue_xxx_store with the current proposal of acquiring limits lock followed by queue freeze while committing limit update. Thanks, --Nilay
