Re: [PATCH 10/12] scsi: fix eh wakeup (scsi_schedule_eh vs scsi_restart_operations)

[Dan Williams <dan.j.williams@intel.com>]

On Sat, Apr 21, 2012 at 5:22 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> 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@kernel.org>
>> Reported-by: Tom Jackson <thomas.p.jackson@intel.com>
>> Tested-by: Tom Jackson <thomas.p.jackson@intel.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  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.

That's part of the intent.  Any command that has been waiting to be
restarted has been potentially waiting up to a minute for recovery to
complete, so the idea is that we trickle the blocked commands through
and then come back for another pass.  This is close to what "late"
callers sto csi_schedule_eh() experience anyways.

> Isn't the correct way to put
>
> if (shost->host_eh_scheduled)
>        continue;
>
> into the scsi_error_handler() loop just *before*
> scsi_restart_operations()?

I think that only narrows the window...  The hole is
scsi_schedule_eh() called anytime after the last run completes and
before the change of the host state, so host_eh_scheduled can go
active immediately after that check right?

We could move the check into scsi_restart_operations() under the lock
before we change the state back to running, but I figure if we got
that far just let the queue run again.

--
Dan