SCSI EH cleanup

Hi all,

this is a resend of a small patchset for cleaning up SCSI EH. Primary goal is to make asynchronous aborts mandatory; there hasn't been a single report so far where asynchronous abort won't work, so the 'no_async_abort' flag has never been used and will be removed with this patchset.
Additionally there's a cleanup for handle failed EH commands, and to detect retries of failed commands.

As usual, comments and reviews are welcome.

Changes to v1:
- Include reviews from Christoph

Christoph Hellwig (1):
libsas: allow async aborts

Hannes Reinecke (4):
scsi: make eh_eflags persistent
scsi_error: do not escalate failed EH command
scsi: make scsi_eh_scmd_add() always succeed
scsi: make asynchronous aborts mandatory

 Documentation/scsi/scsi_eh.txt      |  30 ++++-----
 drivers/scsi/libsas/sas_scsi_host.c |   3 -
 drivers/scsi/scsi_error.c           | 125 ++++++------------------------------
 drivers/scsi/scsi_lib.c             |   4 +-
 drivers/scsi/scsi_priv.h            |   3 +-
 include/scsi/scsi_eh.h              |   1 +
 include/scsi/scsi_host.h            |   5 --
 7 files changed, 34 insertions(+), 137 deletions(-)

1.8.5.6

From: Christoph Hellwig <[email protected]>

We now first try to call ->eh_abort_handler from a work queue, but libsas was always failing that for no good reason. Allow async aborts.

---
 drivers/scsi/libsas/sas_scsi_host.c | 3 ---
 1 file changed, 3 deletions(-)

index 9bd55bc..ee6b39a 100644
@@ -491,9 +491,6 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd)
 	struct Scsi_Host *host = cmd->device->host;
 	struct sas_internal *i = to_sas_internal(host->transportt);
 
-	if (current != host->ehandler)
-		return FAILED;
-
 	if (!i->dft->lldd_abort_task)
 		return FAILED;
 

1.8.5.6

To detect if a failed command has been retried we must not clear scmd->eh_eflags when EH finishes.
The flag should be persistent throughout the lifetime of the command.

---
 Documentation/scsi/scsi_eh.txt | 14 +++++++-------
 drivers/scsi/scsi_error.c      |  4 ++--
 include/scsi/scsi_eh.h         |  1 +
 3 files changed, 10 insertions(+), 9 deletions(-)

index 37eca00..4edb9c1c 100644
@@ -105,11 +105,14 @@ function
 
  2. If the host supports asynchronous completion (as indicated by the
     no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort. If that fails
-    Step #3 is taken.
+    is invoked to schedule an asynchrous abort.
+    Asynchronous abort are not invoked for commands which the
+    SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
+    already had been aborted once, and this is a retry which failed),
+    or when the EH deadline is expired. In these case Step #3 is taken.
 
- 2. scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) is invoked for the
-    command.  See [1-3] for more information.
+ 3. scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) is invoked for the
+    command.  See [1-4] for more information.
 
 [1-3] Asynchronous command aborts
 

@@ -263,7 +266,6 @@ scmd->allowed.
 
  3. scmd recovered
     ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
-	- clear scmd->eh_eflags
 	- scsi_setup_cmd_retry()
 	- move from local eh_work_q to local eh_done_q
     LOCKING: none

@@ -456,8 +458,6 @@ except for #1 must be implemented by eh_strategy_handler().
 
  - shost->host_failed is zero.
 
- - Each scmd's eh_eflags field is cleared.
-
  - Each scmd is in such a state that scsi_setup_cmd_retry() on the
    scmd doesn't make any difference.
 

index 9d7bfbb..023f882 100644
@@ -188,7 +188,6 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 		/*
 		 * Retry after abort failed, escalate to next level.
 		 */
-		scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_INFO, scmd,
 				    "previous abort failed\n"));

@@ -931,6 +930,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
 	ses->result = scmd->result;
 	ses->underflow = scmd->underflow;
 	ses->prot_op = scmd->prot_op;
