[PATCHv2 0/9] New SCSI command timeout handler

[PATCHv2 0/9] New SCSI command timeout handler

[Hannes Reinecke]

this is the first step towards a new non-blocking
error handler. This patch implements a new command
timeout handler which will be sending command aborts
inline without engaging SCSI EH.

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

With the original scsi error recovery I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync
2048+0 records in
2048+0 records out
1048576 bytes (1.0 MB) copied, 3.72732 s, 281 kB/s

real	2m14.475s
user	0m0.000s
sys	0m0.104s

with this patchset I got:
# time dd if=/dev/zero of=/mnt/test.blk bs=512 count=2048 oflag=sync
2048+0 records in
2048+0 records out
1048576 bytes (1.0 MB) copied, 31.5151 s, 33.3 kB/s

real	0m31.519s
user	0m0.000s
sys	0m0.088s

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

As a proof-of-concept I've also enabled the new timeout
handler for virtio, so that things can be tested out
more easily.
As requested I've also hooked in the new SCSI timeout
handler to SAS.

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.

Comments etc are welcome.

Hannes Reinecke (9):
  scsi: move initialization of scmd->eh_entry
  blk-timeout: add BLK_EH_SCHEDULED return code
  scsi: improved eh timeout handler
  virtio_scsi: Enable new EH timeout handler
  virtio-scsi: Implement TMF timeout
  libsas: Enable new EH timeout handler
  mptsas: Enable new EH timeout handler
  mpt2sas: Enable new EH timeout handler
  mpt3sas: Enable new EH timeout handler

 drivers/message/fusion/mptsas.c      |   1 +
 drivers/message/fusion/mptscsih.c    |   7 ++
 drivers/message/fusion/mptscsih.h    |   1 +
 drivers/scsi/libsas/sas_scsi_host.c  |   2 +-
 drivers/scsi/mpt2sas/mpt2sas_scsih.c |  11 +++
 drivers/scsi/mpt3sas/mpt3sas_scsih.c |  11 +++
 drivers/scsi/scsi_error.c            | 126 ++++++++++++++++++++++++++++++++++-
 drivers/scsi/scsi_lib.c              |   4 +-
 drivers/scsi/scsi_scan.c             |   3 +
 drivers/scsi/scsi_sysfs.c            |   5 ++
 drivers/scsi/scsi_transport_fc.c     |   2 +-
 drivers/scsi/virtio_scsi.c           |  14 +++-
 include/linux/blkdev.h               |   1 +
 include/scsi/scsi_cmnd.h             |   1 +
 include/scsi/scsi_device.h           |   2 +
 15 files changed, 184 insertions(+), 7 deletions(-)

-- 
1.7.12.4

[PATCH 1/9] scsi: move initialization of scmd->eh_entry

[Hannes Reinecke]

The 'eh_entry' list might be used even before scsi_softirq_done()
is called. Hence we should rather initialize it together with
the other eh-related variables.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 86d5220..c1774de 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -317,6 +317,8 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd)
 	memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
 	if (cmd->cmd_len == 0)
 		cmd->cmd_len = scsi_command_size(cmd->cmnd);
+	INIT_LIST_HEAD(&cmd->eh_entry);
+	cmd->eh_eflags = 0;
 }
 
 void scsi_device_unbusy(struct scsi_device *sdev)
@@ -1482,8 +1484,6 @@ static void scsi_softirq_done(struct request *rq)
 	unsigned long wait_for = (cmd->allowed + 1) * rq->timeout;
 	int disposition;
 
-	INIT_LIST_HEAD(&cmd->eh_entry);
-
 	atomic_inc(&cmd->device->iodone_cnt);
 	if (cmd->result)
 		atomic_inc(&cmd->device->ioerr_cnt);
-- 
1.7.12.4

[PATCH 2/9] blk-timeout: add BLK_EH_SCHEDULED return code

[Hannes Reinecke]

Add a 'BLK_EH_SCHEDULED' return code for blk-timeout to indicate
that a delayed error recovery has been initiated.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_error.c | 3 +++
 include/linux/blkdev.h    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f43de1e..96b4bb6 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -146,6 +146,9 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
 		rtn = host->hostt->eh_timed_out(scmd);
 
 	scmd->result |= DID_TIME_OUT << 16;
+	/* Check for delayed EH scheduling */
+	if (rtn == BLK_EH_SCHEDULED)
+		return BLK_EH_NOT_HANDLED;
 
 	if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
 		     !scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD)))
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 2fdb4a4..d846e2b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -238,6 +238,7 @@ enum blk_eh_timer_return {
 	BLK_EH_NOT_HANDLED,
 	BLK_EH_HANDLED,
 	BLK_EH_RESET_TIMER,
+	BLK_EH_SCHEDULED,
 };
 
 typedef enum blk_eh_timer_return (rq_timed_out_fn)(struct request *);
-- 
1.7.12.4

[PATCH 3/9] 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 'eh_abort_handler'.

If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
command will be retried if possible. If no retries are allowed
the command will be returned immediately, as we have to assume
the TMF succeeded and the command is completed with the LLDD.
If the TMF fails the command will be pushed back onto the
list of failed commands and the SCSI EH handler will be
called immediately for all timed-out commands.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_error.c        | 123 ++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/scsi_scan.c         |   3 +
 drivers/scsi/scsi_sysfs.c        |   5 ++
 drivers/scsi/scsi_transport_fc.c |   2 +-
 include/scsi/scsi_cmnd.h         |   1 +
 include/scsi/scsi_device.h       |   2 +
 6 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 96b4bb6..467cb3c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -55,6 +55,8 @@ 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 *hostt,
+				 struct scsi_cmnd *scmd);
 
 /* called with shost->host_lock held */
 void scsi_eh_wakeup(struct Scsi_Host *shost)
@@ -90,6 +92,125 @@ void scsi_schedule_eh(struct Scsi_Host *shost)
 EXPORT_SYMBOL_GPL(scsi_schedule_eh);
 
 /**
+ * scsi_eh_abort_handler - Handle command aborts
+ * @work:	sdev on which commands should be aborted.
+ */
+void
+scsi_eh_abort_handler(struct work_struct *work)
+{
+	struct scsi_device *sdev =
+		container_of(work, struct scsi_device, abort_work);
+	struct scsi_cmnd *scmd, *tmp;
+	LIST_HEAD(abort_list);
+	unsigned long flags;
+	int rtn;
+
+	spin_lock_irqsave(&sdev->list_lock, flags);
+	list_splice_init(&sdev->eh_abort_list, &abort_list);
+	spin_unlock_irqrestore(&sdev->list_lock, flags);
+
+	list_for_each_entry_safe(scmd, tmp, &abort_list, eh_entry) {
+		list_del_init(&scmd->eh_entry);
+		if (sdev->sdev_state == SDEV_CANCEL) {
+			SCSI_LOG_ERROR_RECOVERY(3,
+				scmd_printk(KERN_INFO, scmd,
+				    "terminate, device removed\n"));
+			scmd->result |= DID_NO_CONNECT << 16;
+			scsi_finish_command(scmd);
+			continue;
+		}
+		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 == FAILED) {
+			SCSI_LOG_ERROR_RECOVERY(3,
+				scmd_printk(KERN_INFO, scmd,
+					     "abort command failed\n"));
+			list_move_tail(&scmd->eh_entry, &abort_list);
+			goto start_eh;
+		}
+		if (!(scmd->request->cmd_flags & REQ_FAILFAST_DEV) &&
+		    (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) &&
+		    (++scmd->retries <= scmd->allowed)) {
+			SCSI_LOG_ERROR_RECOVERY(3,
+				scmd_printk(KERN_WARNING, scmd,
+					    "retry aborted command\n"));
+
+			scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
+		} else {
+			SCSI_LOG_ERROR_RECOVERY(3,
+				scmd_printk(KERN_WARNING, scmd,
+					    "fast fail aborted command\n"));
+			scmd->result |= DID_TRANSPORT_FAILFAST << 16;
+			scsi_finish_command(scmd);
+		}
+	}
+
+	if (list_empty(&abort_list))
+		return;
+
+start_eh:
+	list_for_each_entry_safe(scmd, tmp, &abort_list, eh_entry) {
+		scmd->result |= DID_TIME_OUT << 16;
+		if (!scsi_eh_scmd_add(scmd, 0)) {
+			SCSI_LOG_ERROR_RECOVERY(3,
+				scmd_printk(KERN_WARNING, scmd,
+					    "terminate aborted command\n"));
+			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
+ */
+enum blk_eh_timer_return
+scsi_abort_command(struct scsi_cmnd *scmd)
+{
+	unsigned long flags;
+	int kick_worker = 0;
+	struct scsi_device *sdev = scmd->device;
+
+	/*
+	 * Do not try a command abort if
+	 * SCSI EH has already started.
+	 */
+	if (scsi_host_in_recovery(sdev->host)) {
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "host in recovery, not aborting\n"));
+		scmd->result |= DID_TIME_OUT << 16;
+		scsi_eh_scmd_add(scmd, 0);
+		return BLK_EH_SCHEDULED;
+	}
+	if (sdev->sdev_state == SDEV_CANCEL ||
+	    sdev->sdev_state == SDEV_OFFLINE) {
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "device removed, terminating command\n"));
+		scmd->result |= DID_NO_CONNECT << 16;
+		scsi_finish_command(scmd);
+		return BLK_EH_SCHEDULED;
+	}
+
+	spin_lock_irqsave(&sdev->list_lock, flags);
+	if (list_empty(&sdev->eh_abort_list))
+		kick_worker = 1;
+	list_add(&scmd->eh_entry, &sdev->eh_abort_list);
+	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	SCSI_LOG_ERROR_RECOVERY(3,
+		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
+	if (kick_worker)
+		schedule_work(&sdev->abort_work);
+	return BLK_EH_SCHEDULED;
+}
+EXPORT_SYMBOL_GPL(scsi_abort_command);
+
+/**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
  * @eh_flag:	optional SCSI_EH flag.
@@ -145,11 +266,11 @@ 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);
 
-	scmd->result |= DID_TIME_OUT << 16;
 	/* Check for delayed EH scheduling */
 	if (rtn == BLK_EH_SCHEDULED)
 		return BLK_EH_NOT_HANDLED;
 
