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
