On 8/14/23 11:18, Bart Van Assche wrote:
> On 8/13/23 18:19, Damien Le Moal wrote:
>> On 8/12/23 06:35, Bart Van Assche wrote:
>>> Make the error handler call .eh_prepare_resubmit() before resubmitting
>>
>> This reads like the eh_prepare_resubmit callback already exists. But you are
>> adding it. So you should state that.
>
> Hi Damien,
>
> I will rephrase the patch description.
>
>>> +++ b/drivers/scsi/Makefile.kunit
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_SCSI_ERROR_TEST) += scsi_error_test.o
>>
>> All the above kunit changes (and the test changes below) seem unrelated to what
>> the commit message describes. Should these be split into a different patch ?
>
> Some people insist on including unit tests in the same patch as
> the patch that introduces the code that is being tested. I can
> move the unit test into a separate patch if that is preferred.
OK. I will let Martin decide that :)
But at the very least, may be mention in the commit message that you also add
the unit tests associated with the change ?
>
>>> + /*
>>> + * Call .eh_prepare_resubmit for each range of commands with identical
>>> + * ULD driver pointer.
>>> + */
>>> + list_for_each_entry_safe(scmd, next, done_q, eh_entry) {
>>> + struct scsi_driver *uld = scsi_cmd_to_driver(scmd);
>>> + struct list_head *prev, uld_cmd_list;
>>> +
>>> + while (&next->eh_entry != done_q &&
>>> + scsi_cmd_to_driver(next) == uld)
>>> + next = list_next_entry(next, eh_entry);
>>> + if (!uld->eh_prepare_resubmit)
>>> + continue;
>>> + prev = scmd->eh_entry.prev;
>>> + list_cut_position(&uld_cmd_list, prev, next->eh_entry.prev);
>>> + uld->eh_prepare_resubmit(&uld_cmd_list);
>>
>> Is it guaranteed that all uld implement eh_prepare_resubmit ?
>
> That is not guaranteed. Hence the if (!uld->eh_prepare_resubmit)
> test in the above loop.
Oops. Missed that one !
Another remark: we'll need this sorting only for devices that are zoned and do
not need zone write locking. For other cases (most cases in fact), we don't and
this could have some performance impact (e.g. fast RAID systems). Is there a way
to have this eh_prepare_resubmit to do nothing for regular devices to avoid that ?
>
> Thanks,
>
> Bart.
--
Damien Le Moal
Western Digital Research