+	scmd->result |= DID_TIME_OUT << 16;
 	if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
 		     !scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD)))
 		rtn = BLK_EH_HANDLED;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 3e58b22..f9cc6fc 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -231,6 +231,7 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	extern void scsi_evt_thread(struct work_struct *work);
 	extern void scsi_requeue_run_queue(struct work_struct *work);
+	extern void scsi_eh_abort_handler(struct work_struct *work);
 
 	sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
 		       GFP_ATOMIC);
@@ -251,9 +252,11 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	INIT_LIST_HEAD(&sdev->cmd_list);
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
+	INIT_LIST_HEAD(&sdev->eh_abort_list);
 	spin_lock_init(&sdev->list_lock);
 	INIT_WORK(&sdev->event_work, scsi_evt_thread);
 	INIT_WORK(&sdev->requeue_work, scsi_requeue_run_queue);
+	INIT_WORK(&sdev->abort_work, scsi_eh_abort_handler);
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
 	sdev->sdev_target = starget;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 931a7d9..af64c1c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -966,6 +966,11 @@ void __scsi_remove_device(struct scsi_device *sdev)
 		put_device(&sdev->sdev_dev);
 
 	/*
+	 * Terminate timed-out commands
+	 */
+	flush_work(&sdev->abort_work);
+
+	/*
 	 * Stop accepting new requests and wait until all queuecommand() and
 	 * scsi_run_queue() invocations have finished before tearing down the
 	 * device.
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index e106c27..1e1de9f 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2079,7 +2079,7 @@ fc_timed_out(struct scsi_cmnd *scmd)
 	if (rport->port_state == FC_PORTSTATE_BLOCKED)
 		return BLK_EH_RESET_TIMER;
 
-	return BLK_EH_NOT_HANDLED;
+	return scsi_abort_command(scmd);
 }
 
 /*
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de5f5d8..d521694 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -144,6 +144,7 @@ extern void scsi_put_command(struct scsi_cmnd *);
 extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
 			       struct device *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
+extern enum blk_eh_timer_return scsi_abort_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index cc64587..e03d379 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -80,6 +80,7 @@ struct scsi_device {
 	spinlock_t list_lock;
 	struct list_head cmd_list;	/* queue of in use SCSI Command structures */
 	struct list_head starved_entry;
+	struct list_head eh_abort_list;
 	struct scsi_cmnd *current_cmnd;	/* currently active command */
 	unsigned short queue_depth;	/* How deep of a queue we want */
 	unsigned short max_queue_depth;	/* max queue depth */
@@ -180,6 +181,7 @@ struct scsi_device {
 
 	struct execute_work	ew; /* used to get process context on put */
 	struct work_struct	requeue_work;
+	struct work_struct	abort_work;
 
 	struct scsi_dh_data	*scsi_dh_data;
 	enum scsi_device_state sdev_state;
-- 
1.7.12.4

[PATCH 4/9] virtio_scsi: Enable new EH timeout handler

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/virtio_scsi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 2168258..1efd219 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -654,6 +654,11 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
 	return virtscsi_tmf(vscsi, cmd);
 }
 
+static enum blk_eh_timer_return virtscsi_timedout(struct scsi_cmnd *scmd)
+{
+	return scsi_abort_command(scmd);
+}
+
 static int virtscsi_target_alloc(struct scsi_target *starget)
 {
 	struct virtio_scsi_target_state *tgt =
@@ -683,6 +688,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
 	.queuecommand = virtscsi_queuecommand_single,
 	.eh_abort_handler = virtscsi_abort,
 	.eh_device_reset_handler = virtscsi_device_reset,
+	.eh_timed_out = virtscsi_timedout,
 
 	.can_queue = 1024,
 	.dma_boundary = UINT_MAX,
@@ -699,6 +705,7 @@ static struct scsi_host_template virtscsi_host_template_multi = {
 	.queuecommand = virtscsi_queuecommand_multi,
 	.eh_abort_handler = virtscsi_abort,
 	.eh_device_reset_handler = virtscsi_device_reset,
+	.eh_timed_out = virtscsi_timedout,
 
 	.can_queue = 1024,
 	.dma_boundary = UINT_MAX,
-- 
1.7.12.4

[PATCH 5/9] virtio-scsi: Implement TMF timeout

[Hannes Reinecke]

Any TMF might be take longer as expected, or not return at all.
So we need to use 'wait_for_completion_timeout' when sending
a TMF to protect against these cases.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/virtio_scsi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 1efd219..abfc684 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -30,6 +30,7 @@
 #define VIRTIO_SCSI_MEMPOOL_SZ 64
 #define VIRTIO_SCSI_EVENT_LEN 8
 #define VIRTIO_SCSI_VQ_BASE 2
+#define VIRTIO_SCSI_TMF_TIMEOUT 30
 
 /* Command queue element */
 struct virtio_scsi_cmd {
@@ -597,8 +598,10 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
 			      GFP_NOIO) < 0)
 		goto out;
 
-	wait_for_completion(&comp);
-	if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
+	if (wait_for_completion_timeout(&comp,
+					VIRTIO_SCSI_TMF_TIMEOUT * HZ) == 0)
+		ret = FAILED;
+	else if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
 	    cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
 		ret = SUCCESS;
 
-- 
1.7.12.4

[PATCH 6/9] libsas: Enable new EH timeout handler

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/libsas/sas_scsi_host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 6e795a1..9d5bd29 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -864,7 +864,7 @@ enum blk_eh_timer_return sas_scsi_timed_out(struct scsi_cmnd *cmd)
 {
 	scmd_printk(KERN_DEBUG, cmd, "command %p timed out\n", cmd);
 
-	return BLK_EH_NOT_HANDLED;
+	return scsi_abort_command(cmd);
 }
 
 int sas_ioctl(struct scsi_device *sdev, int cmd, void __user *arg)
-- 
1.7.12.4

[PATCH 7/9] mptsas: Enable new EH timeout handler

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/message/fusion/mptsas.c   | 1 +
 drivers/message/fusion/mptscsih.c | 7 +++++++
 drivers/message/fusion/mptscsih.h | 1 +
 3 files changed, 9 insertions(+)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index dd239bd..ed56434 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1987,6 +1987,7 @@ static struct scsi_host_template mptsas_driver_template = {
 	.target_destroy			= mptsas_target_destroy,
 	.slave_destroy			= mptscsih_slave_destroy,
 	.change_queue_depth 		= mptscsih_change_queue_depth,
+	.eh_timed_out			= mptscsih_timed_out,
 	.eh_abort_handler		= mptscsih_abort,
 	.eh_device_reset_handler	= mptscsih_dev_reset,
 	.eh_host_reset_handler		= mptscsih_host_reset,
diff --git a/drivers/message/fusion/mptscsih.c b/drivers/message/fusion/mptscsih.c
index 727819c..e743e84 100644
--- a/drivers/message/fusion/mptscsih.c
+++ b/drivers/message/fusion/mptscsih.c
@@ -1679,6 +1679,13 @@ mptscsih_get_tm_timeout(MPT_ADAPTER *ioc)
 	}
 }
 
