Re: [PATCH] scsi: avoid to send scsi command with ->queue_limits lock held

[Damien Le Moal <dlemoal@kernel.org>]

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