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. >> - 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. >> >> 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 ! -- Damien Le Moal Western Digital Research
