Re: [PATCH v9 04/17] scsi: core: Call .eh_prepare_resubmit() before resubmitting

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

On 2023/08/17 23:26, Bart Van Assche wrote:
> On 8/17/23 04:10, Damien Le Moal wrote:
>> On 8/17/23 04:53, Bart Van Assche wrote:
>>> +/*
>>> + * Returns true if the commands in @done_q should be sorted in LBA order
>>> + * before being resubmitted.
>>> + */
>>> +static bool scsi_needs_sorting(struct list_head *done_q)
>>> +{
>>> +	struct scsi_cmnd *scmd;
>>> +
>>> +	list_for_each_entry(scmd, done_q, eh_entry) {
>>> +		struct request *rq = scsi_cmd_to_rq(scmd);
>>> +
>>> +		if (!rq->q->limits.use_zone_write_lock &&
>>> +		    blk_rq_is_seq_zoned_write(rq))
>>> +			return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +/*
>>> + * Comparison function that allows to sort SCSI commands by ULD driver.
>>> + */
>>> +static int scsi_cmp_uld(void *priv, const struct list_head *_a,
>>> +			const struct list_head *_b)
>>> +{
>>> +	struct scsi_cmnd *a = list_entry(_a, typeof(*a), eh_entry);
>>> +	struct scsi_cmnd *b = list_entry(_b, typeof(*b), eh_entry);
>>> +
>>> +	/* See also the comment above the list_sort() definition. */
>>> +	return scsi_cmd_to_driver(a) > scsi_cmd_to_driver(b);
>>> +}
>>> +
>>> +void scsi_call_prepare_resubmit(struct list_head *done_q)
>>> +{
>>> +	struct scsi_cmnd *scmd, *next;
>>> +
>>> +	if (!scsi_needs_sorting(done_q))
>>> +		return;
>>
>> This is strange. The eh_prepare_resubmit callback is generic and its name does
>> not indicate anything related to sorting by LBAs. So this check would prevent
>> other actions not related to sorting by LBA. This should go away.
>>
>> In patch 6, based on the device characteristics, the sd driver should decides
>> if it needs to set .eh_prepare_resubmit or not.
>>
>> And ideally, if all ULDs have eh_prepare_resubmit == NULL, this function should
>> return here before going through the list of commands to resubmit. Given that
>> this list should generally be small, going through it and doing nothing should
>> be OK though...
> 
> I can add a eh_prepare_resubmit == NULL check early in 
> scsi_call_prepare_resubmit(). Regarding the code inside 
> scsi_needs_sorting(), how about moving that code into an additional 
> callback function, e.g. eh_needs_prepare_resubmit? Setting 
> .eh_prepare_resubmit depending on the zone model would prevent 
> constification of struct scsi_driver.

Sounds OK.

> 
> Thanks,
> 
> Bart.

-- 
Damien Le Moal
Western Digital Research