Re: [PATCH v13 05/18] scsi: core: Introduce a mechanism for reordering requests in the error handler

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

On 10/20/23 02:53, Bart Van Assche wrote:
> 
> On 10/18/23 17:24, Damien Le Moal wrote:
>> On 10/19/23 02:54, Bart Van Assche wrote:
>>> +void scsi_call_prepare_resubmit(struct list_head *done_q)
>>> +{
>>> +	struct scsi_cmnd *scmd, *next;
>>> +
>>> +	if (!scsi_needs_preparation(done_q))
>>> +		return;
>>
>> This function will always go through the list of commands in done_q. That could
>> hurt performance for scsi hosts that do not need this prepare resubmit, which I
>> think is UFS only for now. So can't we just add a flag or something to avoid that ?
> 
> Hi Damien,
> 
> The SCSI error handler is only invoked after an RCU grace period has 
> expired. The time spent in scsi_needs_preparation() is negligible
> compared to an RCU grace period, especially if the
> .eh_needs_prepare_resubmit pointers are NULL.

I was thinking in the context of the scsi disk driver, which is the most widely
used scsi driver and does have eh_needs_prepare_resubmit set.

And I now recall that we have already discussed this when I suggested passing
the scsi_host as argument here to avoid that loop for hosts that do not need to
do that reordering (that is, everything but ufs hosts). You did mention that
this is not easily feasible if I remember correctly.

That is really too bad. It would be nice to avoid this loop when it is not
needed. But given that this is the error path, may be that is OK. I'll stay
neutral on this one for now. I need to run some performance tests. FYI, libata
triggers scsi_eh to process the completion of commands with CDL set and sense
data available, to determine if the commands were aborted or not. This loop may
be costly for that case, not sure. Will check.

-- 
Damien Le Moal
Western Digital Research