On 03/13/2017 02:37 PM, Mauricio Faria de Oliveira wrote:
> Hannes,
>
> On 03/01/2017 06:15 AM, Hannes Reinecke wrote:
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index f2cafae..cec439c 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -58,6 +58,7 @@
>> static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
>> static int scsi_try_to_abort_cmd(struct scsi_host_template *,
>> struct scsi_cmnd *);
>> +static int scsi_eh_reset(struct scsi_cmnd *scmd);
>>
>> /* called with shost->host_lock held */
>> void scsi_eh_wakeup(struct Scsi_Host *shost)
>> @@ -249,6 +250,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int
>> eh_flag)
>> if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
>> eh_flag &= ~SCSI_EH_CANCEL_CMD;
>> scmd->eh_eflags |= eh_flag;
>> + scsi_eh_reset(scmd);
>> list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
>> shost->host_failed++;
>> scsi_eh_wakeup(shost);
>> @@ -1107,7 +1109,19 @@ static int scsi_eh_action(struct scsi_cmnd
>> *scmd, int rtn)
>> if (!blk_rq_is_passthrough(scmd->request)) {
>> struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
>> if (sdrv->eh_action)
>> - rtn = sdrv->eh_action(scmd, rtn);
>> + rtn = sdrv->eh_action(scmd, rtn, false);
>> + }
>> + return rtn;
>> +}
>> +
>> +static int scsi_eh_reset(struct scsi_cmnd *scmd)
>> +{
>> + int rtn = SUCCESS;
>> +
>> + if (!blk_rq_is_passthrough(scmd->request)) {
>> + struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
>> + if (sdrv->eh_action)
>> + rtn = sdrv->eh_action(scmd, rtn, true);
>> }
>> return rtn;
>> }
>
> Apparently we can de-duplicate code in scsi_eh_reset()/scsi_eh_action(),
> and change less of sd_eh_action() (i.e., no new parameter).
>
> Something like this. Thoughts?
>
>
> - Deduplicate code in scsi_eh_reset() and scsi_eh_action()
> - A call to scsi_eh_reset() with reset == false calls into eh_action()
>
> - A call to scsi_eh_reset() with reset == true returns early,
> (as happens with/if sd_eh_action() is called in your patch)
>
> - A call to scsi_eh_reset() in scsi_eh_scmd_add() uses SUCCESS just
> for consistency with scsi_eh_reset() in your patch, as the return
> value is ignored in it.
>
> - Less changes to sd_eh_action()
> - Removed one chunk in sd_eh_action() ('reset gate variable')
> - No more parameter changes in sd_eh_action() (no 'reset' parameter)
>
> - Removed forward declaration of scsi_eh_reset(), as already suggested
>
>
>
> static int scsi_eh_reset(struct scsi_cmnd *scmd, int eh_disp, bool reset)
> {
> int rtn = eh_disp;
>
> if (!blk_rq_is_passthrough(scmd->request)) {
> struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
> if (sdrv->eh_action) {
> if (reset) {
> struct scsi_disk *sdkp =
> scsi_disk(scmd->request->rq_disk);
>
> /* New SCSI EH run, reset gate variable */
> sdkp->medium_access_reset = 0;
> return rtn;
> }
> rtn = sdrv->eh_action(scmd, rtn);
> }
> }
> return rtn;
> }
>
Nope.
This is assuming that we're always running on a scsi_disk, and that
scsi_disk is the only one implementing 'eh_action'.
Neither of which is necessarily true.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