+enum blk_eh_timer_return
+mptscsih_timed_out(struct scsi_cmnd *SCpnt)
+{
+	return scsi_abort_command(SCpnt);
+}
+EXPORT_SYMBOL(mptscsih_timed_out);
+
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
 /**
  *	mptscsih_abort - Abort linux scsi_cmnd routine, new_eh variant
diff --git a/drivers/message/fusion/mptscsih.h b/drivers/message/fusion/mptscsih.h
index 83f5031..3f2dd05 100644
--- a/drivers/message/fusion/mptscsih.h
+++ b/drivers/message/fusion/mptscsih.h
@@ -118,6 +118,7 @@ extern int mptscsih_IssueTaskMgmt(MPT_SCSI_HOST *hd, u8 type, u8 channel,
 	u8 id, int lun, int ctx2abort, ulong timeout);
 extern void mptscsih_slave_destroy(struct scsi_device *device);
 extern int mptscsih_slave_configure(struct scsi_device *device);
+enum blk_eh_timer_return mptscsih_timed_out(struct scsi_cmnd *SCpnt);
 extern int mptscsih_abort(struct scsi_cmnd * SCpnt);
 extern int mptscsih_dev_reset(struct scsi_cmnd * SCpnt);
 extern int mptscsih_bus_reset(struct scsi_cmnd * SCpnt);
-- 
1.7.12.4

[PATCH 8/9] mpt2sas: Enable new EH timeout handler

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/mpt2sas/mpt2sas_scsih.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index c6bdc92..47fc66c 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -2565,6 +2565,16 @@ _scsih_tm_display_info(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd)
 }
 
 /**
+ * _scsih_timed_out - eh timeout handler
+ * @scmd: pointer to scsi command object
+ */
+static enum blk_eh_timer_return
+_scsih_timed_out(struct scsi_cmnd *scmd)
+{
+	return scsi_abort_command(scmd);
+}
+
+/**
  * _scsih_abort - eh threads main abort routine
  * @scmd: pointer to scsi command object
  *
@@ -7515,6 +7525,7 @@ static struct scsi_host_template scsih_driver_template = {
 	.scan_start			= _scsih_scan_start,
 	.change_queue_depth 		= _scsih_change_queue_depth,
 	.change_queue_type		= _scsih_change_queue_type,
+	.eh_timed_out			= _scsih_timed_out,
 	.eh_abort_handler		= _scsih_abort,
 	.eh_device_reset_handler	= _scsih_dev_reset,
 	.eh_target_reset_handler	= _scsih_target_reset,
-- 
1.7.12.4

[PATCH 9/9] mpt3sas: Enable new EH timeout handler

[Hannes Reinecke]

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
index dcbf7c8..3a35ddc 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
@@ -2228,6 +2228,16 @@ _scsih_tm_display_info(struct MPT3SAS_ADAPTER *ioc, struct scsi_cmnd *scmd)
 }
 
 /**
+ * _scsih_timed_out - eh timeout handler
+ * @scmd: pointer to scsi command object
+ */
+static enum blk_eh_timer_return
+_scsih_timed_out(struct scsi_cmnd *scmd)
+{
+	return scsi_abort_command(scmd);
+}
+
+/**
  * _scsih_abort - eh threads main abort routine
  * @scmd: pointer to scsi command object
  *
@@ -7230,6 +7240,7 @@ static struct scsi_host_template scsih_driver_template = {
 	.scan_start			= _scsih_scan_start,
 	.change_queue_depth		= _scsih_change_queue_depth,
 	.change_queue_type		= _scsih_change_queue_type,
+	.eh_timed_out			= _scsih_timed_out,
 	.eh_abort_handler		= _scsih_abort,
 	.eh_device_reset_handler	= _scsih_dev_reset,
 	.eh_target_reset_handler	= _scsih_target_reset,
-- 
1.7.12.4

Re: [PATCH 1/9] scsi: move initialization of scmd->eh_entry

[Christoph Hellwig]

On Mon, Jun 10, 2013 at 09:40:50AM +0200, Hannes Reinecke wrote:
> The 'eh_entry' list might be used even before scsi_softirq_done()
> is called. Hence we should rather initialize it together with
> the other eh-related variables.

As mentioned earlier I don't think moving the initialization from one
random point in the middle of the lifetime of the command to another
is a good idea.  If we initialize it it should be when it is allocated.

But looking at the code I can't see why it needs to be initialized at
all.  eh_entry is not used as the head of a list but just as the list
link, and it's never tested for emptyness either, which shouldn't
require it to be initialized at all.

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

[Christoph Hellwig]

On Mon, Jun 10, 2013 at 09:40:52AM +0200, Hannes Reinecke wrote:
> 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 'eh_abort_handler'.
> 
> If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
> command will be retried if possible. If no retries are allowed
> the command will be returned immediately, as we have to assume
> the TMF succeeded and the command is completed with the LLDD.
> If the TMF fails the command will be pushed back onto the
> list of failed commands and the SCSI EH handler will be
> called immediately for all timed-out commands.

Why can't we use a work item per command?  Linking things into a list
just to queue it up to workqueues missed half of the point of the
workqueue infrastructure.

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

[Hannes Reinecke]

On 06/10/2013 10:20 AM, Christoph Hellwig wrote:
> On Mon, Jun 10, 2013 at 09:40:52AM +0200, Hannes Reinecke wrote:
>> 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 'eh_abort_handler'.
>>
>> If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
>> command will be retried if possible. If no retries are allowed
>> the command will be returned immediately, as we have to assume
>> the TMF succeeded and the command is completed with the LLDD.
>> If the TMF fails the command will be pushed back onto the
>> list of failed commands and the SCSI EH handler will be
>> called immediately for all timed-out commands.
> 
> Why can't we use a work item per command?  Linking things into a list
> just to queue it up to workqueues missed half of the point of the
> workqueue infrastructure.
> 
Hmm. I felt that using a per command workqueue might be a bit excessive.
Also the current semantics call for a synchronous command abort.
So even using a per command workqueue won't buy us anything as the
workqueue item would have to wait for the command abort to complete,
which again is quite a waste.
And concurrency would be hell; you'd have to flush the workqueue
items for all outstanding if a device reset should attempted.
And hope that no completion arrives at the time you're attempting to
flush them. etc.

I've been planning for asynchronous command aborts eventually, where
using a per-command workqueue item comes in useful. Gut for now
using existing callbacks makes life so much easier. And per-command
workqueues will just complicate matters.

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 3/9] scsi: improved eh timeout handler

[Jörn Engel]

On Mon, 10 June 2013 11:00:49 +0200, Hannes Reinecke wrote:
> On 06/10/2013 10:20 AM, Christoph Hellwig wrote:
> > 
> > Why can't we use a work item per command?  Linking things into a list
> > just to queue it up to workqueues missed half of the point of the
> > workqueue infrastructure.
> > 
> Hmm. I felt that using a per command workqueue might be a bit excessive.
> Also the current semantics call for a synchronous command abort.
> So even using a per command workqueue won't buy us anything as the
> workqueue item would have to wait for the command abort to complete,
> which again is quite a waste.

Not sure if you confuse workqueue with workitem.  Either way, using a
single work item to handle a queue of commands does not fly and we
either have to use per-command work items or abandon work queues and
use a kernel thread.  The middle ground is either racy or useless.

I don't care too much whether we use per-command work items or a
single system-global thread.  This shouldn't ever become a hot path or
the system is screwed anyway.  The only problem with our current error
handling is that even rare errors can effectively break the system.

Jörn

--
Victory in war is not repetitious.
-- Sun Tzu
--
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 8/9] mpt2sas: Enable new EH timeout handler

[Jörn Engel]

On Mon, 10 June 2013 09:40:57 +0200, Hannes Reinecke wrote:
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/mpt2sas/mpt2sas_scsih.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> index c6bdc92..47fc66c 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> @@ -2565,6 +2565,16 @@ _scsih_tm_display_info(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd)
>  }
>  
>  /**
> + * _scsih_timed_out - eh timeout handler
> + * @scmd: pointer to scsi command object
> + */
> +static enum blk_eh_timer_return
> +_scsih_timed_out(struct scsi_cmnd *scmd)
> +{
> +	return scsi_abort_command(scmd);
> +}

Why did you create this function and not do the following?
+	.eh_timed_out			= scsi_abort_command,

