Re: [PATCH 7/7] scsi_error: streamline scsi_eh_bus_device_reset()

[Hannes Reinecke <hare@suse.de>]

On 10/3/23 19:45, Bart Van Assche wrote:
> On 10/2/23 08:59, Hannes Reinecke wrote:
>> Streamline to use a similar code flow as the other reset functions.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/scsi_error.c | 26 +++++++++++---------------
>>   1 file changed, 11 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 21d84940c9cb..81b38f5da3b6 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -1581,6 +1581,7 @@ static int scsi_eh_bus_device_reset(struct 
>> Scsi_Host *shost,
>>   {
>>       struct scsi_cmnd *scmd, *bdr_scmd, *next;
>>       struct scsi_device *sdev;
>> +    LIST_HEAD(check_list);
>>       enum scsi_disposition rtn;
>>       shost_for_each_device(sdev, shost) {
>> @@ -1606,27 +1607,22 @@ static int scsi_eh_bus_device_reset(struct 
>> Scsi_Host *shost,
>>               sdev_printk(KERN_INFO, sdev,
>>                        "%s: Sending BDR\n", current->comm));
>>           rtn = scsi_try_bus_device_reset(sdev);
>> -        if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
>> -            if (!scsi_device_online(sdev) ||
>> -                rtn == FAST_IO_FAIL ||
>> -                !scsi_eh_tur(bdr_scmd)) {
>> -                list_for_each_entry_safe(scmd, next,
>> -                             work_q, eh_entry) {
>> -                    if (scmd->device == sdev &&
>> -                        scsi_eh_action(scmd, rtn) != FAILED)
>> -                        __scsi_eh_finish_cmd(scmd,
>> -                                     done_q,
>> -                                     DID_RESET);
>> -                }
>> -            }
>> -        } else {
>> +        if (rtn != SUCCESS && rtn != FAST_IO_FAIL)
>>               SCSI_LOG_ERROR_RECOVERY(3,
>>                   sdev_printk(KERN_INFO, sdev,
>>                           "%s: BDR failed\n", current->comm));
>> +        list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
>> +            if (scmd->device != sdev)
>> +                continue;
>> +            if (rtn == SUCCESS)
>> +                list_move_tail(&scmd->eh_entry, &check_list);
>> +            else if (rtn == FAST_IO_FAIL)
>> +                __scsi_eh_finish_cmd(scmd, done_q,
>> +                             DID_TRANSPORT_DISRUPTED);
>>           }
>>       }
>> -    return list_empty(work_q);
>> +    return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
>>   }
> 
> I think the description of this patch is too brief. The following 
> information is missing from the patch description:
> - Why the scsi_device_online() and scsi_eh_tur() checks have been left
>    out.
> - Why DID_RESET has been changed into DID_TRANSPORT_DISRUPTED.
> - Why the list_move_tail() call has been introduced.
> - Why the scsi_eh_test_devices() call has been introduced.
>
Okay, will be doing so.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman