Hello Hannes,
On Wed, Mar 01, 2017 at 10:15:19AM +0100, Hannes Reinecke wrote:
> scsi_eh_scmd_add() currently only will fail if no
> error handler thread is started (which will never be the
> case) or if the state machine encounters an illegal transition.
>
> But if we're encountering an invalid state transition
> chances is we cannot fixup things with the error handler.
> So better add a WARN_ON for illegal host states and
> make scsi_dh_scmd_add() a void function.
>
> Signed-off-by: Hannes Reinecke <hare@xxxxxxx>
> Reviewed-by: Johannes Thumshirn <jthumshirn@xxxxxxx>
> ---
> drivers/scsi/scsi_error.c | 41 +++++++++++++----------------------------
> drivers/scsi/scsi_lib.c | 4 ++--
> drivers/scsi/scsi_priv.h | 2 +-
> 3 files changed, 16 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 4613aa1..802a081 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -163,13 +163,7 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
> }
> }
>
> - if (!scsi_eh_scmd_add(scmd, 0)) {
> - SCSI_LOG_ERROR_RECOVERY(3,
> - scmd_printk(KERN_WARNING, scmd,
> - "terminate aborted command\n"));
> - set_host_byte(scmd, DID_TIME_OUT);
> - scsi_finish_command(scmd);
> - }
> + scsi_eh_scmd_add(scmd, 0);
> }
>
> /**
> @@ -224,28 +218,23 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
> * scsi_eh_scmd_add - add scsi cmd to error handling.
> * @scmd: scmd to run eh on.
> * @eh_flag: optional SCSI_EH flag.
> - *
> - * Return value:
> - * 0 on failure.
> */
> -int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
> +void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
> {
> struct Scsi_Host *shost = scmd->device->host;
> unsigned long flags;
> - int ret = 0;
> + int ret;
>
> - if (!shost->ehandler)
> - return 0;
> + WARN_ON_ONCE(!shost->ehandler);
>
So I saw that you already changed stuff here after Bart reviewed it. Do
you think it would be useful to make the warning per-host-instance,
rather than per-linux-instance?
Like, if for some reason the EH thread for one SCSI host dies - however
that might happen - we would get an individual warning in the klog for
this one host and could maybe even save the setup by
disabling/re-enabling the whole host - effectively removing the host and
adding a new one, plus a new EH thread for it.
With this WARN_ON_ONCE we end up in a broken setup w/o any information
what exactly broke. Previously we would get at least a SCSI-logging
message. Which would also help with analysis of such bugs - correlate
data etc.
Beste Grüße / Best regards,
- Benjamin Block
>
> spin_lock_irqsave(shost->host_lock, flags);
> - if (scsi_host_set_state(shost, SHOST_RECOVERY))
> - if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
> - goto out_unlock;
> -
> + if (scsi_host_set_state(shost, SHOST_RECOVERY)) {
> + ret = scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY);
> + WARN_ON_ONCE(ret);
> + }
> if (shost->eh_deadline != -1 && !shost->last_reset)
> shost->last_reset = jiffies;
>
> - ret = 1;
> if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
> eh_flag &= ~SCSI_EH_CANCEL_CMD;
> scmd->eh_eflags |= eh_flag;
> @@ -253,9 +242,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
> list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
> shost->host_failed++;
> scsi_eh_wakeup(shost);
> - out_unlock:
> spin_unlock_irqrestore(shost->host_lock, flags);
> - return ret;
> }
>
> /**
> @@ -284,13 +271,11 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
> rtn = host->hostt->eh_timed_out(scmd);
>
> if (rtn == BLK_EH_NOT_HANDLED) {
> - if (!host->hostt->no_async_abort &&
> - scsi_abort_command(scmd) == SUCCESS)
> - return BLK_EH_NOT_HANDLED;
> -
> - set_host_byte(scmd, DID_TIME_OUT);
> - if (!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))
> - rtn = BLK_EH_HANDLED;
> + if (host->hostt->no_async_abort ||
> + scsi_abort_command(scmd) != SUCCESS) {
> + set_host_byte(scmd, DID_TIME_OUT);
> + scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
> + }
> }
>
> return rtn;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index f5e45a2..0735a46 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1593,8 +1593,8 @@ static void scsi_softirq_done(struct request *rq)
> scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
> break;
> default:
> - if (!scsi_eh_scmd_add(cmd, 0))
> - scsi_finish_command(cmd);
> + scsi_eh_scmd_add(cmd, 0);
> + break;
> }
> }
>
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 99bfc98..5be6cbf6 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -72,7 +72,7 @@ extern int scsi_dev_info_list_add_keyed(int compatible, char *vendor,
> extern int scsi_error_handler(void *host);
> extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
> extern void scsi_eh_wakeup(struct Scsi_Host *shost);
> -extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);
> +extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
> void scsi_eh_ready_devs(struct Scsi_Host *shost,
> struct list_head *work_q,
> struct list_head *done_q);
> --
> 1.8.5.6
>
--
Linux on z Systems Development / IBM Systems & Technology Group
IBM Deutschland Research & Development GmbH
Vorsitz. AufsR.: Martina Koederitz / Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