> +/**
>   * _scsih_abort - eh threads main abort routine
>   * @scmd: pointer to scsi command object
>   *
> @@ -7515,6 +7525,7 @@ static struct scsi_host_template scsih_driver_template = {
>  	.scan_start			= _scsih_scan_start,
>  	.change_queue_depth 		= _scsih_change_queue_depth,
>  	.change_queue_type		= _scsih_change_queue_type,
> +	.eh_timed_out			= _scsih_timed_out,
>  	.eh_abort_handler		= _scsih_abort,
>  	.eh_device_reset_handler	= _scsih_dev_reset,
>  	.eh_target_reset_handler	= _scsih_target_reset,
> -- 
> 1.7.12.4
> 

Jörn

--
The cheapest, fastest and most reliable components of a computer
system are those that aren't there.
-- Gordon Bell, DEC labratories
--
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 3/9] scsi: improved eh timeout handler

[Jörn Engel]

On Mon, 10 June 2013 09:40:52 +0200, Hannes Reinecke wrote:
> +
> +	spin_lock_irqsave(&sdev->list_lock, flags);
> +	if (list_empty(&sdev->eh_abort_list))
> +		kick_worker = 1;
> +	list_add(&scmd->eh_entry, &sdev->eh_abort_list);
> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	SCSI_LOG_ERROR_RECOVERY(3,
> +		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
> +	if (kick_worker)
> +		schedule_work(&sdev->abort_work);

You fixed the case with the non-empty list.  But afaics the same
workitem can still be called multiple times simultaneously, which will
crash the system.

Jörn

--
The only real mistake is the one from which we learn nothing.
-- John Powell
--
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 3/9] scsi: improved eh timeout handler

[Jörn Engel]

On Mon, 10 June 2013 11:19:16 -0400, Jörn Engel wrote:
> 
> I don't care too much whether we use per-command work items or a
> single system-global thread.

Actually, I do care.  We have to abort the commands in parallel, as a
fairly large number of abort can queue up and individual aborts can
take 20s on hardware I care about.

20s for an abort is pretty bad, but given today's reality there is no
need to make things worse by serializing.

Jörn

--
A defeated army first battles and then seeks victory.
-- Sun Tzu
--
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 8/9] mpt2sas: Enable new EH timeout handler

[Hannes Reinecke]

On 06/10/2013 05:31 PM, Jörn Engel wrote:
> On Mon, 10 June 2013 09:40:57 +0200, Hannes Reinecke wrote:
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/mpt2sas/mpt2sas_scsih.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> index c6bdc92..47fc66c 100644
>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
>> @@ -2565,6 +2565,16 @@ _scsih_tm_display_info(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd)
>>  }
>>  
>>  /**
>> + * _scsih_timed_out - eh timeout handler
>> + * @scmd: pointer to scsi command object
>> + */
>> +static enum blk_eh_timer_return
>> +_scsih_timed_out(struct scsi_cmnd *scmd)
>> +{
>> +	return scsi_abort_command(scmd);
>> +}
> 
> Why did you create this function and not do the following?
> +	.eh_timed_out			= scsi_abort_command,
> 
Good point.

That function was required for an earlier version (where
I had 'void scsi_abort_command()').
So yeah, that function isn't required.

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 3/9] scsi: improved eh timeout handler

[Hannes Reinecke]

On 06/11/2013 01:24 AM, Jörn Engel wrote:
> On Mon, 10 June 2013 11:19:16 -0400, Jörn Engel wrote:
>>
>> I don't care too much whether we use per-command work items or a
>> single system-global thread.
> 
> Actually, I do care.  We have to abort the commands in parallel, as a
> fairly large number of abort can queue up and individual aborts can
> take 20s on hardware I care about.
> 
> 20s for an abort is pretty bad, but given today's reality there is no
> need to make things worse by serializing.
> 
We're only serializing aborts per LUN, so this is a _big_
improvement as the original, where we would be serializing
per _host_.
Also, upon the first abort failure EH will be escalating to
LUN reset, so we won't have to wait for all aborts to time out.

More importantly, the current synchronous implementation of
command aborts does not allow for complete de-serialisation:
- There is no way to abort a running command abort, so we
  have to wait for it to complete, with the chance of running
  into a timeout.
- We will have to sent command aborts in parallel, and can
  only stop sending aborts once the first returns an error.
- After we've received an error we have to wait for the
  outstanding aborts to complete.
-> So the max wait-time will be 2 times the abort timeout.
  Not much of a gain here :-)

The _correct_ way of handling asynchronous aborts would
be to mandate that the LLDD has to send a command completion
on the original command once an abort has been issued.
Then we could just kick off the TMF and rearm the request
timer. Everything else would then be handled via normal
I/O paths.

However, this would mean to implement new callouts into
each and every driver. And the actual gain would be
dubious, as the several IHVs indicated that a command
abort might be handled lazily, ie the target will return
a good status, but abort the command only at a later time.
Other vendors treat a command abort as a best bet, and
rely on the LUN reset to clear up things.

So overall I doubt we'd be gaining much from a fully
asynchronous command abort. I'd rather concentrate
on getting the remaining bits like LUN reset working
correctly.

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 3/9] scsi: improved eh timeout handler

[Jörn Engel]

On Tue, 11 June 2013 08:18:51 +0200, Hannes Reinecke wrote:
> On 06/11/2013 01:24 AM, Jörn Engel wrote:
> > On Mon, 10 June 2013 11:19:16 -0400, Jörn Engel wrote:
> >>
> >> I don't care too much whether we use per-command work items or a
> >> single system-global thread.
> > 
> > Actually, I do care.  We have to abort the commands in parallel, as a
> > fairly large number of abort can queue up and individual aborts can
> > take 20s on hardware I care about.
> > 
> > 20s for an abort is pretty bad, but given today's reality there is no
> > need to make things worse by serializing.
> > 
> We're only serializing aborts per LUN, so this is a _big_
> improvement as the original, where we would be serializing
> per _host_.

I agree it is a big improvement.  But I also have some evidence
indicating it may not be enough.  So let me change my private patch to
do parallel aborts and see how it fares.

> Also, upon the first abort failure EH will be escalating to
> LUN reset, so we won't have to wait for all aborts to time out.

The case I saw was a successful abort taking 20s.

> More importantly, the current synchronous implementation of
> command aborts does not allow for complete de-serialisation:
> - There is no way to abort a running command abort, so we
>   have to wait for it to complete, with the chance of running
>   into a timeout.
> - We will have to sent command aborts in parallel, and can
>   only stop sending aborts once the first returns an error.
> - After we've received an error we have to wait for the
>   outstanding aborts to complete.
> -> So the max wait-time will be 2 times the abort timeout.
>   Not much of a gain here :-)

I have seen 10 commands get queued for aborts.  Assuming the 20s above
are the worst case, it will make the difference between 200s and 40s.

Granted, 40s is still horrible.  The command in question has just
timed out and the kernel should inform userspace about this asap.  If
a command with a 10s timeout takes 50s to complete, userspace will
have to add another layer of timeouts.  That should never be
necessary.

> The _correct_ way of handling asynchronous aborts would
> be to mandate that the LLDD has to send a command completion
> on the original command once an abort has been issued.
> Then we could just kick off the TMF and rearm the request
> timer. Everything else would then be handled via normal
> I/O paths.
> 
> However, this would mean to implement new callouts into
> each and every driver. And the actual gain would be
> dubious, as the several IHVs indicated that a command
> abort might be handled lazily, ie the target will return
> a good status, but abort the command only at a later time.
> Other vendors treat a command abort as a best bet, and
> rely on the LUN reset to clear up things.
> 
> So overall I doubt we'd be gaining much from a fully
> asynchronous command abort. I'd rather concentrate
> on getting the remaining bits like LUN reset working
> correctly.

Fair approach.  Again, let me play with it and don't let this stop you
from making other improvements.

Jörn

--
There's nothing that will change someone's moral outlook quicker
than cash in large sums.
-- Larry Flynt
--
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 3/9] scsi: improved eh timeout handler

[James Bottomley]

On Mon, 2013-06-10 at 01:20 -0700, Christoph Hellwig wrote:
> On Mon, Jun 10, 2013 at 09:40:52AM +0200, Hannes Reinecke wrote:
> > 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 'eh_abort_handler'.
> > 
> > If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
> > command will be retried if possible. If no retries are allowed
> > the command will be returned immediately, as we have to assume
> > the TMF succeeded and the command is completed with the LLDD.
> > If the TMF fails the command will be pushed back onto the
> > list of failed commands and the SCSI EH handler will be
> > called immediately for all timed-out commands.
> 
> Why can't we use a work item per command?  Linking things into a list
> just to queue it up to workqueues missed half of the point of the
> workqueue infrastructure.

Actually, I think we can dump the workqueue altogether.  The only reason
we need it is because the current abort handlers wait for the command
and return the completion state.  However, all LLDs are capable of
emitting TMFs at interrupt level, so if we separated the emit from the
wait, we could simply do this sequence:

