On Fri, 2012-04-13 at 16:37 -0700, Dan Williams wrote: > Rapid ata hotplug on a libsas controller results in cases where libsas > is waiting indefinitely on eh to perform an ata probe. > > A race exists between scsi_schedule_eh() and scsi_restart_operations() > in the case when scsi_restart_operations() issues i/o to other devices > in the sas domain. When this happens the host state transitions from > SHOST_RECOVERY (set by scsi_schedule_eh) back to SHOST_RUNNING and > ->host_busy is non-zero so we put the eh thread to sleep even though > ->host_eh_scheduled is active. > > Before putting the error handler to sleep we need to check if the > host_state needs to return to SHOST_RECOVERY for another trip through > eh. > > Cc: Tejun Heo <tj@xxxxxxxxxx> > Reported-by: Tom Jackson <thomas.p.jackson@xxxxxxxxx> > Tested-by: Tom Jackson <thomas.p.jackson@xxxxxxxxx> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> > --- > drivers/scsi/scsi_error.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 2cfcbff..0945d47 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1687,6 +1687,20 @@ static void scsi_restart_operations(struct Scsi_Host *shost) > * requests are started. > */ > scsi_run_host_queues(shost); > + > + /* > + * if eh is active and host_eh_scheduled is pending we need to re-run > + * recovery. we do this check after scsi_run_host_queues() to allow > + * everything pent up since the last eh run a chance to make forward > + * progress before we sync again. Either we'll immediately re-run > + * recovery or scsi_device_unbusy() will wake us again when these > + * pending commands complete. > + */ > + spin_lock_irqsave(shost->host_lock, flags); > + if (shost->host_eh_scheduled) > + if (scsi_host_set_state(shost, SHOST_RECOVERY)) > + WARN_ON(scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY)); > + spin_unlock_irqrestore(shost->host_lock, flags); This doesn't really look to be the way to fix the race, because we'll start up the host again before closing it down. Isn't the correct way to put if (shost->host_eh_scheduled) continue; into the scsi_error_handler() loop just *before* scsi_restart_operations()? James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
