[PATCHv6 0/3] New EH command timeout handler

[PATCHv6 0/3] New EH command timeout handler

[Hannes Reinecke]

Hi all,

this patchset implements a new SCSI EH command timeout handler
which will be sending command aborts inline without actually
engaging SCSI EH.
SCSI EH will only be invoked if command abort fails.

In addition the commands will be returned directly
if the command abort succeeded, cutting down recovery
times dramatically.

With the original SCSI EH I got:
# time dd if=/dev/zero of=/dev/dm-2 bs=4k count=4k oflag=direct
4096+0 records in
4096+0 records out
16777216 bytes (17 MB) copied, 142.652 s, 118 kB/s

real	2m22.657s
user	0m0.013s
sys	0m0.145s

With this patchset I got:
# time dd if=/dev/zero of=/dev/dm-2 bs=4k count=4k oflag=direct
4096+0 records in
4096+0 records out
16777216 bytes (17 MB) copied, 52.1579 s, 322 kB/s

real	0m52.163s
user	0m0.012s
sys	0m0.145s

Test was to disable RSCN on the target port, disable the
target port, and then start the 'dd' command as indicated.

Changes to the original version:
- Use a private list in scsi_eh_abort_handler to avoid
  list starvation (pointed out by Joern Engel)
- Terminate command aborts when the first abort fails
- Do not attempt command aborts if the host is already in recovery
  or if the device is removed.
- Flush abort workqueue if the device is removed.

Changes to v2:
- Removed eh_entry initialisation
- Convert to per-command workqueue

Changes to v3:
- Use delayed_work
- Enable new eh timeout handler for virtio, SAS, and FC
- Modify logging messages to include scmd pointer

Changes to v4:
- Remove stubs when enabling new eh timeout handler
  for other drivers

Changes to v5:
- Enable new eh timeout handler per default
- Update documentation

Hannes Reinecke (3):
  scsi: Fix erratic device offline during EH
  scsi: improved eh timeout handler
  scsi_error: Update documentation

 Documentation/scsi/scsi_eh.txt          |  69 +++++++------
 Documentation/scsi/scsi_mid_low_api.txt |   9 +-
 drivers/scsi/scsi.c                     |   9 +-
 drivers/scsi/scsi_error.c               | 177 ++++++++++++++++++++++++++++----
 drivers/scsi/scsi_priv.h                |   2 +
 include/scsi/scsi_cmnd.h                |   1 +
 include/scsi/scsi_host.h                |   5 +
 7 files changed, 214 insertions(+), 58 deletions(-)

-- 
1.7.12.4

[PATCH 1/3] scsi: Fix erratic device offline during EH

[Hannes Reinecke]

Commit 18a4d0a22ed6c54b67af7718c305cd010f09ddf8
(Handle disk devices which can not process medium access commands)
was introduced to offline any device which cannot process medium
access commands.
However, commit 3eef6257de48ff84a5d98ca533685df8a3beaeb8
(Reduce error recovery time by reducing use of TURs) reduced
the number of TURs by sending it only on the first failing
command, which might or might not be a medium access command.
So in combination this results in an erratic device offlining
during EH; if the command where the TUR was sent upon happens
to be a medium access command the device will be set offline,
if not everything proceeds as normal.

So instead of checking the EH command in the ->eh_action
callback we should rather call ->eh_action when we're
about to finish the command _and_ have sent a TUR previously.
This should then set the device offline as advertised.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Ewan Milne <emilne@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_error.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index abf0916..c88cb7e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -941,12 +941,6 @@ retry:
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
-		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
-		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
-	}
-
 	return rtn;
 }
 
@@ -964,6 +958,18 @@ static int scsi_request_sense(struct scsi_cmnd *scmd)
 	return scsi_send_eh_cmnd(scmd, NULL, 0, scmd->device->eh_timeout, ~0);
 }
 
+static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
+{
+	static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
+
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, tur_command, 6, rtn);
+	}
+	return rtn;
+}
+
 /**
  * scsi_eh_finish_cmd - Handle a cmd that eh is finished with.
  * @scmd:	Original SCSI cmd that eh has finished.
@@ -1142,7 +1148,9 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 
 		list_for_each_entry_safe(scmd, next, cmd_list, eh_entry)
 			if (scmd->device == sdev) {
-				if (finish_cmds)
+				if (finish_cmds &&
+				    (try_stu ||
+				     scsi_eh_action(scmd, SUCCESS) == SUCCESS))
 					scsi_eh_finish_cmd(scmd, done_q);
 				else
 					list_move_tail(&scmd->eh_entry, work_q);
@@ -1281,7 +1289,8 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
 			    !scsi_eh_tur(stu_scmd)) {
 				list_for_each_entry_safe(scmd, next,
 							  work_q, eh_entry) {
-					if (scmd->device == sdev)
+					if (scmd->device == sdev &&
+					    scsi_eh_action(scmd, SUCCESS) == SUCCESS)
 						scsi_eh_finish_cmd(scmd, done_q);
 				}
 			}
@@ -1348,7 +1357,8 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
 			    !scsi_eh_tur(bdr_scmd)) {
 				list_for_each_entry_safe(scmd, next,
 							 work_q, eh_entry) {
-					if (scmd->device == sdev)
+					if (scmd->device == sdev &&
+					    scsi_eh_action(scmd, rtn) != FAILED)
 						scsi_eh_finish_cmd(scmd,
 								   done_q);
 				}
-- 
1.7.12.4

[PATCH 2/3] scsi: improved eh timeout handler

[Hannes Reinecke]

When a command runs into a timeout we need to send an 'ABORT TASK'
TMF. This is typically done by the 'eh_abort_handler' LLDD callback.

Conceptually, however, this function is a normal SCSI command, so
there is no need to enter the error handler.

This patch implements a new scsi_abort_command() function which
invokes an asynchronous function scsi_eh_abort_handler() to
abort the commands via the usual 'eh_abort_handler'.

If abort succeeds the command is either retried or terminated,
depending on the number of allowed retries. However, 'eh_eflags'
records the abort, so if the retry would fail again the
command is pushed onto the error handler without trying to
abort it (again); it'll be cleared up from SCSI EH.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Cc: Ren Mingxin <renmx@cn.fujitsu.com>
---
 drivers/scsi/scsi.c       |   3 +
 drivers/scsi/scsi_error.c | 149 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/scsi/scsi_priv.h  |   2 +
 include/scsi/scsi_cmnd.h  |   1 +
 include/scsi/scsi_host.h  |   5 ++
 5 files changed, 147 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fe0bcb1..2b04a57 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -297,6 +297,7 @@ struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, gfp_t gfp_mask)
 
 		cmd->device = dev;
 		INIT_LIST_HEAD(&cmd->list);
+		INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 		spin_lock_irqsave(&dev->list_lock, flags);
 		list_add_tail(&cmd->list, &dev->cmd_list);
 		spin_unlock_irqrestore(&dev->list_lock, flags);
@@ -353,6 +354,8 @@ void scsi_put_command(struct scsi_cmnd *cmd)
 	list_del_init(&cmd->list);
 	spin_unlock_irqrestore(&cmd->device->list_lock, flags);
 
+	cancel_delayed_work(&cmd->abort_work);
+
 	__scsi_put_command(cmd->device->host, cmd, &sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_put_command);
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c88cb7e..8eaf524 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -53,6 +53,7 @@ static void scsi_eh_done(struct scsi_cmnd *scmd);
 #define HOST_RESET_SETTLE_TIME  (10)
 
 static int scsi_eh_try_stu(struct scsi_cmnd *scmd);
+static int scsi_try_to_abort_cmd(struct scsi_host_template *, struct scsi_cmnd *);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)
@@ -100,6 +101,115 @@ static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 }
 
 /**
+ * scmd_eh_abort_handler - Handle command aborts
+ * @work:	command to be aborted.
+ */
+void
+scmd_eh_abort_handler(struct work_struct *work)
+{
+	struct scsi_cmnd *scmd =
+		container_of(work, struct scsi_cmnd, abort_work.work);
+	struct scsi_device *sdev = scmd->device;
+	unsigned long flags;
+	int rtn;
+
+	spin_lock_irqsave(sdev->host->host_lock, flags);
+	if (scsi_host_eh_past_deadline(sdev->host)) {
+		spin_unlock_irqrestore(sdev->host->host_lock, flags);
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "scmd %p eh timeout, not aborting\n", scmd));
+	} else {
+		spin_unlock_irqrestore(sdev->host->host_lock, flags);
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "aborting command %p\n", scmd));
+		rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
+		if (rtn == SUCCESS) {
+			scmd->result |= DID_TIME_OUT << 16;
+			if (!scsi_noretry_cmd(scmd) &&
+			    (++scmd->retries <= scmd->allowed)) {
+				SCSI_LOG_ERROR_RECOVERY(3,
+					scmd_printk(KERN_WARNING, scmd,
+						    "scmd %p retry "
+						    "aborted command\n", scmd));
+				scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
+			} else {
+				SCSI_LOG_ERROR_RECOVERY(3,
+					scmd_printk(KERN_WARNING, scmd,
+						    "scmd %p finish "
+						    "aborted command\n", scmd));
+				scsi_finish_command(scmd);
+			}
+			return;
+		}
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "scmd %p abort failed, rtn %d\n",
+				    scmd, rtn));
+	}
+
+	if (scsi_eh_scmd_add(scmd, 0)) {
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_WARNING, scmd,
+				    "scmd %p terminate "
+				    "aborted command\n", scmd));
+		scmd->result |= DID_TIME_OUT << 16;
+		scsi_finish_command(scmd);
+	}
+}
+
+/**
+ * scsi_abort_command - schedule a command abort
+ * @scmd:	scmd to abort.
+ *
+ * We only need to abort commands after a command timeout
+ */
+static int
+scsi_abort_command(struct scsi_cmnd *scmd)
+{
+	struct scsi_device *sdev = scmd->device;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
+
+	if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) {
+		/*
+		 * command abort timed out.
+		 */
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "scmd %p abort timeout\n", scmd));
+		cancel_delayed_work(&scmd->abort_work);
+		return FAILED;
+	}
+
+	/*
+	 * Do not try a command abort if
+	 * SCSI EH has already started.
+	 */
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (scsi_host_in_recovery(shost)) {
+		spin_unlock_irqrestore(shost->host_lock, flags);
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "scmd %p not aborting, host in recovery\n",
+				    scmd));
+		return FAILED;
+	}
+
+	if (shost->eh_deadline && !shost->last_reset)
+		shost->last_reset = jiffies;
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
+	scmd->eh_eflags |= SCSI_EH_ABORT_SCHEDULED;
+	SCSI_LOG_ERROR_RECOVERY(3,
+		scmd_printk(KERN_INFO, scmd,
+			    "scmd %p abort scheduled\n", scmd));
+	schedule_delayed_work(&scmd->abort_work, HZ / 100);
+	return SUCCESS;
+}
+
+/**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
  * @eh_flag:	optional SCSI_EH flag.
@@ -125,6 +235,8 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 		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++;
@@ -161,6 +273,10 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 	else if (host->hostt->eh_timed_out)
 		rtn = host->hostt->eh_timed_out(scmd);
 
+	if (rtn == BLK_EH_NOT_HANDLED && !host->hostt->no_async_abort)
+		if (scsi_abort_command(scmd) == SUCCESS)
+			return BLK_EH_NOT_HANDLED;
+
 	scmd->result |= DID_TIME_OUT << 16;
 
 	if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
@@ -1577,7 +1693,7 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
 }
 
 /**
- * scsi_noretry_cmd - determinte if command should be failed fast
+ * scsi_noretry_cmd - determine if command should be failed fast
  * @scmd:	SCSI cmd to examine.
  */
 int scsi_noretry_cmd(struct scsi_cmnd *scmd)