on timeout, fire the abort from interrupt and mark the command as having
an abort issued (possibly by adding a pointer to the abort task), return
BLK_EH_RESET_TIMER.

Now if the timeout fires again, assume the abort was unsucessful and
escalate to LUN reset.

This is fully asynchronous, fully tracked and doesn't rely on work
queues.

The necessary additions for something like this are the from interrupt
issue abort and LUN reset, which could just be additional callbacks in
the host template.

James

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

[Ewan Milne]

On Tue, 2013-06-11 at 18:57 +0000, James Bottomley wrote:
> On Mon, 2013-06-10 at 01:20 -0700, Christoph Hellwig wrote:
> > On Mon, Jun 10, 2013 at 09:40:52AM +0200, Hannes Reinecke wrote:
> > > 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 'eh_abort_handler'.
> > > 
> > > If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
> > > command will be retried if possible. If no retries are allowed
> > > the command will be returned immediately, as we have to assume
> > > the TMF succeeded and the command is completed with the LLDD.
> > > If the TMF fails the command will be pushed back onto the
> > > list of failed commands and the SCSI EH handler will be
> > > called immediately for all timed-out commands.
> > 
> > Why can't we use a work item per command?  Linking things into a list
> > just to queue it up to workqueues missed half of the point of the
> > workqueue infrastructure.
> 
> Actually, I think we can dump the workqueue altogether.  The only reason
> we need it is because the current abort handlers wait for the command
> and return the completion state.  However, all LLDs are capable of
> emitting TMFs at interrupt level, so if we separated the emit from the
> wait, we could simply do this sequence:
> 
> on timeout, fire the abort from interrupt and mark the command as having
> an abort issued (possibly by adding a pointer to the abort task), return
> BLK_EH_RESET_TIMER.

Doesn't this cause blk_rq_timed_out to reset the timer on the req to
the original timeout value again?  It seems like this would increase
the time before any further attempted error handling.  The default
timeout is 30 seconds for sd, but it could be much longer (e.g.
WRITE SAME, which was 120 seconds last I looked).

> Now if the timeout fires again, assume the abort was unsucessful and
> escalate to LUN reset.
> 
> This is fully asynchronous, fully tracked and doesn't rely on work
> queues.
> 
> The necessary additions for something like this are the from interrupt
> issue abort and LUN reset, which could just be additional callbacks in
> the host template.
> 
> James
> 

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

[James Bottomley]

On Tue, 2013-06-11 at 16:41 -0400, Ewan Milne wrote:
> On Tue, 2013-06-11 at 18:57 +0000, James Bottomley wrote:
> > On Mon, 2013-06-10 at 01:20 -0700, Christoph Hellwig wrote:
> > > On Mon, Jun 10, 2013 at 09:40:52AM +0200, Hannes Reinecke wrote:
> > > > 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 'eh_abort_handler'.
> > > > 
> > > > If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
> > > > command will be retried if possible. If no retries are allowed
> > > > the command will be returned immediately, as we have to assume
> > > > the TMF succeeded and the command is completed with the LLDD.
> > > > If the TMF fails the command will be pushed back onto the
> > > > list of failed commands and the SCSI EH handler will be
> > > > called immediately for all timed-out commands.
> > > 
> > > Why can't we use a work item per command?  Linking things into a list
> > > just to queue it up to workqueues missed half of the point of the
> > > workqueue infrastructure.
> > 
> > Actually, I think we can dump the workqueue altogether.  The only reason
> > we need it is because the current abort handlers wait for the command
> > and return the completion state.  However, all LLDs are capable of
> > emitting TMFs at interrupt level, so if we separated the emit from the
> > wait, we could simply do this sequence:
> > 
> > on timeout, fire the abort from interrupt and mark the command as having
> > an abort issued (possibly by adding a pointer to the abort task), return
> > BLK_EH_RESET_TIMER.
> 
> Doesn't this cause blk_rq_timed_out to reset the timer on the req to
> the original timeout value again?  It seems like this would increase
> the time before any further attempted error handling.  The default
> timeout is 30 seconds for sd, but it could be much longer (e.g.
> WRITE SAME, which was 120 seconds last I looked).

It currently does, but that's fixable via a special return code.

James

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

[Hannes Reinecke]

On 06/11/2013 08:57 PM, James Bottomley wrote:
> On Mon, 2013-06-10 at 01:20 -0700, Christoph Hellwig wrote:
>> On Mon, Jun 10, 2013 at 09:40:52AM +0200, Hannes Reinecke wrote:
>>> 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 'eh_abort_handler'.
>>>
>>> If the 'eh_abort_handler' returns SUCCESS or FAST_IO_FAIL the
>>> command will be retried if possible. If no retries are allowed
>>> the command will be returned immediately, as we have to assume
>>> the TMF succeeded and the command is completed with the LLDD.
>>> If the TMF fails the command will be pushed back onto the
>>> list of failed commands and the SCSI EH handler will be
>>> called immediately for all timed-out commands.
>>
>> Why can't we use a work item per command?  Linking things into a list
>> just to queue it up to workqueues missed half of the point of the
>> workqueue infrastructure.
> 
> Actually, I think we can dump the workqueue altogether.  The only reason
> we need it is because the current abort handlers wait for the command
> and return the completion state.  However, all LLDs are capable of
> emitting TMFs at interrupt level, so if we separated the emit from the
> wait, we could simply do this sequence:
> 
> on timeout, fire the abort from interrupt and mark the command as having
> an abort issued (possibly by adding a pointer to the abort task), return
> BLK_EH_RESET_TIMER.
> 
> Now if the timeout fires again, assume the abort was unsucessful and
> escalate to LUN reset.
> 
> This is fully asynchronous, fully tracked and doesn't rely on work
> queues.
> 
Hehe. Been there, done that, doesn't work :-(
That was my original idea, too.

But some LLDDs send TMFs in a non-interrupt-safe way, so the only
way to make it work was to use a workqueue.
(Eg qla2xxx has a parameter to send TMFs async, but this doesn't
work on older firmware).

> The necessary additions for something like this are the from interrupt
> issue abort and LUN reset, which could just be additional callbacks in
> the host template.
> 
However, we could make that optional, so that LLDDs not capable of
sending TMFs in an interrupt-safe manner will continue to use the
original framework.

Ok, let's see whether this flies.

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 3/9] scsi: improved eh timeout handler

[Bart Van Assche]

On 06/11/13 20:57, James Bottomley wrote:
> Actually, I think we can dump the workqueue altogether.  The only reason
> we need it is because the current abort handlers wait for the command
> and return the completion state.  However, all LLDs are capable of
> emitting TMFs at interrupt level, so if we separated the emit from the
> wait, we could simply do this sequence:
>
> on timeout, fire the abort from interrupt and mark the command as having
> an abort issued (possibly by adding a pointer to the abort task), return
> BLK_EH_RESET_TIMER.
>
> Now if the timeout fires again, assume the abort was unsuccessful and
> escalate to LUN reset.
>
> This is fully asynchronous, fully tracked and doesn't rely on work
> queues.
>
> The necessary additions for something like this are the from interrupt
> issue abort and LUN reset, which could just be additional callbacks in
> the host template.

Do we really need a new callback in the host template for a command 
abort that does not wait ? Several LLDs already have their own internal 
data structures for keeping track of the request state and can use these 
data structures to set a flag "command has been aborted". If aborting a 
command fails and the command completes that flag can then be used to 
avoid a second invocation of scsi_done(). At least, that's what the SRP 
initiator already does today in srp_abort().

Bart.

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

[Hannes Reinecke]

