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

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

On Mon, Jan 06, 2025 at 10:30:10AM +0900, Damien Le Moal wrote:
> On 1/6/25 10:02 AM, Ming Lei wrote:
> > On Sat, Jan 04, 2025 at 04:17:47PM +0900, 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.
> > 
> > Is it really necessary to make freeze lock to depend on ->limits_lock?
> 
> Yes, because you do not want to have requests in-flight when applying new limits.

Thinking of further, actually almost all soft update in ->store()
needn't to freeze queue:

- all are scalar update 

- we can guarantee the new value is correct by validating first, so both
new val and old val are correct from device viewpoint

- the freeze is added recently in v6.11 af2814149883 ("block: freeze the queue
in queue_attr_store") for addressing nothing

Not mention sd_revalidate_disk() can be called in sd_open() without queue
freeze.

But update from hardware need queue to be frozen, or doing that before
disk is up.

My idea is to try to cut the lock chain, and your approach is to try to
order everything. From lock dependency viewpoint, it is simpler to
avoid the dependency from beginning because it is too complicated to
order everything.

> 
> > 
> > sd_revalidate_disk() is really one special case, so I think this patch
> > does correct thing.
> > 
> >>
> >> 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.
> > 
> > No.
> > 
> > block layer is common library, which is storage abstraction, so it is
> > pretty reasonable for storage drivers to depend block layer. You can
> > look at it from another way, if any related queue limits change, the
> > current storage driver need corresponding change too, with or without
> > this change.
> 
> Of course block device driver layers like SCSI depend on the block layer. But
> that dependency is at a high level API/function level. My concern is that your
> patch mimics too much the block layer implementation internals instead of
> relying on a high level API like we do now.

Here block layer API isn't involved, since block layer doesn't or can't
provide API to update single field of queue_limits.

Also the shadow sd_limits can be thought as one scsi's own hardware property
abstract, and its name just follows block layer queue's limits for the
sake of simplicity.

Long term, block layer may do similar categorization for queue_limits
according to function, then scsi disk's shadow structure can be removed
if scsi doesn't have special requirement.


Thanks,
Ming