@@ -1585,6 +1701,8 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
 	switch (host_byte(scmd->result)) {
 	case DID_OK:
 		break;
+	case DID_TIME_OUT:
+		goto check_type;
 	case DID_BUS_BUSY:
 		return (scmd->request->cmd_flags & REQ_FAILFAST_TRANSPORT);
 	case DID_PARITY:
@@ -1598,18 +1716,19 @@ int scsi_noretry_cmd(struct scsi_cmnd *scmd)
 		return (scmd->request->cmd_flags & REQ_FAILFAST_DRIVER);
 	}
 
-	switch (status_byte(scmd->result)) {
-	case CHECK_CONDITION:
-		/*
-		 * assume caller has checked sense and determinted
-		 * the check condition was retryable.
-		 */
-		if (scmd->request->cmd_flags & REQ_FAILFAST_DEV ||
-		    scmd->request->cmd_type == REQ_TYPE_BLOCK_PC)
-			return 1;
-	}
+	switch (status_byte(scmd->result) != CHECK_CONDITION)
+		return 0;
 
-	return 0;
+check_type:
+	/*
+	 * assume caller has checked sense and determined
+	 * the check condition was retryable.
+	 */
+	if (scmd->request->cmd_flags & REQ_FAILFAST_DEV ||
+	    scmd->request->cmd_type == REQ_TYPE_BLOCK_PC)
+		return 1;
+	else
+		return 0;
 }
 
 /**
@@ -1659,9 +1778,13 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
 		 * looks good.  drop through, and check the next byte.
 		 */
 		break;
+	case DID_ABORT:
+		if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) {
+			scmd->result |= DID_TIME_OUT << 16;
+			return SUCCESS;
+		}
 	case DID_NO_CONNECT:
 	case DID_BAD_TARGET:
-	case DID_ABORT:
 		/*
 		 * note - this means that we just report the status back
 		 * to the top level driver, not that we actually think
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 8f9a0ca..f079a59 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -19,6 +19,7 @@ struct scsi_nl_hdr;
  * 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) \
 	(((scmd)->sense_buffer[0] & 0x70) == 0x70)
@@ -66,6 +67,7 @@ extern int __init scsi_init_devinfo(void);
 extern void scsi_exit_devinfo(void);
 
 /* scsi_error.c */