On 06/12/2013 08:34 AM, Bart Van Assche wrote:
> On 06/11/13 20:57, James Bottomley wrote:
>> Actually, I think we can dump the workqueue altogether.  The only
>> reason
>> we need it is because the current abort handlers wait for the command
>> and return the completion state.  However, all LLDs are capable of
>> emitting TMFs at interrupt level, so if we separated the emit from
>> the
>> wait, we could simply do this sequence:
>>
>> on timeout, fire the abort from interrupt and mark the command as
>> having
>> an abort issued (possibly by adding a pointer to the abort task),
>> return
>> BLK_EH_RESET_TIMER.
>>
>> Now if the timeout fires again, assume the abort was unsuccessful and
>> escalate to LUN reset.
>>
>> This is fully asynchronous, fully tracked and doesn't rely on work
>> queues.
>>
>> The necessary additions for something like this are the from
>> interrupt
>> issue abort and LUN reset, which could just be additional
>> callbacks in
>> the host template.
> 
> Do we really need a new callback in the host template for a command
> abort that does not wait ? Several LLDs already have their own
> internal data structures for keeping track of the request state and
> can use these data structures to set a flag "command has been
> aborted". If aborting a command fails and the command completes that
> flag can then be used to avoid a second invocation of scsi_done().
> At least, that's what the SRP initiator already does today in
> srp_abort().
> 
Currently I'm playing around with this:

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 4908480..498164a 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -174,6 +174,9 @@ struct scsi_host_template {
        int (* eh_bus_reset_handler)(struct scsi_cmnd *);
        int (* eh_host_reset_handler)(struct scsi_cmnd *);

+       int (* eh_send_abort)(struct scsi_cmnd *);
+       void (* eh_cancel_abort)(struct scsi_cmnd *);
+
        /*

'eh_send_abort' sends the TMF asynchronously; it is the drivers
responsibility to return the command to be aborted to the SCSI
midlayer after calling this function.
The return value is 'SUCCESS' if the TMF was sent or 'FAILED' if
sending the TMF failed. It does _not_ indicate the status of the TMF
itself, as this is done implicitly by returning the command with an
appropriate status.

'eh_cancel_abort' is optional, and can be called at any time after
'eh_send_abort' to cancel the TMF. After calling 'eh_cancel_abort'
neither the TMF nor the command must be considered valid from the LLDD.
'eh_cancel_abort' is for LLDDs requiring cleanups when aborting
TMFs. LLDDs not requiring any cleanup simply can not implement this
callback.

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 4/9] virtio_scsi: Enable new EH timeout handler

[Paolo Bonzini]

Il 10/06/2013 03:40, Hannes Reinecke ha scritto:
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/virtio_scsi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 2168258..1efd219 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -654,6 +654,11 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
>  	return virtscsi_tmf(vscsi, cmd);
>  }
>  
> +static enum blk_eh_timer_return virtscsi_timedout(struct scsi_cmnd *scmd)
> +{
> +	return scsi_abort_command(scmd);
> +}
> +
>  static int virtscsi_target_alloc(struct scsi_target *starget)
>  {
>  	struct virtio_scsi_target_state *tgt =
> @@ -683,6 +688,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
>  	.queuecommand = virtscsi_queuecommand_single,
>  	.eh_abort_handler = virtscsi_abort,
>  	.eh_device_reset_handler = virtscsi_device_reset,
> +	.eh_timed_out = virtscsi_timedout,
>  
>  	.can_queue = 1024,
>  	.dma_boundary = UINT_MAX,
> @@ -699,6 +705,7 @@ static struct scsi_host_template virtscsi_host_template_multi = {
>  	.queuecommand = virtscsi_queuecommand_multi,
>  	.eh_abort_handler = virtscsi_abort,
>  	.eh_device_reset_handler = virtscsi_device_reset,
> +	.eh_timed_out = virtscsi_timedout,
>  
>  	.can_queue = 1024,
>  	.dma_boundary = UINT_MAX,
> 

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 5/9] virtio-scsi: Implement TMF timeout

[Paolo Bonzini]

Il 10/06/2013 03:40, Hannes Reinecke ha scritto:
> Any TMF might be take longer as expected, or not return at all.
> So we need to use 'wait_for_completion_timeout' when sending
> a TMF to protect against these cases.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/virtio_scsi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 1efd219..abfc684 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -30,6 +30,7 @@
>  #define VIRTIO_SCSI_MEMPOOL_SZ 64
>  #define VIRTIO_SCSI_EVENT_LEN 8
>  #define VIRTIO_SCSI_VQ_BASE 2
> +#define VIRTIO_SCSI_TMF_TIMEOUT 30
>  
>  /* Command queue element */
>  struct virtio_scsi_cmd {
> @@ -597,8 +598,10 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  			      GFP_NOIO) < 0)
>  		goto out;
>  
> -	wait_for_completion(&comp);
> -	if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
> +	if (wait_for_completion_timeout(&comp,
> +					VIRTIO_SCSI_TMF_TIMEOUT * HZ) == 0)
> +		ret = FAILED;
> +	else if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
>  	    cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
>  		ret = SUCCESS;
>  
> 


Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH 5/9] virtio-scsi: Implement TMF timeout

[Asias He]

On Mon, Jun 10, 2013 at 09:40:54AM +0200, Hannes Reinecke wrote:
> Any TMF might be take longer as expected, or not return at all.
> So we need to use 'wait_for_completion_timeout' when sending
> a TMF to protect against these cases.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Asias He <asias@redhat.com>

> ---
>  drivers/scsi/virtio_scsi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 1efd219..abfc684 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -30,6 +30,7 @@
>  #define VIRTIO_SCSI_MEMPOOL_SZ 64
>  #define VIRTIO_SCSI_EVENT_LEN 8
>  #define VIRTIO_SCSI_VQ_BASE 2
> +#define VIRTIO_SCSI_TMF_TIMEOUT 30
>  
>  /* Command queue element */
>  struct virtio_scsi_cmd {
> @@ -597,8 +598,10 @@ static int virtscsi_tmf(struct virtio_scsi *vscsi, struct virtio_scsi_cmd *cmd)
>  			      GFP_NOIO) < 0)
>  		goto out;
>  
> -	wait_for_completion(&comp);
> -	if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
> +	if (wait_for_completion_timeout(&comp,
> +					VIRTIO_SCSI_TMF_TIMEOUT * HZ) == 0)
> +		ret = FAILED;
> +	else if (cmd->resp.tmf.response == VIRTIO_SCSI_S_OK ||
>  	    cmd->resp.tmf.response == VIRTIO_SCSI_S_FUNCTION_SUCCEEDED)
>  		ret = SUCCESS;
>  
> -- 
> 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

-- 
Asias

Re: [PATCH 4/9] virtio_scsi: Enable new EH timeout handler

[Asias He]

On Mon, Jun 10, 2013 at 09:40:53AM +0200, Hannes Reinecke wrote:
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Reviewed-by: Asias He <asias@redhat.com>

