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

Bart Van Assche
2023-08-17T14:26:45Z

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.

Thanks,

Bart.