| Message ID | 1449127063-94512-3-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, 2015-12-03 at 08:17 +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@suse.de> > --- > drivers/scsi/scsi_error.c | 39 +++++++++++++-------------------------- > drivers/scsi/scsi_lib.c | 4 ++-- > drivers/scsi/scsi_priv.h | 2 +- > 3 files changed, 16 insertions(+), 29 deletions(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 984ddcb..deb35737 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -162,13 +162,7 @@ scmd_eh_abort_handler(struct work_struct *work) > } > } > > - 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,37 +218,32 @@ scsi_abort_command(struct scsi_cmnd *scmd) > * 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; > > - if (!shost->ehandler) > - return 0; > + WARN_ON(!shost->ehandler); > > spin_lock_irqsave(shost->host_lock, flags); > + WARN_ON(shost->shost_state != SHOST_RUNNING && > + shost->shost_state != SHOST_CANCEL && > + shost->shost_state != SHOST_RECOVERY && > + shost->shost_state != SHOST_CANCEL_RECOVERY); > if (scsi_host_set_state(shost, SHOST_RECOVERY)) > - if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) > - goto out_unlock; > + scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY); > > 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; > 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; > } > > /** > @@ -285,13 +274,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 fa6b2c4..2dd7d0a 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -1647,8 +1647,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 27b4d0a..8c26823 100644 > --- a/drivers/scsi/scsi_priv.h > +++ b/drivers/scsi/scsi_priv.h > @@ -71,7 +71,7 @@ extern enum blk_eh_timer_return scsi_times_out(struct > request *req); > 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); Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 03, 2015 at 08:17:40AM +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. The ehandler parts looks trivially correct, but I'm a little worried about the state transition. The states that we can't transition from are: SHOST_CREATED, SHOST_DEL and SHOST_DEL_RECOVERY. We initialize the state to SHOST_CREATED in scsi_host_alloc and transition away from it in scsi_add_host_with_dma, so that's a true "should be impossible" condition. We transition to SHOST_DEL or SHOST_DEL_RECOVERY in scsi_remove_host and the host remains in it until the final reference is dropped. Given that we wait for all pending I/O in blk_cleanup_queue called from __scsi_remove_device this should be fine as well. So: Reviewed-by: Christoph Hellwig <hch@lst.de> But preferably with an updated changelog that explains things better. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 984ddcb..deb35737 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -162,13 +162,7 @@ scmd_eh_abort_handler(struct work_struct *work) } } - 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,37 +218,32 @@ scsi_abort_command(struct scsi_cmnd *scmd) * 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; - if (!shost->ehandler) - return 0; + WARN_ON(!shost->ehandler); spin_lock_irqsave(shost->host_lock, flags); + WARN_ON(shost->shost_state != SHOST_RUNNING && + shost->shost_state != SHOST_CANCEL && + shost->shost_state != SHOST_RECOVERY && + shost->shost_state != SHOST_CANCEL_RECOVERY); if (scsi_host_set_state(shost, SHOST_RECOVERY)) - if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)) - goto out_unlock; + scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY); 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; 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; } /** @@ -285,13 +274,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 fa6b2c4..2dd7d0a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1647,8 +1647,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 27b4d0a..8c26823 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -71,7 +71,7 @@ extern enum blk_eh_timer_return scsi_times_out(struct request *req); 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);
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@suse.de> --- drivers/scsi/scsi_error.c | 39 +++++++++++++-------------------------- drivers/scsi/scsi_lib.c | 4 ++-- drivers/scsi/scsi_priv.h | 2 +- 3 files changed, 16 insertions(+), 29 deletions(-)