> ---
>  drivers/scsi/virtio_scsi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index 2168258..1efd219 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -654,6 +654,11 @@ static int virtscsi_abort(struct scsi_cmnd *sc)
>  	return virtscsi_tmf(vscsi, cmd);
>  }
>  
> +static enum blk_eh_timer_return virtscsi_timedout(struct scsi_cmnd *scmd)
> +{
> +	return scsi_abort_command(scmd);
> +}
> +
>  static int virtscsi_target_alloc(struct scsi_target *starget)
>  {
>  	struct virtio_scsi_target_state *tgt =
> @@ -683,6 +688,7 @@ static struct scsi_host_template virtscsi_host_template_single = {
>  	.queuecommand = virtscsi_queuecommand_single,
>  	.eh_abort_handler = virtscsi_abort,
>  	.eh_device_reset_handler = virtscsi_device_reset,
> +	.eh_timed_out = virtscsi_timedout,
>  
>  	.can_queue = 1024,
>  	.dma_boundary = UINT_MAX,
> @@ -699,6 +705,7 @@ static struct scsi_host_template virtscsi_host_template_multi = {
>  	.queuecommand = virtscsi_queuecommand_multi,
>  	.eh_abort_handler = virtscsi_abort,
>  	.eh_device_reset_handler = virtscsi_device_reset,
> +	.eh_timed_out = virtscsi_timedout,
>  
>  	.can_queue = 1024,
>  	.dma_boundary = UINT_MAX,
> -- 
> 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

-- 
Asias

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

[Hannes Reinecke]

On 09/03/2013 11:13 AM, Bart Van Assche wrote:
> On 09/02/13 15:11, Hannes Reinecke wrote:
>> On 09/02/2013 02:45 PM, Bart Van Assche wrote:
>>> This patch adds several new calls to LLD EH handlers. Is it
>>> guaranteed that these will only be invoked before scsi_remove_host()
>>> has finished ? For more background information, see also "[PATCH]
>>> Make scsi_remove_host() wait until error handling finished"
>>> (http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).
>>>
>> Well, that depends how scsi_remove_host() handles outstanding
>> commands. What happens if you call scsi_remove_host() and there is
>> still I/O in flight? I would assume that any HBA would have to kill
>> any outstanding I/O prior to calling scsi_remove_host() (FC most
>> certainly does this).
>> Which would mean that it'll have to wait for scsi_put_command() to
>> be called for all outstanding commands. And as scsi_put_command()
>> will be called only _after_ our routine runs (see the reasoning
>> above) we should be safe.
> 
> Hello Hannes,
> 
> Since fc_remove_host() has to be invoked before scsi_remove_host()
> and since fc_remove_host() changes the port state into
> FC_PORTSTATE_DELETED this means that fc_remote_port_chkready() will
> return DID_NO_CONNECT while scsi_remove_host() is in progress. I
> think this prevents that the SYNCHRONIZE CACHE command submitted by
> sd_remove() reaches SCSI disks over FC since sd_remove() is invoked
> from inside scsi_remove_host(). The SRP transport patch I had posted
> recently makes sure that the SYNCHRONIZE CACHE command submitted by
> sd_remove() reaches the target SCSI disk. This makes me wonder what
> the correct behavior is for SCSI transport drivers - disabling I/O
> before scsi_remove_host() starts or ensuring that I/O is still
> possible while scsi_remove_host() is in progress ? I think the call
> chain is as follows: scsi_remove_host() -> device_del() ->
> bus_remove_device() -> device_release_driver() ->
> __device_release_driver() -> sd_remove() -> sd_shutdown() ->
> sd_sync_cache().
> 

The actual call chain is
scsi_remove_host() -> scsi_forget_host() -> __scsi_remove_device()
-> device_del() etc.

What's important here is that __scsi_remove_device() sets the state
'SDEV_DEL' and calls blk_cleanup_queue().

So after __scsi_remove_device() no further I/O is possible.
However, prior to setting SDEV_DEL I/O should be perfectly okay, so
one would assume that the SYNCHRONIZE CACHE command should be sent
to the device. USB most certainly does it.

A safe practice, however, would be to ensure that no _other_ I/O can
be sent to the device, ie that all I/O coming in via the request
queue or ioctl should be short-circuited and never hit the device at
all. That's what fc_remote_port_chkready() does.

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 3/9] scsi: improved eh timeout handler

[Bart Van Assche]

On 09/02/13 15:11, Hannes Reinecke wrote:
> On 09/02/2013 02:45 PM, Bart Van Assche wrote:
>> This patch adds several new calls to LLD EH handlers. Is it
>> guaranteed that these will only be invoked before scsi_remove_host()
>> has finished ? For more background information, see also "[PATCH]
>> Make scsi_remove_host() wait until error handling finished"
>> (http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).
>>
> Well, that depends how scsi_remove_host() handles outstanding
> commands. What happens if you call scsi_remove_host() and there is
> still I/O in flight? I would assume that any HBA would have to kill
> any outstanding I/O prior to calling scsi_remove_host() (FC most
> certainly does this).
> Which would mean that it'll have to wait for scsi_put_command() to
> be called for all outstanding commands. And as scsi_put_command()
> will be called only _after_ our routine runs (see the reasoning
> above) we should be safe.

Hello Hannes,

Since fc_remove_host() has to be invoked before scsi_remove_host() and 
since fc_remove_host() changes the port state into FC_PORTSTATE_DELETED 
this means that fc_remote_port_chkready() will return DID_NO_CONNECT 
while scsi_remove_host() is in progress. I think this prevents that the 
SYNCHRONIZE CACHE command submitted by sd_remove() reaches SCSI disks 
over FC since sd_remove() is invoked from inside scsi_remove_host(). The 
SRP transport patch I had posted recently makes sure that the 
SYNCHRONIZE CACHE command submitted by sd_remove() reaches the target 
SCSI disk. This makes me wonder what the correct behavior is for SCSI 
transport drivers - disabling I/O before scsi_remove_host() starts or 
ensuring that I/O is still possible while scsi_remove_host() is in 
progress ? I think the call chain is as follows: scsi_remove_host() -> 
device_del() -> bus_remove_device() -> device_release_driver() -> 
__device_release_driver() -> sd_remove() -> sd_shutdown() -> 
sd_sync_cache().

Bart.

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

[Bart Van Assche]

On 09/02/13 15:11, Hannes Reinecke wrote:
> On 09/02/2013 02:45 PM, Bart Van Assche wrote:
>> On 09/02/13 09:12, Hannes Reinecke wrote:
>>> @@ -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);
>>
>> Is this approach safe ? Is it e.g. possible that the abort work
>> starts just before the cancel_delayed_work() call and continues to
>> run after scsi_put_command() has finished ? In
>> drivers/scsi/libfc/fc_exch.c a similar issue is solved by holding an
>> additional reference as long as delayed work (fc_exch.timeout_work)
>> is queued.
>>
> I have been thinking of this, and in fact my original approach had
> 'cancel_delayed_work_sync' here. However, this didn't work as
> scsi_put_command() might end up being called from an softirq context.
>
>>From my understanding the workqueue stuff guarantees that either
> a) the workqueue item is still queued
>     -> cancel_delayed_work will be in fact synchronous, as it'll
>        cancel queue item from the queue
> b) the workqueue item is running
>     -> cancel_delayed_work is essentially a no-op, as the function
>        is running and will be terminated anyway.
>
> IE from the API perspective the transition between 'queued' and
> 'running' is atomic, and no in-between states are visible.
>
> So case a) is obviously safe, and for case b) the abort function is
> already running. But then the abort function has been called from
> the block timeout handler, which did a blk_mark_rq_complete() prior
> to calling the handler. So any completion coming in after that will
> be ignored, and scsi_put_command() won't be called.
>
> Hence we should be safe here.

Hello Hannes,

I think that you have just explained why that cancel_delayed_work() call 
in scsi_put_command() is not necessary at all ... In case of normal 
command completion, scsi_put_command() will be reached with no 
scmd_eh_abort_handler() call queued since there was no timeout. If the 
command timed out scsi_put_command() will be called after the abort_work 
has already been dequeued since work items are dequeued before being 
executed.

Bart.

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

[Hannes Reinecke]

On 09/02/2013 02:45 PM, Bart Van Assche wrote:
> On 09/02/13 09:12, Hannes Reinecke wrote:
>> @@ -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);
> 
> Is this approach safe ? Is it e.g. possible that the abort work
> starts just before the cancel_delayed_work() call and continues to
> run after scsi_put_command() has finished ? In
> drivers/scsi/libfc/fc_exch.c a similar issue is solved by holding an
> additional reference as long as delayed work (fc_exch.timeout_work)
> is queued.
> 
I have been thinking of this, and in fact my original approach had
'cancel_delayed_work_sync' here. However, this didn't work as
scsi_put_command() might end up being called from an softirq context.

From my understanding the workqueue stuff guarantees that either
a) the workqueue item is still queued
   -> cancel_delayed_work will be in fact synchronous, as it'll
      cancel queue item from the queue
b) the workqueue item is running
   -> cancel_delayed_work is essentially a no-op, as the function
      is running and will be terminated anyway.

IE from the API perspective the transition between 'queued' and
'running' is atomic, and no in-between states are visible.

So case a) is obviously safe, and for case b) the abort function is
already running. But then the abort function has been called from
the block timeout handler, which did a blk_mark_rq_complete() prior
to calling the handler. So any completion coming in after that will
be ignored, and scsi_put_command() won't be called.

Hence we should be safe here.

>> +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);
>> +    }
>> +}
> 
> This patch adds several new calls to LLD EH handlers. Is it
> guaranteed that these will only be invoked before scsi_remove_host()
> has finished ? For more background information, see also "[PATCH]
> Make scsi_remove_host() wait until error handling finished"
> (http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).
> 
Well, that depends how scsi_remove_host() handles outstanding
commands. What happens if you call scsi_remove_host() and there is
still I/O in flight? I would assume that any HBA would have to kill
any outstanding I/O prior to calling scsi_remove_host() (FC most
certainly does this).
Which would mean that it'll have to wait for scsi_put_command() to
be called for all outstanding commands. And as scsi_put_command()
will be called only _after_ our routine runs (see the reasoning
above) we should be safe.

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 3/9] scsi: improved eh timeout handler

[Bart Van Assche]

On 09/02/13 09:12, Hannes Reinecke wrote:
> @@ -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);

Is this approach safe ? Is it e.g. possible that the abort work starts 
just before the cancel_delayed_work() call and continues to run after 
scsi_put_command() has finished ? In drivers/scsi/libfc/fc_exch.c a 
similar issue is solved by holding an additional reference as long as 
delayed work (fc_exch.timeout_work) is queued.

> +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);
> +	}
> +}