+	ses->eh_eflags = scmd->eh_eflags;
 
 	scmd->prot_op = SCSI_PROT_NORMAL;
 	scmd->eh_eflags = 0;

@@ -994,6 +994,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
 	scmd->result = ses->result;
 	scmd->underflow = ses->underflow;
 	scmd->prot_op = ses->prot_op;
+	scmd->eh_eflags = ses->eh_eflags;
 }
 EXPORT_SYMBOL(scsi_eh_restore_cmnd);
 

@@ -1126,7 +1127,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
  */
 void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
 {
-	scmd->eh_eflags = 0;
 	list_move_tail(&scmd->eh_entry, done_q);
 }
 EXPORT_SYMBOL(scsi_eh_finish_cmd);

index 98d366b..a25b328 100644
@@ -31,6 +31,7 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 struct scsi_eh_save {
 	/* saved state */
 	int result;
+	int eh_eflags;
 	enum dma_data_direction data_direction;
 	unsigned underflow;
 	unsigned char cmd_len;

1.8.5.6

Hello Hannes,

Is it on purpose that this patch does not remove the following statement from drivers/scsi/libsas/sas_scsi_host.c?

		cmd->eh_eflags = 0;

Thanks,

Bart.

Oh, yes, you are right.

Will be fixing it up.

Cheers,

Hannes

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

When a command is sent as part of the error handling there is not point whatsoever to start EH escalation when that command fails; we are _already_ in the error handler, and the escalation is about to commence anyway.
So just call 'scsi_try_to_abort_cmd()' to abort outstanding commands and let the main EH routine handle the rest.

---
 drivers/scsi/scsi_error.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

index 023f882..d0ecdef 100644
@@ -887,15 +887,6 @@ static int scsi_try_to_abort_cmd(struct scsi_host_template *hostt,
 	return hostt->eh_abort_handler(scmd);
 }
 
-static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
-{
-	if (scsi_try_to_abort_cmd(scmd->device->host->hostt, scmd) != SUCCESS)
-		if (scsi_try_bus_device_reset(scmd) != SUCCESS)
-			if (scsi_try_target_reset(scmd) != SUCCESS)
-				if (scsi_try_bus_reset(scmd) != SUCCESS)
-					scsi_try_host_reset(scmd);
-}
-
 /**
  * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recovery
  * @scmd:       SCSI command structure to hijack

@@ -1080,7 +1071,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			break;
 		}
 	} else if (rtn != FAILED) {
-		scsi_abort_eh_cmnd(scmd);
+		scsi_try_to_abort_cmd(shost->hostt, scmd);
 		rtn = FAILED;
 	}
 

1.8.5.6

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.

---
 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(-)

index d0ecdef..381d642 100644
@@ -162,13 +162,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);
 }
 
 /**

@@ -223,37 +217,32 @@ 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;
 
-	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;
 }
 
 /**

@@ -282,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;

index c35b6de..03e9d40 100644
@@ -1571,8 +1571,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;
 	}
 }
 

index 193636a..e4e3483 100644
@@ -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

Okay, will be fixing it up.

Thanks.

Cheers,

Hannes

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

There hasn't been any reports for HBAs where asynchronous abort would not work, so we should make it mandatory and remove the fallback.

---
 Documentation/scsi/scsi_eh.txt | 18 ++++------
 drivers/scsi/scsi_error.c      | 81 ++++--------------------------------------
 drivers/scsi/scsi_lib.c        |  2 +-
 drivers/scsi/scsi_priv.h       |  3 +-
 include/scsi/scsi_host.h       |  5 ---
 5 files changed, 15 insertions(+), 94 deletions(-)

index 4edb9c1c..11e447b 100644
@@ -70,7 +70,7 @@ with the command.
 	scmd is requeued to blk queue.
 
  - otherwise
-	scsi_eh_scmd_add(scmd, 0) is invoked for the command.  See
+	scsi_eh_scmd_add(scmd) is invoked for the command.  See
 	[1-3] for details of this function.
 
 

@@ -103,9 +103,7 @@ function
         eh_timed_out() callback did not handle the command.
 	Step #2 is taken.
 
- 2. If the host supports asynchronous completion (as indicated by the
-    no_async_abort setting in the host template) scsi_abort_command()
-    is invoked to schedule an asynchrous abort.
+ 2. scsi_abort_command() is invoked to schedule an asynchrous abort.
     Asynchronous abort are not invoked for commands which the
     SCSI_EH_ABORT_SCHEDULED flag is set (this indicates that the command
     already had been aborted once, and this is a retry which failed),

@@ -127,16 +125,13 @@ function
 
  scmds enter EH via scsi_eh_scmd_add(), which does the following.
 
- 1. Turns on scmd->eh_eflags as requested.  It's 0 for error
-    completions and SCSI_EH_CANCEL_CMD for timeouts.
+ 1. Links scmd->eh_entry to shost->eh_cmd_q
 
- 2. Links scmd->eh_entry to shost->eh_cmd_q
+ 2. Sets SHOST_RECOVERY bit in shost->shost_state
 
- 3. Sets SHOST_RECOVERY bit in shost->shost_state
+ 3. Increments shost->host_failed
 
- 4. Increments shost->host_failed
-
- 5. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
+ 4. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
 
  As can be seen above, once any scmd is added to shost->eh_cmd_q,
 SHOST_RECOVERY shost_state bit is turned on.  This prevents any new

@@ -252,7 +247,6 @@ scmd->allowed.
 
  1. Error completion / time out
     ACTION: scsi_eh_scmd_add() is invoked for scmd
-	- set scmd->eh_eflags
 	- add scmd to shost->eh_cmd_q
 	- set SHOST_RECOVERY
 	- shost->host_failed++

index 03e9d40..a656d27 100644
@@ -1571,7 +1571,7 @@ static void scsi_softirq_done(struct request *rq)
 			scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
 			break;
 		default:
-			scsi_eh_scmd_add(cmd, 0);
+			scsi_eh_scmd_add(cmd);
 			break;
 	}
 }

index e4e3483..827172e 100644
@@ -18,7 +18,6 @@
 /*
  * Scsi Error Handler Flags
  */
-#define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
 #define SCSI_EH_ABORT_SCHEDULED	0x0002	/* Abort has been scheduled */
 
 #define SCSI_SENSE_VALID(scmd) \

@@ -72,7 +71,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 void scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *);
 void scsi_eh_ready_devs(struct Scsi_Host *shost,
 			struct list_head *work_q,
 			struct list_head *done_q);

