Re: [PATCH] scsi: fix hang in scsi error handling

[Hannes Reinecke <hare@suse.de>]

On 07/15/2015 02:47 PM, Kevin Groeneveld wrote:
> With the following setup/steps I can consistently trigger the scsi host to
> hang requiring a reboot:
> 1. iMX6Q processor with built in AHCI compatible SATA host
> 2. SATA port multiplier in CBS mode connected to iMX6Q
> 3. HDD connected to port multiplier
> 4. CDROM connected to port multiplier
> 5. trigger continuous I/O to HDD
> 6. repeatedly execute CDROM_DRIVE_STATUS ioctl on CDROM with no disc in
>    drive
> 
> I don't think this issue is iMX6 specific but that is the only platform
> I have duplicated the hang on.
> 
> To trigger the issue at least two CPU cores must be enabled and the HDD
> access and CDROM ioctls must be happening concurrently. If I only enable
> one CPU core the hang does not occur.
> 
> The following C program can be used to trigger the CDROM ioctl:
> 
> #include <stdio.h>
> #include <fcntl.h>
> #include <linux/cdrom.h>
> 
> int main(int argc, char* argv[])
> {
> 	int fd;
> 
> 	fd = open("/dev/cdrom", O_RDONLY | O_NONBLOCK);
> 	if(fd < 0)
> 	{
> 		perror("cannot open /dev/cdrom");
> 		return fd;
> 	}
> 
> 	for(;;)
> 	{
> 		ioctl(fd, CDROM_DRIVE_STATUS, 0);
> 		usleep(100 * 1000);
> 	}
> }
> 
> When the hang occurs shost->host_busy == 2 and shost->host_failed == 1 in
> the scsi_eh_wakeup function. However this function only wakes the error
> handler if host_busy == host_failed.
> 
Which just means that one command is still outstanding, and we need
to wait for it to complete.
But see below...

> The patch changes the condition to test if host_busy >= host_failed and
> updates the corresponding condition in scsi_error_handler. Without the
> patch I can trigger the hang within seconds. With the patch I have not
> duplicated the hang after hours of testing.
> 
> Signed-off-by: Kevin Groeneveld <kgroeneveld@lenbrook.com>
> ---
>  drivers/scsi/scsi_error.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 106884a..853964b 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -61,7 +61,7 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *,
>  /* called with shost->host_lock held */
>  void scsi_eh_wakeup(struct Scsi_Host *shost)
>  {
> -	if (atomic_read(&shost->host_busy) == shost->host_failed) {
> +	if (atomic_read(&shost->host_busy) >= shost->host_failed) {
>  		trace_scsi_eh_wakeup(shost);
>  		wake_up_process(shost->ehandler);
>  		SCSI_LOG_ERROR_RECOVERY(5, shost_printk(KERN_INFO, shost,
> @@ -2173,7 +2173,7 @@ int scsi_error_handler(void *data)
>  	while (!kthread_should_stop()) {
>  		set_current_state(TASK_INTERRUPTIBLE);
>  		if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
> -		    shost->host_failed != atomic_read(&shost->host_busy)) {
> +		    shost->host_failed > atomic_read(&shost->host_busy)) {
>  			SCSI_LOG_ERROR_RECOVERY(1,
>  				shost_printk(KERN_INFO, shost,
>  					     "scsi_eh_%d: sleeping\n",
> 
Hmm.
I am really not sure about this.

'host_busy' indicates the number of outstanding commands, and
'host_failed' is the number of commands which have failed (on the
ground that failed commands are considered outstanding, too).

So the first hunk would change the behaviour from
'start SCSI EH once all commands are completed or failed' to
'start SCSI EH for _any_ command if scsi_eh_wakeup is called'
(note that shost_failed might be '0'...).
Which doesn't sound right.

The second hunk seems to be okay, as in principle 'host_busy' could
have been decreased before the check is done (ie someone could have
called ->done on a failed command).
But even so this would point to an invalid command completion; as
soon as a command is marked as 'failed' control is back in the SCSI
midlayer, and no-one else should be tampering with it.

I guess this needs further debugging to get to the bottom of it.

Sorry, but:

NACK.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +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)
--
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