This patch adds several new calls to LLD EH handlers. Is it guaranteed 
that these will only be invoked before scsi_remove_host() has finished ? 
For more background information, see also "[PATCH] Make 
scsi_remove_host() wait until error handling finished" 
(http://thread.gmane.org/gmane.linux.scsi/82572/focus=82779).

Thanks,

Bart.

[PATCH 3/9] 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 | 146 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_priv.h  |   2 +
 include/scsi/scsi_cmnd.h  |   2 +
 4 files changed, 140 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 b7dd774..21c2465 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,116 @@ 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
+ */
+enum blk_eh_timer_return
+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 BLK_EH_NOT_HANDLED;
+	}
+
+	/*
+	 * 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 BLK_EH_NOT_HANDLED;
+	}
+
+	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 BLK_EH_SCHEDULED;
+}
+EXPORT_SYMBOL_GPL(scsi_abort_command);
+
+/**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
  * @eh_flag:	optional SCSI_EH flag.
@@ -125,6 +236,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++;
@@ -1581,7 +1694,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)
@@ -1589,6 +1702,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:
@@ -1602,18 +1717,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;
 }
 
 /**
@@ -1663,9 +1779,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..e3137e3 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 */
 
 	/*
@@ -144,6 +145,7 @@ extern void scsi_put_command(struct scsi_cmnd *);
 extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
 			       struct device *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
+extern enum blk_eh_timer_return scsi_abort_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
-- 
1.7.12.4

[PATCH 3/9] 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 | 146 +++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/scsi_priv.h  |   2 +
 include/scsi/scsi_cmnd.h  |   2 +
 4 files changed, 140 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 b7dd774..21c2465 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,116 @@ 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
+ */
+enum blk_eh_timer_return
+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 BLK_EH_NOT_HANDLED;
+	}
+
+	/*
+	 * 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 BLK_EH_NOT_HANDLED;
+	}
+
+	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 BLK_EH_SCHEDULED;
+}
+EXPORT_SYMBOL_GPL(scsi_abort_command);
+
+/**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
  * @eh_flag:	optional SCSI_EH flag.
@@ -125,6 +236,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++;
@@ -1581,7 +1694,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)
@@ -1589,6 +1702,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:
@@ -1602,18 +1717,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;
 }
 
 /**
@@ -1663,9 +1779,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..e3137e3 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 */
 
 	/*
@@ -144,6 +145,7 @@ extern void scsi_put_command(struct scsi_cmnd *);
 extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
 			       struct device *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
+extern enum blk_eh_timer_return scsi_abort_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
-- 
1.7.12.4

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

[Hannes Reinecke]

On 08/22/2013 10:51 AM, Ren Mingxin wrote:
> Hi, Hannes:
>
> On 07/01/2013 10:24 PM, Hannes Reinecke wrote:
>> 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>
>> ---
>>   drivers/scsi/scsi.c       |   1 +
>>   drivers/scsi/scsi_error.c | 139
>> ++++++++++++++++++++++++++++++++++++++++++----
>>   drivers/scsi/scsi_priv.h  |   2 +
>>   include/scsi/scsi_cmnd.h  |   2 +
>>   4 files changed, 132 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
>> index ebe3b0a..06257cf 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_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);
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index e76e895..835f7e4 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -55,6 +55,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)
>> @@ -102,6 +103,111 @@ 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);
>> +    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,
>> +                    "eh timeout, not aborting\n"));
>
> Command address should be also printed for debugging conveniently:
> +"eh timeout, not aborting command %p\n", scmd));
>
Hmm. Okay, I could. Will be preparing an updated patchset.

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 3/9] scsi: improved eh timeout handler

[Ren Mingxin]

Hi, Hannes:

On 07/01/2013 10:24 PM, Hannes Reinecke wrote:
> 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>
> ---
>   drivers/scsi/scsi.c       |   1 +
>   drivers/scsi/scsi_error.c | 139 ++++++++++++++++++++++++++++++++++++++++++----
>   drivers/scsi/scsi_priv.h  |   2 +
>   include/scsi/scsi_cmnd.h  |   2 +
>   4 files changed, 132 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index ebe3b0a..06257cf 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_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);
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index e76e895..835f7e4 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -55,6 +55,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)
> @@ -102,6 +103,111 @@ 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);
> +	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,
> +				    "eh timeout, not aborting\n"));

Command address should be also printed for debugging conveniently:
+"eh timeout, not aborting command %p\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)&&

I think 'scsi_device_online(scmd->device)' is also necessary here.

>
> +			    (++scmd->retries<= scmd->allowed)) {
> +				SCSI_LOG_ERROR_RECOVERY(3,
> +					scmd_printk(KERN_WARNING, scmd,
> +						    "retry aborted command\n"));

Command address should be also printed here.

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

Command address should be also printed here.

>
> +				scsi_finish_command(scmd);
> +			}
> +			return;
> +		}
> +		SCSI_LOG_ERROR_RECOVERY(3,
> +			scmd_printk(KERN_INFO, scmd,
> +				    "abort command failed, rtn %d\n", rtn));

Command address should be also printed here.

>
> +	}
> +
> +	if (scsi_eh_scmd_add(scmd, 0)) {
> +		SCSI_LOG_ERROR_RECOVERY(3,
> +			scmd_printk(KERN_WARNING, scmd,
> +				    "terminate aborted command\n"));

Command address should be also printed here.

>
> +		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
> + */
> +enum blk_eh_timer_return
> +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_work_sync(&scmd->abort_work);
> +		return BLK_EH_NOT_HANDLED;
> +	}
> +
> +	/*
> +	 * 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,
> +				    "host in recovery, not aborting\n"));

Command address should be also printed here.

Thanks,
Ren

>
> +		return BLK_EH_NOT_HANDLED;
> +	}
> +
> +	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_work(&scmd->abort_work);
> +	return BLK_EH_SCHEDULED;
> +}
> +EXPORT_SYMBOL_GPL(scsi_abort_command);
> +
> +/**
>    * scsi_eh_scmd_add - add scsi cmd to error handling.
>    * @scmd:	scmd to run eh on.
>    * @eh_flag:	optional SCSI_EH flag.
> @@ -127,6 +233,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++;
> @@ -1532,6 +1640,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:
> @@ -1545,18 +1655,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 determinted
> +	 * 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;
>   }
>
>   /**
> @@ -1606,9 +1717,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..a2f062e 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 work_struct abort_work;
>   	int eh_eflags;		/* Used by error handlr */
>
>   	/*
> @@ -144,6 +145,7 @@ extern void scsi_put_command(struct scsi_cmnd *);
>   extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
>   			       struct device *);
>   extern void scsi_finish_command(struct scsi_cmnd *cmd);
> +extern enum blk_eh_timer_return scsi_abort_command(struct scsi_cmnd *cmd);
>
>   extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
>   				 size_t *offset, size_t *len);

[PATCH 3/9] 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>
---
 drivers/scsi/scsi.c       |   1 +
 drivers/scsi/scsi_error.c | 139 ++++++++++++++++++++++++++++++++++++++++++----
 drivers/scsi/scsi_priv.h  |   2 +
 include/scsi/scsi_cmnd.h  |   2 +
 4 files changed, 132 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ebe3b0a..06257cf 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_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);
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index e76e895..835f7e4 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -55,6 +55,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)
@@ -102,6 +103,111 @@ 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);
+	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,
+				    "eh timeout, not aborting\n"));
+	} 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,
+						    "retry aborted command\n"));
+				scsi_queue_insert(scmd, SCSI_MLQUEUE_EH_RETRY);
+			} else {
+				SCSI_LOG_ERROR_RECOVERY(3,
+					scmd_printk(KERN_WARNING, scmd,
+						    "finish aborted command\n"));
+				scsi_finish_command(scmd);
+			}
+			return;
+		}
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "abort command failed, rtn %d\n", rtn));
+	}
+
+	if (scsi_eh_scmd_add(scmd, 0)) {
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_WARNING, scmd,
+				    "terminate aborted command\n"));
+		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
+ */
+enum blk_eh_timer_return
+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_work_sync(&scmd->abort_work);
+		return BLK_EH_NOT_HANDLED;
+	}
+
+	/*
+	 * 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,
+				    "host in recovery, not aborting\n"));
+		return BLK_EH_NOT_HANDLED;
+	}
+
+	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_work(&scmd->abort_work);
+	return BLK_EH_SCHEDULED;
+}
+EXPORT_SYMBOL_GPL(scsi_abort_command);
+
+/**
  * scsi_eh_scmd_add - add scsi cmd to error handling.
  * @scmd:	scmd to run eh on.
  * @eh_flag:	optional SCSI_EH flag.
@@ -127,6 +233,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++;
@@ -1532,6 +1640,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:
@@ -1545,18 +1655,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 determinted
+	 * 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;
 }
 
 /**
@@ -1606,9 +1717,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..a2f062e 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 work_struct abort_work;
 	int eh_eflags;		/* Used by error handlr */
 
 	/*
@@ -144,6 +145,7 @@ extern void scsi_put_command(struct scsi_cmnd *);
 extern void __scsi_put_command(struct Scsi_Host *, struct scsi_cmnd *,
 			       struct device *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
+extern enum blk_eh_timer_return scsi_abort_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
-- 
1.7.12.4