index 36680f1..ed7026c 100644
@@ -452,11 +452,6 @@ struct scsi_host_template {
 	unsigned no_write_same:1;
 
 	/*
-	 * True if asynchronous aborts are not supported
-	 */
-	unsigned no_async_abort:1;
-
-	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
 	unsigned int max_host_blocked;

1.8.5.6

Hello Hannes,

Supporting asynchronous aborts means that if no response is received for an abort after a certain time that the abort has to be considered as failed. A SCSI LLD may have to allocate resources before a TMF can be sent (e.g. ib_srp has to do that). These resources have to be cleaned up if a TMF times out. We don't want to see a TMF timeout handler in every LLD that supports asynchronous aborts. So we need a TMF timeout handler in the SCSI core. Have you considered to allocate a new SCSI request for submitting a TMF instead of overwriting fields in an existing scsi_cmnd structure? In that case we will be able to reuse the block layer timeout handler.

Something else I noticed is that the access of .eh_action in scsi_eh_done() races with the shost->eh_action = NULL assigment in scsi_send_eh_cmnd(). Shouldn't these accesses be protected by locking?

Thanks,

Bart.

TMF timeout is a completely different issue, and isn't touched at all by this patchset.

ATM the SCSI core expects any of the eh_XXX callback functions to be synchronous, ie any TMF timeout handling is delegated to the driver.

I plan to change that for the eh_abort() call, as most HBAs will be receiving a completion for both commands, the TMF and the associated command. So for these drivers there is not point in assuming a synchronous call; we can easily make this call asynchronous and wait for the completion to deliver the final status.
And indeed have a generic TMF timeout handling in the SCSI core.

And yes, I have considered allocating a separate command for TMFs. In fact, I'll be looking at implementing a RQF_RESET meta request (much like the RQF_FLUSH request we already have).
That would neatly solve the problem sg_reset has nowadays (namely that it doesn't have a SCSI command attached to it), and it would also decouple the failed command from the error handling itself. With that we could return the command to the block layer even when EH is still running, giving us better failover response times.

No, I don't think so.
The only place where this can race is once wait_for_completion_timeout() has timed out.
There indeed is a chance that scsi_eh_done() is called just before shost->eh_action = NULL.
But then this is not really critical as the 'done' structure itself is still valid until scsi_send_eh_cmnd() itself is finished. And calling 'complete' at any time before that will just be a no-op as the waiting is already terminated.

One _might_ consider a really fucked-up architecture where the assignment

	eh_action = scmd->device->host->eh_action;

is executed before 'shost->eh_action = NULL', but the final
complete(eh_action)
is executed after the entire scsi_send_eh_cmnd() function has completed. So from that point we should be issuing a 'wmb' or somesuch after shost->eh_action = NULL.
However, I do consider this highly unlikely.

Cheers,

Hannes

Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +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)

An additional comment: I don't think it is safe to execute a host reset while any asynchronous aborts are ongoing. I think the SCSI EH will get really confused if after a host reset has been performed a response is received from an asynchronous abort that was issued before the host reset started. Although a SCSI target must not send a LU RESET response before all previously submitted SCSI commands and TMFs have finished, what prevents in e.g. a multipath topology that an abort response and a LU RESET response are received out of order?

Thanks,

Bart.

Hmm.
Not sure this can happen nowadays.
eh_abort() is synchronous, and the command is considered in-flight until eh_abort() returns.
And as SCSI EH only starts until all commands are completed or timed out we will not have any stuck TMFs before sending a host reset. Things are different when calling 'sg_reset' manually, but I'll be revisiting that anyway in the near future as it has it's own set of problems.

Cheers,

Hannes

Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +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)