+extern void scmd_eh_abort_handler(struct work_struct *work);
 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);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de5f5d8..91558a1 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -55,6 +55,7 @@ struct scsi_cmnd {
 	struct scsi_device *device;
 	struct list_head list;  /* scsi_cmnd participates in queue lists */
 	struct list_head eh_entry; /* entry for the host eh_cmd_q */
+	struct delayed_work abort_work;
 	int eh_eflags;		/* Used by error handlr */
 
 	/*
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 896bb05..6ca9174 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -476,6 +476,11 @@ struct scsi_host_template {
 	unsigned ordered_tag: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.7.12.4

[PATCH 3/3] scsi_error: Update documentation

[Hannes Reinecke]

Update the documentation to cover asynchronous command aborts
and clarify sections referring to SCSI timeout handling.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 Documentation/scsi/scsi_eh.txt          | 69 +++++++++++++++++++--------------
 Documentation/scsi/scsi_mid_low_api.txt |  9 ++++-
 drivers/scsi/scsi.c                     |  6 +--
 3 files changed, 48 insertions(+), 36 deletions(-)

diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 6ff16b6..a0c8511 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -42,20 +42,14 @@ discussion.
 
  Once LLDD gets hold of a scmd, either the LLDD will complete the
 command by calling scsi_done callback passed from midlayer when
-invoking hostt->queuecommand() or SCSI midlayer will time it out.
+invoking hostt->queuecommand() or the block layer will time it out.
 
 
 [1-2-1] Completing a scmd w/ scsi_done
 
  For all non-EH commands, scsi_done() is the completion callback.  It
-does the following.
-
- 1. Delete timeout timer.  If it fails, it means that timeout timer
-    has expired and is going to finish the command.  Just return.
-
- 2. Link scmd to per-cpu scsi_done_q using scmd->en_entry
-
- 3. Raise SCSI_SOFTIRQ
+just calls blk_complete_request() to delete the block layer timer and
+raise SCSI_SOFTIRQ
 
  SCSI_SOFTIRQ handler scsi_softirq calls scsi_decide_disposition() to
 determine what to do with the command.  scsi_decide_disposition()
@@ -64,10 +58,12 @@ with the command.
 
  - SUCCESS
 	scsi_finish_command() is invoked for the command.  The
-	function does some maintenance choirs and notify completion by
-	calling scmd->done() callback, which, for fs requests, would
-	be HLD completion callback - sd:sd_rw_intr, sr:rw_intr,
-	st:st_intr.
+	function does some maintenance chores and then calls
+	scsi_io_completion() to finish the I/O.
+	scsi_io_completion() then notifies the block layer on
+	the completed request by calling blk_end_request and
+	friends or figures out what to do with the remainder
+	of the data in case of an error.
 
  - NEEDS_RETRY
  - ADD_TO_MLQUEUE
@@ -86,33 +82,45 @@ function
  1. invokes optional hostt->eh_timed_out() callback.  Return value can
     be one of
 
-    - EH_HANDLED
-	This indicates that eh_timed_out() dealt with the timeout.  The
-	scmd is passed to __scsi_done() and thus linked into per-cpu
-	scsi_done_q.  Normal command completion described in [1-2-1]
-	follows.
+    - BLK_EH_HANDLED
+	This indicates that eh_timed_out() dealt with the timeout.
+	The command is passed back to the block layer and completed
+	via __blk_complete_requests().
+
+	*NOTE* After returning BLK_EH_HANDLED the SCSI layer is
+	assumed to be finished with the command, and no other
+	functions from the SCSI layer will be called. So this
+	should typically only be returned if the eh_timed_out()
+	handler raced with normal completion.
 
-    - EH_RESET_TIMER
+    - BLK_EH_RESET_TIMER
 	This indicates that more time is required to finish the
 	command.  Timer is restarted.  This action is counted as a
 	retry and only allowed scmd->allowed + 1(!) times.  Once the
-	limit is reached, action for EH_NOT_HANDLED is taken instead.
+	limit is reached, action for BLK_EH_NOT_HANDLED is taken instead.
 
-	*NOTE* This action is racy as the LLDD could finish the scmd
-	after the timeout has expired but before it's added back.  In
-	such cases, scsi_done() would think that timeout has occurred
-	and return without doing anything.  We lose completion and the
-	command will time out again.
-
-    - EH_NOT_HANDLED
-	This is the same as when eh_timed_out() callback doesn't exist.
+    - BLK_EH_NOT_HANDLED
+        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. If that fails
+    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.
 
+[1-3] Asynchronous command aborts
+
+ After a timeout occurs a command abort is scheduled from
+ scsi_abort_command(). If the abort is successful the command
+ will either be retried (if the number of retries is not exhausted)
+ or terminated with DID_TIME_OUT.
+ Otherwise scsi_eh_scmd_add() is invoked for the command.
+ See [1-4] for more information.
 
-[1-3] How EH takes over
+[1-4] How EH takes over
 
  scmds enter EH via scsi_eh_scmd_add(), which does the following.
 
@@ -320,7 +328,8 @@ scmd->allowed.
 
     <<scsi_eh_abort_cmds>>
 
-	This action is taken for each timed out command.
+	This action is taken for each timed out command when
+	no_async_abort is enabled in the host template.
 	hostt->eh_abort_handler() is invoked for each scmd.  The
 	handler returns SUCCESS if it has succeeded to make LLDD and
 	all related hardware forget about the scmd.
diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
index 2b06aba..78f419e 100644
--- a/Documentation/scsi/scsi_mid_low_api.txt
+++ b/Documentation/scsi/scsi_mid_low_api.txt
@@ -882,8 +882,11 @@ Details:
  *
  *      Calling context: kernel thread
  *
- *      Notes: Invoked from scsi_eh thread. No other commands will be
- *      queued on current host during eh.
+ *      Notes: If 'no_async_abort' is defined this callback
+ *  	will be invoked from scsi_eh thread. No other commands
+ *	will then be queued on current host during eh.
+ *	Otherwise it will be called whenever scsi_times_out()
+ *      is called due to a command timeout.
  *
  *      Optionally defined in: LLD
  **/
@@ -1257,6 +1260,8 @@ of interest:
                    address space
     use_clustering - 1=>SCSI commands in mid level's queue can be merged,
                      0=>disallow SCSI command merging
+    no_async_abort - 1=>Asynchronous aborts are not supported
+    		     0=>Timed-out commands will be aborted asynchronously
     hostt        - pointer to driver's struct scsi_host_template from which
                    this struct Scsi_Host instance was spawned
     hostt->proc_name  - name of LLD. This is the driver name that sysfs uses
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2b04a57..d8afec8 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -745,15 +745,13 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
 }
 
 /**
- * scsi_done - Enqueue the finished SCSI command into the done queue.
+ * scsi_done - Invoke completion on finished SCSI command.
  * @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
  * ownership back to SCSI Core -- i.e. the LLDD has finished with it.
  *
  * Description: This function is the mid-level's (SCSI Core) interrupt routine,
  * which regains ownership of the SCSI command (de facto) from a LLDD, and
- * enqueues the command to the done queue for further processing.
- *
- * This is the producer of the done queue who enqueues at the tail.
+ * calls blk_complete_request() for further processing.
  *
  * This function is interrupt context safe.
  */
-- 
1.7.12.4

Re: [PATCH 2/3] scsi: improved eh timeout handler

[Ren Mingxin]

Hi, Hannes:

On 09/02/2013 07:58 PM, Hannes Reinecke wrote:
> If abort succeeds the command is either retried or terminated,
> depending on the number of allowed retries. However, 'eh_eflags'
> records the abort, so if the retry would fail again the
> command is pushed onto the error handler without trying to
> abort it (again); it'll be cleared up from SCSI EH.

I'm still thinking about the aborting 'scsi_eh_abort_cmds()' in SCSI
EH - does it make sense to abort in SCSI EH since we've tried to
abort via your scsi_abort_command()? Though the aborting in SCSI EH
will handle commands which havn't been aborted in scsi_abort_command
since EH has been engaged.

Thanks,
Ren

Re: [PATCH 1/3] scsi: Fix erratic device offline during EH

[Jeremy Linton]

On 9/2/2013 6:58 AM, Hannes Reinecke wrote:

> +static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn) +{ +	static
> unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0}; + +	if
> (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) { +		struct scsi_driver
> *sdrv = scsi_cmd_to_driver(scmd); +		if (sdrv->eh_action) +			rtn =
> sdrv->eh_action(scmd, tur_command, 6, rtn); +	} +	return rtn; +} +

	Is there are reason for using TUR here instead of STD inquiry? STD inquiry has
the advantage that it can act like a "ping" but doesn't return unit
attentions.

	Per my previous comments, trapping unit attentions in the error handler has
caused UA's like luns changed, or power loss to get lost without being
processed. For tape devices loosing UA's like this often means that the higher
level driver won't be notified that the tape is rewound, resulting in serious
issues.

Re: [PATCH 2/3] scsi: improved eh timeout handler

[Hannes Reinecke]

On 09/11/2013 11:16 AM, Ren Mingxin wrote:
> Hi, Hannes:
>
> On 09/02/2013 07:58 PM, Hannes Reinecke wrote:
>> If abort succeeds the command is either retried or terminated,
>> depending on the number of allowed retries. However, 'eh_eflags'
>> records the abort, so if the retry would fail again the
>> command is pushed onto the error handler without trying to
>> abort it (again); it'll be cleared up from SCSI EH.
>
> I'm still thinking about the aborting 'scsi_eh_abort_cmds()' in SCSI
> EH - does it make sense to abort in SCSI EH since we've tried to
> abort via your scsi_abort_command()? Though the aborting in SCSI EH
> will handle commands which havn't been aborted in scsi_abort_command
> since EH has been engaged.
>
Well, with the original design scsi_abort_command() was optional, and
only enabled for selected HBAs.
And I would rather keep it the original implementation, as there might
be HBAs who don't like inline aborts.

Cheers,

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

Re: [PATCH 2/3] scsi: improved eh timeout handler

[Ren Mingxin]

Hi, Hannes:

On 09/02/2013 07:58 PM, Hannes Reinecke wrote:
> +scmd_eh_abort_handler(struct work_struct *work)
> +{
> +	struct scsi_cmnd *scmd =
> +		container_of(work, struct scsi_cmnd, abort_work.work);
> +	struct scsi_device *sdev = scmd->device;
> +	unsigned long flags;
> +	int rtn;
> +
> +	spin_lock_irqsave(sdev->host->host_lock, flags);
> +	if (scsi_host_eh_past_deadline(sdev->host)) {
> +		spin_unlock_irqrestore(sdev->host->host_lock, flags);
> +		SCSI_LOG_ERROR_RECOVERY(3,
> +			scmd_printk(KERN_INFO, scmd,
> +				    "scmd %p eh timeout, not aborting\n", scmd));
> +	} else {
> +		spin_unlock_irqrestore(sdev->host->host_lock, flags);
> +		SCSI_LOG_ERROR_RECOVERY(3,
> +			scmd_printk(KERN_INFO, scmd,
> +				    "aborting command %p\n", scmd));
> +		rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
> +		if (rtn == SUCCESS) {
> +			scmd->result |= DID_TIME_OUT<<  16;
> +			if (!scsi_noretry_cmd(scmd)&&
> +			    (++scmd->retries<= scmd->allowed)) {

I think scsi_host_eh_past_deadline() should be checked here like:

-			if (!scsi_noretry_cmd(scmd)&&
+			if (!scsi_host_eh_past_deadline(sdev->host)&&
+			    !scsi_noretry_cmd(scmd)&&

According to my test, once retry requires 30 seconds. If eh_deadline
is reached, we can stop EH here without waiting for long term
retrying.

Thanks,
Ren

>
> +				SCSI_LOG_ERROR_RECOVERY(3,
> +					scmd_printk(KERN_WARNING, scmd,
> +						    "scmd %p retry "
> +						    "aborted command\n", scmd));
> +				scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
> +			} else {
> +				SCSI_LOG_ERROR_RECOVERY(3,
> +					scmd_printk(KERN_WARNING, scmd,
> +						    "scmd %p finish "
> +						    "aborted command\n", scmd));
> +				scsi_finish_command(scmd);
> +			}
> +			return;
> +		}

Re: [PATCHv6 0/3] New EH command timeout handler

[Christoph Hellwig]

James, and comments on this?

It would be really good if we could make some progress getting various
infrastructure merged.

On Mon, Sep 02, 2013 at 01:58:35PM +0200, Hannes Reinecke wrote:
> Hi all,
> 
> this patchset implements a new SCSI EH command timeout handler
> which will be sending command aborts inline without actually
> engaging SCSI EH.
> SCSI EH will only be invoked if command abort fails.
> 
> In addition the commands will be returned directly
> if the command abort succeeded, cutting down recovery
> times dramatically.
> 
> With the original SCSI EH I got:
> # time dd if=/dev/zero of=/dev/dm-2 bs=4k count=4k oflag=direct
> 4096+0 records in
> 4096+0 records out
> 16777216 bytes (17 MB) copied, 142.652 s, 118 kB/s
> 
> real	2m22.657s
> user	0m0.013s
> sys	0m0.145s
> 
> With this patchset I got:
> # time dd if=/dev/zero of=/dev/dm-2 bs=4k count=4k oflag=direct
> 4096+0 records in
> 4096+0 records out
> 16777216 bytes (17 MB) copied, 52.1579 s, 322 kB/s
> 
> real	0m52.163s
> user	0m0.012s
> sys	0m0.145s
> 
> Test was to disable RSCN on the target port, disable the
> target port, and then start the 'dd' command as indicated.
> 
> Changes to the original version:
> - Use a private list in scsi_eh_abort_handler to avoid
>   list starvation (pointed out by Joern Engel)
> - Terminate command aborts when the first abort fails
> - Do not attempt command aborts if the host is already in recovery
>   or if the device is removed.
> - Flush abort workqueue if the device is removed.
> 
> Changes to v2:
> - Removed eh_entry initialisation
> - Convert to per-command workqueue
> 
> Changes to v3:
> - Use delayed_work
> - Enable new eh timeout handler for virtio, SAS, and FC
> - Modify logging messages to include scmd pointer
> 
> Changes to v4:
> - Remove stubs when enabling new eh timeout handler
>   for other drivers
> 
> Changes to v5:
> - Enable new eh timeout handler per default
> - Update documentation
> 
> Hannes Reinecke (3):
>   scsi: Fix erratic device offline during EH
>   scsi: improved eh timeout handler
>   scsi_error: Update documentation
> 
>  Documentation/scsi/scsi_eh.txt          |  69 +++++++------
>  Documentation/scsi/scsi_mid_low_api.txt |   9 +-
>  drivers/scsi/scsi.c                     |   9 +-
>  drivers/scsi/scsi_error.c               | 177 ++++++++++++++++++++++++++++----
>  drivers/scsi/scsi_priv.h                |   2 +
>  include/scsi/scsi_cmnd.h                |   1 +
>  include/scsi/scsi_host.h                |   5 +
>  7 files changed, 214 insertions(+), 58 deletions(-)
> 
> -- 
> 1.7.12.4
> 
> --
> 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
---end quoted text---

Re: [PATCH 2/3] scsi: improved eh timeout handler

[Hannes Reinecke]

On 09/20/2013 09:59 AM, Ren Mingxin wrote:
> Hi, Hannes:
>
> On 09/02/2013 07:58 PM, Hannes Reinecke wrote:
>> +scmd_eh_abort_handler(struct work_struct *work)
>> +{
>> +    struct scsi_cmnd *scmd =
>> +        container_of(work, struct scsi_cmnd, abort_work.work);
>> +    struct scsi_device *sdev = scmd->device;
>> +    unsigned long flags;
>> +    int rtn;
>> +
>> +    spin_lock_irqsave(sdev->host->host_lock, flags);
>> +    if (scsi_host_eh_past_deadline(sdev->host)) {
>> +        spin_unlock_irqrestore(sdev->host->host_lock, flags);
>> +        SCSI_LOG_ERROR_RECOVERY(3,
>> +            scmd_printk(KERN_INFO, scmd,
>> +                    "scmd %p eh timeout, not aborting\n", scmd));
>> +    } else {
>> +        spin_unlock_irqrestore(sdev->host->host_lock, flags);
>> +        SCSI_LOG_ERROR_RECOVERY(3,
>> +            scmd_printk(KERN_INFO, scmd,
>> +                    "aborting command %p\n", scmd));
>> +        rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
>> +        if (rtn == SUCCESS) {
>> +            scmd->result |= DID_TIME_OUT<<  16;
>> +            if (!scsi_noretry_cmd(scmd)&&
>> +                (++scmd->retries<= scmd->allowed)) {
>
> I think scsi_host_eh_past_deadline() should be checked here like:
>
> -            if (!scsi_noretry_cmd(scmd)&&
> +            if (!scsi_host_eh_past_deadline(sdev->host)&&
> +                !scsi_noretry_cmd(scmd)&&
>
> According to my test, once retry requires 30 seconds. If eh_deadline
> is reached, we can stop EH here without waiting for long term
> retrying.
>

Hmm. We could. Providing the eh_deadline patches are going in.

James, should I resend the patchset?
Does it even have a chance of getting in?

Cheers,

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

[PATCH] scsi: Set the minimum valid value of 'eh_deadline' as 0

[Ren Mingxin]

The former minimum valid value of 'eh_deadline' is 1s, which means
the earliest occasion to shorten EH is 1 second later since a
command is failed or timed out. But if we want to skip EH steps
ASAP, we have to wait until the first EH step is finished. If the
duration of the first EH step is long, this waiting time is
excruciating. So, it is necessary to accept 0 as the minimum valid
value for 'eh_deadline'.

According to my test, with Hannes' patchset 'New EH command timeout
handler' as well, the minimum IO time is improved from 73s
(eh_deadline = 1) to 43s(eh_deadline = 0) when commands are timed
out by disabling RSCN and target port.

Another thing: scsi_finish_command() should be invoked if
scsi_eh_scmd_add() is returned on failure - let EH finish those
commands.

Signed-off-by: Ren Mingxin <renmx@cn.fujitsu.com>
---
 drivers/scsi/hosts.c      |   14 +++++++++++---
 drivers/scsi/scsi_error.c |   40 +++++++++++++++++++++++++++-------------
 drivers/scsi/scsi_sysfs.c |   36 +++++++++++++++++++++++++-----------
 include/scsi/scsi_host.h  |    2 +-
 4 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f334859..e84123a 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -316,11 +316,11 @@ static void scsi_host_dev_release(struct device *dev)
 	kfree(shost);
 }
 
-static unsigned int shost_eh_deadline;
+static unsigned int shost_eh_deadline = -1;
 
 module_param_named(eh_deadline, shost_eh_deadline, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(eh_deadline,
-		 "SCSI EH timeout in seconds (should be between 1 and 2^32-1)");
+		 "SCSI EH timeout in seconds (should be between 0 and 2^32-1)");
 
 static struct device_type scsi_host_type = {
 	.name =		"scsi_host",
@@ -394,7 +394,15 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
-	shost->eh_deadline = shost_eh_deadline * HZ;
+	if (shost_eh_deadline == -1)
+		shost->eh_deadline = -1;
+	else if ((ulong) shost_eh_deadline * HZ > UINT_MAX) {
+		printk(KERN_WARNING "scsi%d: eh_deadline %u exceeds the "
+		       "maximum, setting to %u\n",
+		       shost->host_no, shost_eh_deadline, UINT_MAX / HZ);
+		shost->eh_deadline = UINT_MAX / HZ * HZ;
+	} else
+		shost->eh_deadline = shost_eh_deadline * HZ;
 
 	if (sht->supported_mode == MODE_UNKNOWN)
 		/* means we didn't set it ... default to INITIATOR */
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index adb4cbe..c2f9431 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(scsi_schedule_eh);
 
 static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
 {
-	if (!shost->last_reset || !shost->eh_deadline)
+	if (!shost->last_reset || shost->eh_deadline == -1)
 		return 0;
 
 	if (time_before(jiffies,
@@ -127,29 +127,43 @@ scmd_eh_abort_handler(struct work_struct *work)
 		rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
 		if (rtn == SUCCESS) {
 			scmd->result |= DID_TIME_OUT << 16;
-			if (!scsi_noretry_cmd(scmd) &&
+			spin_lock_irqsave(sdev->host->host_lock, flags);
+			if (scsi_host_eh_past_deadline(sdev->host)) {
+				spin_unlock_irqrestore(sdev->host->host_lock,
+						       flags);
+				SCSI_LOG_ERROR_RECOVERY(3,
+					scmd_printk(KERN_INFO, scmd,
+						    "scmd %p eh timeout, "
+						    "not retrying aborted "
+						    "command\n", scmd));
+			} else if (!scsi_noretry_cmd(scmd) &&
 			    (++scmd->retries <= scmd->allowed)) {
+				spin_unlock_irqrestore(sdev->host->host_lock,
+						       flags);
 				SCSI_LOG_ERROR_RECOVERY(3,
 					scmd_printk(KERN_WARNING, scmd,
 						    "scmd %p retry "
 						    "aborted command\n", scmd));
 				scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
+				return;
 			} else {
+				spin_unlock_irqrestore(sdev->host->host_lock,
+						       flags);
 				SCSI_LOG_ERROR_RECOVERY(3,
 					scmd_printk(KERN_WARNING, scmd,
 						    "scmd %p finish "
 						    "aborted command\n", scmd));
 				scsi_finish_command(scmd);
+				return;
 			}
-			return;
-		}
-		SCSI_LOG_ERROR_RECOVERY(3,
-			scmd_printk(KERN_INFO, scmd,
-				    "scmd %p abort failed, rtn %d\n",
-				    scmd, rtn));
+		} else
+			SCSI_LOG_ERROR_RECOVERY(3,
+				scmd_printk(KERN_INFO, scmd,
+					    "scmd %p abort failed, rtn %d\n",
+					    scmd, rtn));
 	}
 
-	if (scsi_eh_scmd_add(scmd, 0)) {
+	if (!scsi_eh_scmd_add(scmd, 0)) {
 		SCSI_LOG_ERROR_RECOVERY(3,
 			scmd_printk(KERN_WARNING, scmd,
 				    "scmd %p terminate "
@@ -197,7 +211,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
 		return FAILED;
 	}
 
-	if (shost->eh_deadline && !shost->last_reset)
+	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 	spin_unlock_irqrestore(shost->host_lock, flags);
 
@@ -231,7 +245,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
 		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
 			goto out_unlock;
 
-	if (shost->eh_deadline && !shost->last_reset)
+	if (shost->eh_deadline != -1 && !shost->last_reset)
 		shost->last_reset = jiffies;
 
 	ret = 1;
@@ -265,7 +279,7 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 	trace_scsi_dispatch_cmd_timeout(scmd);
 	scsi_log_completion(scmd, TIMEOUT_ERROR);
 
-	if (host->eh_deadline && !host->last_reset)
+	if (host->eh_deadline != -1 && !host->last_reset)
 		host->last_reset = jiffies;
 
 	if (host->transportt->eh_timed_out)
@@ -2130,7 +2144,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
 			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
 
 	spin_lock_irqsave(shost->host_lock, flags);
-	if (shost->eh_deadline)
+	if (shost->eh_deadline != -1)
 		shost->last_reset = 0;
 	spin_unlock_irqrestore(shost->host_lock, flags);
 	scsi_eh_flush_done_q(&eh_done_q);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 57c78ef..b0591aa 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -287,7 +287,9 @@ show_shost_eh_deadline(struct device *dev,
 {
 	struct Scsi_Host *shost = class_to_shost(dev);
 
-	return sprintf(buf, "%d\n", shost->eh_deadline / HZ);
+	if (shost->eh_deadline == -1)
+		return snprintf(buf, strlen("off") + 2, "off\n");
+	return sprintf(buf, "%u\n", shost->eh_deadline / HZ);
 }
 
 static ssize_t
@@ -296,22 +298,34 @@ store_shost_eh_deadline(struct device *dev, struct device_attribute *attr,
 {
 	struct Scsi_Host *shost = class_to_shost(dev);
 	int ret = -EINVAL;
-	int deadline;
-	unsigned long flags;
+	unsigned long deadline, flags;
+	char *cp;
 
 	if (shost->transportt && shost->transportt->eh_strategy_handler)
 		return ret;
 
-	if (sscanf(buf, "%d\n", &deadline) == 1) {
-		spin_lock_irqsave(shost->host_lock, flags);
-		if (scsi_host_in_recovery(shost))
-			ret = -EBUSY;
-		else {
+	if (!strncmp(buf, "off", strlen("off")))
+		deadline = -1;
+	else {
+		deadline = simple_strtoul(buf, &cp, 0);
+		if ((*cp && (*cp != '\n')) ||
+		    deadline * HZ > UINT_MAX)
+			return ret;
+	}
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	if (scsi_host_in_recovery(shost))
+		ret = -EBUSY;
+	else {
+		if (deadline == -1)
+			shost->eh_deadline = -1;
+		else
 			shost->eh_deadline = deadline * HZ;
-			ret = count;
-		}
-		spin_unlock_irqrestore(shost->host_lock, flags);
+
+		ret = count;
 	}
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	return ret;
 }
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 6ca9174..b50355c 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -603,7 +603,7 @@ struct Scsi_Host {
 	unsigned int host_eh_scheduled;    /* EH scheduled without command */
 
 	unsigned int host_no;  /* Used for IOCTL_GET_IDLUN, /proc/scsi et al. */
-	int eh_deadline;
+	unsigned int eh_deadline;
 	unsigned long last_reset;
 
 	/*
-- 
1.7.1

Re: [PATCH] scsi: Set the minimum valid value of 'eh_deadline' as 0

[Hannes Reinecke]

On 10/09/2013 09:43 AM, Ren Mingxin wrote:
> The former minimum valid value of 'eh_deadline' is 1s, which means
> the earliest occasion to shorten EH is 1 second later since a
> command is failed or timed out. But if we want to skip EH steps
> ASAP, we have to wait until the first EH step is finished. If the
> duration of the first EH step is long, this waiting time is
> excruciating. So, it is necessary to accept 0 as the minimum valid
> value for 'eh_deadline'.
>
> According to my test, with Hannes' patchset 'New EH command timeout
> handler' as well, the minimum IO time is improved from 73s
> (eh_deadline = 1) to 43s(eh_deadline = 0) when commands are timed
> out by disabling RSCN and target port.
>
> Another thing: scsi_finish_command() should be invoked if
> scsi_eh_scmd_add() is returned on failure - let EH finish those
> commands.
>
D'accord. Looks like a valid case.
I'll be merging it to my original patchset, seeing that
James _still_ hasn't responded to it, let alone merged it.

Sigh.

Cheers,

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

Re: [PATCH] scsi: Set the minimum valid value of 'eh_deadline' as 0

[Ewan Milne]

On Wed, 2013-10-09 at 15:43 +0800, Ren Mingxin wrote:
> The former minimum valid value of 'eh_deadline' is 1s, which means
> the earliest occasion to shorten EH is 1 second later since a
> command is failed or timed out. But if we want to skip EH steps
> ASAP, we have to wait until the first EH step is finished. If the
> duration of the first EH step is long, this waiting time is
> excruciating. So, it is necessary to accept 0 as the minimum valid
> value for 'eh_deadline'.
> 
> According to my test, with Hannes' patchset 'New EH command timeout
> handler' as well, the minimum IO time is improved from 73s
> (eh_deadline = 1) to 43s(eh_deadline = 0) when commands are timed
> out by disabling RSCN and target port.
> 
> Another thing: scsi_finish_command() should be invoked if
> scsi_eh_scmd_add() is returned on failure - let EH finish those
> commands.
> 
> Signed-off-by: Ren Mingxin <renmx@cn.fujitsu.com>
> ---
>  drivers/scsi/hosts.c      |   14 +++++++++++---
>  drivers/scsi/scsi_error.c |   40 +++++++++++++++++++++++++++-------------
>  drivers/scsi/scsi_sysfs.c |   36 +++++++++++++++++++++++++-----------
>  include/scsi/scsi_host.h  |    2 +-
>  4 files changed, 64 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index f334859..e84123a 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -316,11 +316,11 @@ static void scsi_host_dev_release(struct device *dev)
>  	kfree(shost);
>  }
>  
> -static unsigned int shost_eh_deadline;
> +static unsigned int shost_eh_deadline = -1;

This should probably be "static int shost_eh_deadline = -1;".
And the range tests in scsi_host_alloc() and store_shost_eh_deadline()
below should probably use INT_MAX rather than UINT_MAX.

>  
>  module_param_named(eh_deadline, shost_eh_deadline, uint, S_IRUGO|S_IWUSR);
>  MODULE_PARM_DESC(eh_deadline,
> -		 "SCSI EH timeout in seconds (should be between 1 and 2^32-1)");
> +		 "SCSI EH timeout in seconds (should be between 0 and 2^32-1)");
>  
>  static struct device_type scsi_host_type = {
>  	.name =		"scsi_host",
> @@ -394,7 +394,15 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
>  	shost->use_clustering = sht->use_clustering;
>  	shost->ordered_tag = sht->ordered_tag;
> -	shost->eh_deadline = shost_eh_deadline * HZ;
> +	if (shost_eh_deadline == -1)
> +		shost->eh_deadline = -1;
> +	else if ((ulong) shost_eh_deadline * HZ > UINT_MAX) {
> +		printk(KERN_WARNING "scsi%d: eh_deadline %u exceeds the "
> +		       "maximum, setting to %u\n",
> +		       shost->host_no, shost_eh_deadline, UINT_MAX / HZ);
> +		shost->eh_deadline = UINT_MAX / HZ * HZ;

Just use "shost->eh_deadline = INT_MAX" here, leave off the "/ HZ * HZ".

> +	} else
> +		shost->eh_deadline = shost_eh_deadline * HZ;
>  
>  	if (sht->supported_mode == MODE_UNKNOWN)
>  		/* means we didn't set it ... default to INITIATOR */
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index adb4cbe..c2f9431 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(scsi_schedule_eh);
>  
>  static int scsi_host_eh_past_deadline(struct Scsi_Host *shost)
>  {
> -	if (!shost->last_reset || !shost->eh_deadline)
> +	if (!shost->last_reset || shost->eh_deadline == -1)
>  		return 0;
>  
>  	if (time_before(jiffies,
> @@ -127,29 +127,43 @@ scmd_eh_abort_handler(struct work_struct *work)
>  		rtn = scsi_try_to_abort_cmd(sdev->host->hostt, scmd);
>  		if (rtn == SUCCESS) {
>  			scmd->result |= DID_TIME_OUT << 16;
> -			if (!scsi_noretry_cmd(scmd) &&
> +			spin_lock_irqsave(sdev->host->host_lock, flags);
> +			if (scsi_host_eh_past_deadline(sdev->host)) {
> +				spin_unlock_irqrestore(sdev->host->host_lock,
> +						       flags);
> +				SCSI_LOG_ERROR_RECOVERY(3,
> +					scmd_printk(KERN_INFO, scmd,
> +						    "scmd %p eh timeout, "
> +						    "not retrying aborted "
> +						    "command\n", scmd));
> +			} else if (!scsi_noretry_cmd(scmd) &&
>  			    (++scmd->retries <= scmd->allowed)) {
> +				spin_unlock_irqrestore(sdev->host->host_lock,
> +						       flags);
>  				SCSI_LOG_ERROR_RECOVERY(3,
>  					scmd_printk(KERN_WARNING, scmd,
>  						    "scmd %p retry "
>  						    "aborted command\n", scmd));
>  				scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
> +				return;
>  			} else {
> +				spin_unlock_irqrestore(sdev->host->host_lock,
> +						       flags);
>  				SCSI_LOG_ERROR_RECOVERY(3,
>  					scmd_printk(KERN_WARNING, scmd,
>  						    "scmd %p finish "
>  						    "aborted command\n", scmd));
>  				scsi_finish_command(scmd);
> +				return;
>  			}
> -			return;
> -		}
> -		SCSI_LOG_ERROR_RECOVERY(3,
> -			scmd_printk(KERN_INFO, scmd,
> -				    "scmd %p abort failed, rtn %d\n",
> -				    scmd, rtn));
> +		} else
> +			SCSI_LOG_ERROR_RECOVERY(3,
> +				scmd_printk(KERN_INFO, scmd,
> +					    "scmd %p abort failed, rtn %d\n",
> +					    scmd, rtn));
>  	}
>  
> -	if (scsi_eh_scmd_add(scmd, 0)) {
> +	if (!scsi_eh_scmd_add(scmd, 0)) {
>  		SCSI_LOG_ERROR_RECOVERY(3,
>  			scmd_printk(KERN_WARNING, scmd,
>  				    "scmd %p terminate "
> @@ -197,7 +211,7 @@ scsi_abort_command(struct scsi_cmnd *scmd)
>  		return FAILED;
>  	}
>  
> -	if (shost->eh_deadline && !shost->last_reset)
> +	if (shost->eh_deadline != -1 && !shost->last_reset)
>  		shost->last_reset = jiffies;
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  
> @@ -231,7 +245,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
>  		if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
>  			goto out_unlock;
>  
> -	if (shost->eh_deadline && !shost->last_reset)
> +	if (shost->eh_deadline != -1 && !shost->last_reset)
>  		shost->last_reset = jiffies;
>  
>  	ret = 1;
> @@ -265,7 +279,7 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
>  	trace_scsi_dispatch_cmd_timeout(scmd);
>  	scsi_log_completion(scmd, TIMEOUT_ERROR);
>  
> -	if (host->eh_deadline && !host->last_reset)
> +	if (host->eh_deadline != -1 && !host->last_reset)
>  		host->last_reset = jiffies;
>  
>  	if (host->transportt->eh_timed_out)
> @@ -2130,7 +2144,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
>  			scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
>  
>  	spin_lock_irqsave(shost->host_lock, flags);
> -	if (shost->eh_deadline)
> +	if (shost->eh_deadline != -1)
>  		shost->last_reset = 0;
>  	spin_unlock_irqrestore(shost->host_lock, flags);
>  	scsi_eh_flush_done_q(&eh_done_q);
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 57c78ef..b0591aa 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -287,7 +287,9 @@ show_shost_eh_deadline(struct device *dev,
>  {
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  
> -	return sprintf(buf, "%d\n", shost->eh_deadline / HZ);
> +	if (shost->eh_deadline == -1)
> +		return snprintf(buf, strlen("off") + 2, "off\n");
> +	return sprintf(buf, "%u\n", shost->eh_deadline / HZ);
>  }
>  
>  static ssize_t
> @@ -296,22 +298,34 @@ store_shost_eh_deadline(struct device *dev, struct device_attribute *attr,
>  {
>  	struct Scsi_Host *shost = class_to_shost(dev);
>  	int ret = -EINVAL;
> -	int deadline;
> -	unsigned long flags;
> +	unsigned long deadline, flags;
> +	char *cp;
>  
>  	if (shost->transportt && shost->transportt->eh_strategy_handler)
>  		return ret;
>  
> -	if (sscanf(buf, "%d\n", &deadline) == 1) {
> -		spin_lock_irqsave(shost->host_lock, flags);
> -		if (scsi_host_in_recovery(shost))
> -			ret = -EBUSY;
> -		else {
> +	if (!strncmp(buf, "off", strlen("off")))
> +		deadline = -1;
> +	else {
> +		deadline = simple_strtoul(buf, &cp, 0);
> +		if ((*cp && (*cp != '\n')) ||
> +		    deadline * HZ > UINT_MAX)
> +			return ret;
> +	}
> +
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	if (scsi_host_in_recovery(shost))
> +		ret = -EBUSY;
> +	else {
> +		if (deadline == -1)
> +			shost->eh_deadline = -1;
> +		else
>  			shost->eh_deadline = deadline * HZ;
> -			ret = count;
> -		}
> -		spin_unlock_irqrestore(shost->host_lock, flags);
> +
> +		ret = count;
>  	}
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +
>  	return ret;
>  }
>  
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 6ca9174..b50355c 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -603,7 +603,7 @@ struct Scsi_Host {
>  	unsigned int host_eh_scheduled;    /* EH scheduled without command */
>  
>  	unsigned int host_no;  /* Used for IOCTL_GET_IDLUN, /proc/scsi et al. */
> -	int eh_deadline;
> +	unsigned int eh_deadline;
>  	unsigned long last_reset;
>  
>  	/*

Re: [PATCH] scsi: Set the minimum valid value of 'eh_deadline' as 0

[Ren Mingxin]

Hi, Ewan, Hannes:

On 10/09/2013 08:28 PM, Ewan Milne wrote:
> On Wed, 2013-10-09 at 15:43 +0800, Ren Mingxin wrote:
>> The former minimum valid value of 'eh_deadline' is 1s, which means
>> the earliest occasion to shorten EH is 1 second later since a
>> command is failed or timed out. But if we want to skip EH steps
>> ASAP, we have to wait until the first EH step is finished. If the
>> duration of the first EH step is long, this waiting time is
>> excruciating. So, it is necessary to accept 0 as the minimum valid
>> value for 'eh_deadline'.
>>
>> According to my test, with Hannes' patchset 'New EH command timeout
>> handler' as well, the minimum IO time is improved from 73s
>> (eh_deadline = 1) to 43s(eh_deadline = 0) when commands are timed
>> out by disabling RSCN and target port.
>>
>> Another thing: scsi_finish_command() should be invoked if
>> scsi_eh_scmd_add() is returned on failure - let EH finish those
>> commands.
>>
>> Signed-off-by: Ren Mingxin<renmx@cn.fujitsu.com>
>> ---
>>   drivers/scsi/hosts.c      |   14 +++++++++++---
>>   drivers/scsi/scsi_error.c |   40 +++++++++++++++++++++++++++-------------
>>   drivers/scsi/scsi_sysfs.c |   36 +++++++++++++++++++++++++-----------
>>   include/scsi/scsi_host.h  |    2 +-
>>   4 files changed, 64 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index f334859..e84123a 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -316,11 +316,11 @@ static void scsi_host_dev_release(struct device *dev)
>>   	kfree(shost);
>>   }
>>
>> -static unsigned int shost_eh_deadline;
>> +static unsigned int shost_eh_deadline = -1;
>
> This should probably be "static int shost_eh_deadline = -1;".
> And the range tests in scsi_host_alloc() and store_shost_eh_deadline()
> below should probably use INT_MAX rather than UINT_MAX.

The maximum value is decreased then.
Hannes, agree?

>>
>>   module_param_named(eh_deadline, shost_eh_deadline, uint, S_IRUGO|S_IWUSR);
>>   MODULE_PARM_DESC(eh_deadline,
>> -		 "SCSI EH timeout in seconds (should be between 1 and 2^32-1)");
>> +		 "SCSI EH timeout in seconds (should be between 0 and 2^32-1)");

And the description above should be modified as:

+		"SCSI EH timeout in seconds (should be between 0 and (2^31-1)/HZ)");


>>
>>
>>   static struct device_type scsi_host_type = {
>>   	.name =		"scsi_host",
>> @@ -394,7 +394,15 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>>   	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
>>   	shost->use_clustering = sht->use_clustering;
>>   	shost->ordered_tag = sht->ordered_tag;
>> -	shost->eh_deadline = shost_eh_deadline * HZ;
>> +	if (shost_eh_deadline == -1)
>> +		shost->eh_deadline = -1;
>> +	else if ((ulong) shost_eh_deadline * HZ>  UINT_MAX) {
>> +		printk(KERN_WARNING "scsi%d: eh_deadline %u exceeds the "
>> +		       "maximum, setting to %u\n",
>> +		       shost->host_no, shost_eh_deadline, UINT_MAX / HZ);
>> +		shost->eh_deadline = UINT_MAX / HZ * HZ;
>
> Just use "shost->eh_deadline = INT_MAX" here, leave off the "/ HZ * HZ".

Nod.

Thanks,
Ren

Re: [PATCH 1/3] scsi: Fix erratic device offline during EH

[James Bottomley]

On Mon, 2013-09-02 at 13:58 +0200, Hannes Reinecke wrote:
> Commit 18a4d0a22ed6c54b67af7718c305cd010f09ddf8
> (Handle disk devices which can not process medium access commands)
> was introduced to offline any device which cannot process medium
> access commands.
> However, commit 3eef6257de48ff84a5d98ca533685df8a3beaeb8
> (Reduce error recovery time by reducing use of TURs) reduced
> the number of TURs by sending it only on the first failing
> command, which might or might not be a medium access command.
> So in combination this results in an erratic device offlining
> during EH; if the command where the TUR was sent upon happens
> to be a medium access command the device will be set offline,
> if not everything proceeds as normal.
> 
> So instead of checking the EH command in the ->eh_action
> callback we should rather call ->eh_action when we're
> about to finish the command _and_ have sent a TUR previously.
> This should then set the device offline as advertised.
> 
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Ewan Milne <emilne@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_error.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index abf0916..c88cb7e 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -941,12 +941,6 @@ retry:
>  
>  	scsi_eh_restore_cmnd(scmd, &ses);
>  
> -	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> -		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
> -		if (sdrv->eh_action)
> -			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
> -	}
> -
>  	return rtn;
>  }
>  
> @@ -964,6 +958,18 @@ static int scsi_request_sense(struct scsi_cmnd *scmd)
>  	return scsi_send_eh_cmnd(scmd, NULL, 0, scmd->device->eh_timeout, ~0);
>  }
>  
> +static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
> +{
> +	static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
> +
> +	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
> +		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
> +		if (sdrv->eh_action)
> +			rtn = sdrv->eh_action(scmd, tur_command, 6, rtn);

This is all a bit pointless.  You've altered eh_action so it's always
input an eh TUR command, so just eliminate the check of the eh command
and assume it's a TUR in the implementation (i.e. fix up sd.c)

Once that's done, I think the patch looks like the one below, is that
OK?

I still have qualms about the media access check because what can happen
is that we abort, TUR succeeds then the next Media access command fails,
we abort, TUR succeeds etc until the media access timeout goes off.  The
problem is that it never gets escalated to a reset which might fix the
condition.  However, this is beyond the scope of the current patch set.

James

---
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 83e591b..aef80f1 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -923,12 +923,6 @@ retry:
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
-		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
-		if (sdrv->eh_action)
-			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
-	}
-
 	return rtn;
 }
 
@@ -946,6 +940,16 @@ static int scsi_request_sense(struct scsi_cmnd *scmd)
 	return scsi_send_eh_cmnd(scmd, NULL, 0, scmd->device->eh_timeout, ~0);
 }
 
+static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
+{
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, rtn);
+	}
+	return rtn;
+}
+
 /**
  * scsi_eh_finish_cmd - Handle a cmd that eh is finished with.
  * @scmd:	Original SCSI cmd that eh has finished.
@@ -1094,7 +1098,9 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
 
 		list_for_each_entry_safe(scmd, next, cmd_list, eh_entry)
 			if (scmd->device == sdev) {
-				if (finish_cmds)
+				if (finish_cmds &&
+				    (try_stu ||
+				     scsi_eh_action(scmd, SUCCESS) == SUCCESS))
 					scsi_eh_finish_cmd(scmd, done_q);
 				else
 					list_move_tail(&scmd->eh_entry, work_q);
@@ -1208,7 +1214,8 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
 			    !scsi_eh_tur(stu_scmd)) {
 				list_for_each_entry_safe(scmd, next,
 							  work_q, eh_entry) {
-					if (scmd->device == sdev)
+					if (scmd->device == sdev &&
+					    scsi_eh_action(scmd, SUCCESS) == SUCCESS)
 						scsi_eh_finish_cmd(scmd, done_q);
 				}
 			}
@@ -1264,7 +1271,8 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
 			    !scsi_eh_tur(bdr_scmd)) {
 				list_for_each_entry_safe(scmd, next,
 							 work_q, eh_entry) {
-					if (scmd->device == sdev)
+					if (scmd->device == sdev &&
+					    scsi_eh_action(scmd, rtn) != FAILED)
 						scsi_eh_finish_cmd(scmd,
 								   done_q);
 				}
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..d037391 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -109,7 +109,7 @@ static int sd_suspend(struct device *);
 static int sd_resume(struct device *);
 static void sd_rescan(struct device *);
 static int sd_done(struct scsi_cmnd *);
-static int sd_eh_action(struct scsi_cmnd *, unsigned char *, int, int);
+static int sd_eh_action(struct scsi_cmnd *, int);
 static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
 static void scsi_disk_release(struct device *cdev);
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
@@ -1530,23 +1530,23 @@ static const struct block_device_operations sd_fops = {
 /**
  *	sd_eh_action - error handling callback
  *	@scmd:		sd-issued command that has failed
- *	@eh_cmnd:	The command that was sent during error handling
- *	@eh_cmnd_len:	Length of eh_cmnd in bytes
  *	@eh_disp:	The recovery disposition suggested by the midlayer
  *
- *	This function is called by the SCSI midlayer upon completion of
- *	an error handling command (TEST UNIT READY, START STOP UNIT,
- *	etc.) The command sent to the device by the error handler is
- *	stored in eh_cmnd. The result of sending the eh command is
- *	passed in eh_disp.
+ *	This function is called by the SCSI midlayer upon completion of an
+ *	error test command (currently TEST UNIT READY). The result of sending
+ *	the eh command is passed in eh_disp.  We're looking for devices that
+ *	fail medium access commands but are OK with non access commands like
+ *	test unit ready (so wrongly see the device as having a successful
+ *	recovery)
  **/
-static int sd_eh_action(struct scsi_cmnd *scmd, unsigned char *eh_cmnd,
-			int eh_cmnd_len, int eh_disp)
+static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
 {
 	struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
 
 	if (!scsi_device_online(scmd->device) ||
-	    !scsi_medium_access_command(scmd))
+	    !scsi_medium_access_command(scmd) ||
+	    host_byte(scmd->result) != DID_TIME_OUT ||
+	    eh_disp != SUCCESS)
 		return eh_disp;
 
 	/*
@@ -1556,9 +1556,7 @@ static int sd_eh_action(struct scsi_cmnd *scmd, unsigned char *eh_cmnd,
 	 * process of recovering or has it suffered an internal failure
 	 * that prevents access to the storage medium.
 	 */
-	if (host_byte(scmd->result) == DID_TIME_OUT && eh_disp == SUCCESS &&
-	    eh_cmnd_len && eh_cmnd[0] == TEST_UNIT_READY)
-		sdkp->medium_access_timed_out++;
+	sdkp->medium_access_timed_out++;
 
 	/*
 	 * If the device keeps failing read/write commands but TEST UNIT
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index d443aa0..20fdfc2 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -16,7 +16,7 @@ struct scsi_driver {
 
 	void (*rescan)(struct device *);
 	int (*done)(struct scsi_cmnd *);
-	int (*eh_action)(struct scsi_cmnd *, unsigned char *, int, int);
+	int (*eh_action)(struct scsi_cmnd *, int);
 };
 #define to_scsi_driver(drv) \
 	container_of((drv), struct scsi_driver, gendrv)

Re: [PATCH 1/3] scsi: Fix erratic device offline during EH

[Martin K. Petersen]

>>>>> "James" == James Bottomley <jbottomley@parallels.com> writes:

James> Once that's done, I think the patch looks like the one below, is
James> that OK?

Looks OK to me.


James> I still have qualms about the media access check because what can
James> happen is that we abort, TUR succeeds then the next Media access
James> command fails, we abort, TUR succeeds etc until the media access
James> timeout goes off.  The problem is that it never gets escalated to
James> a reset which might fix the condition.

All the cases we had in the field involved devices where the command
processing part of the device worked fine but the media access became
impossible (think wedged read/write head assembly, dead FTL on an SSD).

I haven't seen any cases where a device would respond to SPC commands
while failing all media accesses and then be able to resume media
accesses after a reset. Doesn't mean that such a device doesn't exist,
of course, but the patch was originally done to handle the "irreparably
broken" error case.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/3] scsi: Fix erratic device offline during EH

[Hannes Reinecke]

On 10/16/2013 09:22 PM, James Bottomley wrote:
> On Mon, 2013-09-02 at 13:58 +0200, Hannes Reinecke wrote:
>> Commit 18a4d0a22ed6c54b67af7718c305cd010f09ddf8
>> (Handle disk devices which can not process medium access commands)
>> was introduced to offline any device which cannot process medium
>> access commands.
>> However, commit 3eef6257de48ff84a5d98ca533685df8a3beaeb8
>> (Reduce error recovery time by reducing use of TURs) reduced
>> the number of TURs by sending it only on the first failing
>> command, which might or might not be a medium access command.
>> So in combination this results in an erratic device offlining
>> during EH; if the command where the TUR was sent upon happens
>> to be a medium access command the device will be set offline,
>> if not everything proceeds as normal.
>>
>> So instead of checking the EH command in the ->eh_action
>> callback we should rather call ->eh_action when we're
>> about to finish the command _and_ have sent a TUR previously.
>> This should then set the device offline as advertised.
>>
>> Cc: Martin K. Petersen <martin.petersen@oracle.com>
>> Cc: Ewan Milne <emilne@redhat.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/scsi_error.c | 28 +++++++++++++++++++---------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index abf0916..c88cb7e 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -941,12 +941,6 @@ retry:
>>
>>   	scsi_eh_restore_cmnd(scmd, &ses);
>>
>> -	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>> -		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
>> -		if (sdrv->eh_action)
>> -			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
>> -	}
>> -
>>   	return rtn;
>>   }
>>
>> @@ -964,6 +958,18 @@ static int scsi_request_sense(struct scsi_cmnd *scmd)
>>   	return scsi_send_eh_cmnd(scmd, NULL, 0, scmd->device->eh_timeout, ~0);
>>   }
>>
>> +static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
>> +{
>> +	static unsigned char tur_command[6] = {TEST_UNIT_READY, 0, 0, 0, 0, 0};
>> +
>> +	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
>> +		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
>> +		if (sdrv->eh_action)
>> +			rtn = sdrv->eh_action(scmd, tur_command, 6, rtn);
>
> This is all a bit pointless.  You've altered eh_action so it's always
> input an eh TUR command, so just eliminate the check of the eh command
> and assume it's a TUR in the implementation (i.e. fix up sd.c)
>
> Once that's done, I think the patch looks like the one below, is that
> OK?
>
Yes, the patch looks okay. No objections from my side.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

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