On 1/4/25 20:28, Nilay Shroff wrote: > > > On 1/4/25 12:47 PM, Damien Le Moal wrote: >> On 12/31/24 13:22, Ming Lei wrote: >>> Block request queue is often frozen before acquiring the queue >>> ->limits_lock. >> >> "often" is rather vague. What cases are we talking about here beside the block >> layer sysfs ->store() operations ? Fixing these is easy and does not need this >> change. > Other than sysfs ->store(), I see there're few call sites in NVMe driver (nvme_update_ > ns_info_block(), nvme_update_ns_info_generic(), nvme_update_ns_info() etc.) which first > freezes queue and then acquire limits lock. Also there's one call site (__blk_mq_update_ > nr_hw_queues) in block layer which does the same. I sent a patch to address the block layer sysfs. Starting looking into these other calls with the reversed locking. >> Furthermore, this change almost feels like a layering violation as it replicates >> most of the queue limits structure inside sd. This introducing a strong >> dependency to the block layer internals which we should avoid. >> > In theory, we don't need to hold limits lock while sd_revalidate_disk() reads various > limits from hardware. However can't we make this one exception (till we find a better > solution) for sd_revalidate_disk() and allow it to acquire limits lock while blk-mq > request is processed? Sure, but issuing IOs to probe a device with the limits lock held is also *not* an issue. All that can cause is a slight delay for user initiated changes through sysfs. The fundamental issue is not issuing IOs with the limits lock held, it is the inconsistent ordering of the calls to blk_mq_freeze_queue and queue_limits_start_update(). My take on this is that we should always freeze the queue only once the limits lock is held, with the queue freeze only around queue_limits_commit_update(). If the consensus is to do the reverse, that's fine with me as well, but probably will be more work to change (as this large patch tends to indicate). -- Damien Le Moal Western Digital Research