Hello Hannes,

There is a problem with asynchronous aborts. Some SCSI drivers, e.g. ib_srp, support fast error recovery by performing a transport layer reconnect without reporting this event as a failure to the SCSI core. While such a reconnect is ongoing it is important that no attempt is made to use the data structures that represent the connection. Hence the scsi_target_block() call in srp_reconnect_rport(). This blocks most .queuecommand() calls except those that originate from the SCSI EH. Hence the if (current == shost->ehandler) mutex_lock(&rport->mutex) code in srp_queuecommand(). Asynchronous aborts break this code because the asynchronous abort code submits an abort from another context than the SCSI EH thread. I know that this way of detecting the SCSI EH context is not an optimal solution. A few years ago I have tried to modify the SCSI EH such that reconnects and .queuecommand() calls could be serialized but James was not interested in such patches at that time.

Bart.

Don't think that's much of an issue.
scsi_abort_command() (ie the call which triggers async aborts) is only called if the .eh_timed_out() callback returns BLK_EH_NOT_HANDLED. So during reconnects we will not schedule any calls to async aborts. We _might_ have scheduled an async abort prior to a call to srp_reconnect_rport(), but that would be equivalent with calling srp_reconnect_rport() with commands still in flight.
Is that even possible?
If so, how do you handle these commands after reconnect returns? Any I_T_L nexus will be gone from the target, right?

Cheers,

Hannes

Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +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)

srp_reconnect_rport() can proceed while commands are in flight. What will happen is that no response will be received for the commands that are in flight and hence that these commands will time out at the initiator side. However, any newly submitted commands will be processed normally.

Bart.