[PATCH 0/4] New SCSI command timeout handler

[PATCH 0/4] New SCSI command timeout handler

[Hannes Reinecke]

Hi all,

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.

Comments etc are welcome.

Hannes Reinecke (4):
  scsi: move initialization of scmd->eh_entry
  blk-timeout: add BLK_EH_SCHEDULED return code
  scsi: improved eh timeout handler
  virtio_scsi: use improved eh timeout handler

 drivers/scsi/scsi_error.c        | 82 ++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_lib.c          |  4 +-
 drivers/scsi/scsi_scan.c         |  3 ++
 drivers/scsi/scsi_transport_fc.c |  3 +-
 drivers/scsi/virtio_scsi.c       |  8 ++++
 include/linux/blkdev.h           |  1 +
 include/scsi/scsi_cmnd.h         |  1 +
 include/scsi/scsi_device.h       |  2 +
 8 files changed, 101 insertions(+), 3 deletions(-)

-- 
1.7.12.4

[PATCH 1/4] 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

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

[Jörn Engel]

On Thu, 6 June 2013 11:43:52 +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.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>

Tested-by: Joern Engel <joern@logfs.org>

Jörn

--
It is better to die of hunger having lived without grief and fear,
than to live with a troubled spirit amid abundance.
-- Epictetus
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] 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

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

[Jörn Engel]

On Thu, 6 June 2013 11:43:53 +0200, Hannes Reinecke wrote:
> 
> 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>

Since the patch I tested diverged from your patches 2-4, this is only
Acked-by: Joern Enge <joern@logfs.org>

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

Jörn

--
Geld macht nicht glücklich.
Glück macht nicht satt.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] 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.
For any other return code from 'eh_abort_handler' the command
will be pushed onto the existing SCSI EH handler, or aborted
with DID_TIME_OUT if that fails.

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 96b4bb6..0a6b21c 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,83 @@ 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_Host *shost = sdev->host;
+	struct scsi_cmnd *scmd, *tmp;
+	unsigned long flags;
+	int rtn;
+
+	spin_lock_irqsave(&sdev->list_lock, flags);
+	list_for_each_entry_safe(scmd, tmp, &sdev->eh_abort_list, eh_entry) {
+		list_del_init(&scmd->eh_entry);
+		spin_unlock_irqrestore(&sdev->list_lock, flags);
+		SCSI_LOG_ERROR_RECOVERY(3,
+			scmd_printk(KERN_INFO, scmd,
+				    "aborting command %p\n", scmd));
+		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
+		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
+			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);
+			}
+		} else {
+			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);
+			}
+		}
+		spin_lock_irqsave(&sdev->list_lock, flags);
+	}
+	spin_unlock_irqrestore(&sdev->list_lock, flags);
+}
+
+/**
+ * scsi_abort_command - schedule a command abort
+ * @scmd:	scmd to abort.
+ *
+ * We only need to abort commands after a command timeout
+ */
+void
+scsi_abort_command(struct scsi_cmnd *scmd)
+{
+	unsigned long flags;
+	int kick_worker = 0;
+	struct scsi_device *sdev = scmd->device;
+
+	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);
+	SCSI_LOG_ERROR_RECOVERY(3,
+		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
+	spin_unlock_irqrestore(&sdev->list_lock, flags);
+	if (kick_worker)
+		schedule_work(&sdev->abort_work);
+}
+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.
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_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index e106c27..09237bf 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -2079,7 +2079,8 @@ fc_timed_out(struct scsi_cmnd *scmd)
 	if (rport->port_state == FC_PORTSTATE_BLOCKED)
 		return BLK_EH_RESET_TIMER;
 
-	return BLK_EH_NOT_HANDLED;
+	scsi_abort_command(scmd);
+	return BLK_EH_SCHEDULED;
 }
 
 /*
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de5f5d8..460e514 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 void 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

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

[Jörn Engel]

On Thu, 6 June 2013 11:43:54 +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.
> For any other return code from 'eh_abort_handler' the command
> will be pushed onto the existing SCSI EH handler, or aborted
> with DID_TIME_OUT if that fails.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_error.c        | 79 ++++++++++++++++++++++++++++++++++++++++
>  drivers/scsi/scsi_scan.c         |  3 ++
>  drivers/scsi/scsi_transport_fc.c |  3 +-
>  include/scsi/scsi_cmnd.h         |  1 +
>  include/scsi/scsi_device.h       |  2 +
>  5 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 96b4bb6..0a6b21c 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,83 @@ 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_Host *shost = sdev->host;
> +	struct scsi_cmnd *scmd, *tmp;
> +	unsigned long flags;
> +	int rtn;
> +
> +	spin_lock_irqsave(&sdev->list_lock, flags);
> +	list_for_each_entry_safe(scmd, tmp, &sdev->eh_abort_list, eh_entry) {
> +		list_del_init(&scmd->eh_entry);

The _init bit is not needed.  I prefer list_del, as the poisoning
sometimes helps catching bugs.

> +		spin_unlock_irqrestore(&sdev->list_lock, flags);
> +		SCSI_LOG_ERROR_RECOVERY(3,
> +			scmd_printk(KERN_INFO, scmd,
> +				    "aborting command %p\n", scmd));
> +		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
> +		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
> +			if (((scmd->request->cmd_flags & REQ_FAILFAST_DEV) ||

Am I being stupid again or should this be negated?

> +			     (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);
> +			}
> +		} else {
> +			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);
> +			}
> +		}
> +		spin_lock_irqsave(&sdev->list_lock, flags);
> +	}
> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +}
> +
> +/**
> + * scsi_abort_command - schedule a command abort
> + * @scmd:	scmd to abort.
> + *
> + * We only need to abort commands after a command timeout
> + */
> +void
> +scsi_abort_command(struct scsi_cmnd *scmd)
> +{
> +	unsigned long flags;
> +	int kick_worker = 0;
> +	struct scsi_device *sdev = scmd->device;
> +
> +	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);
> +	SCSI_LOG_ERROR_RECOVERY(3,
> +		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));

The printk can be moved outside the spinlock.  Who knows, maybe this
will become a scalability bottleneck before the millenium is over.

> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	if (kick_worker)
> +		schedule_work(&sdev->abort_work);
> +}
> +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.
> 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);

Function declarations in a .c file?  Ick!

>  
>  	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_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index e106c27..09237bf 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -2079,7 +2079,8 @@ fc_timed_out(struct scsi_cmnd *scmd)
>  	if (rport->port_state == FC_PORTSTATE_BLOCKED)
>  		return BLK_EH_RESET_TIMER;
>  
> -	return BLK_EH_NOT_HANDLED;
> +	scsi_abort_command(scmd);
> +	return BLK_EH_SCHEDULED;
>  }
>  
>  /*
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> index de5f5d8..460e514 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 void 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
> 

Jörn

--
You can't tell where a program is going to spend its time. Bottlenecks
occur in surprising places, so don't try to second guess and put in a
speed hack until you've proven that's where the bottleneck is.
-- Rob Pike
--
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/4] scsi: improved eh timeout handler

[Hannes Reinecke]

On 06/06/2013 06:23 PM, Jörn Engel wrote:
> On Thu, 6 June 2013 11:43:54 +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.
>> For any other return code from 'eh_abort_handler' the command
>> will be pushed onto the existing SCSI EH handler, or aborted
>> with DID_TIME_OUT if that fails.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/scsi_error.c        | 79 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/scsi/scsi_scan.c         |  3 ++
>>   drivers/scsi/scsi_transport_fc.c |  3 +-
>>   include/scsi/scsi_cmnd.h         |  1 +
>>   include/scsi/scsi_device.h       |  2 +
>>   5 files changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 96b4bb6..0a6b21c 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,83 @@ 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_Host *shost = sdev->host;
>> +	struct scsi_cmnd *scmd, *tmp;
>> +	unsigned long flags;
>> +	int rtn;
>> +
>> +	spin_lock_irqsave(&sdev->list_lock, flags);
>> +	list_for_each_entry_safe(scmd, tmp, &sdev->eh_abort_list, eh_entry) {
>> +		list_del_init(&scmd->eh_entry);
>
> The _init bit is not needed.  I prefer list_del, as the poisoning
> sometimes helps catching bugs.
>
Strictly speaking not.
But then I'm on the good side of programming who prefer to handle issues 
gracefully, not aborting with a kernel oops :-)

>> +		spin_unlock_irqrestore(&sdev->list_lock, flags);
>> +		SCSI_LOG_ERROR_RECOVERY(3,
>> +			scmd_printk(KERN_INFO, scmd,
>> +				    "aborting command %p\n", scmd));
>> +		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
>> +		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
>> +			if (((scmd->request->cmd_flags & REQ_FAILFAST_DEV) ||
>
> Am I being stupid again or should this be negated?
>
Knowing you I would think the former; where do you see the negation?

>> +			     (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);
>> +			}
>> +		} else {
>> +			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);
>> +			}
>> +		}
>> +		spin_lock_irqsave(&sdev->list_lock, flags);
>> +	}
>> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
>> +}
>> +
>> +/**
>> + * scsi_abort_command - schedule a command abort
>> + * @scmd:	scmd to abort.
>> + *
>> + * We only need to abort commands after a command timeout
>> + */
>> +void
>> +scsi_abort_command(struct scsi_cmnd *scmd)
>> +{
>> +	unsigned long flags;
>> +	int kick_worker = 0;
>> +	struct scsi_device *sdev = scmd->device;
>> +
>> +	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);
>> +	SCSI_LOG_ERROR_RECOVERY(3,
>> +		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
>
> The printk can be moved outside the spinlock.  Who knows, maybe this
> will become a scalability bottleneck before the millenium is over.
>
That's a debugging thing only. But yeah, you're right, it should move 
out of the spinlock.

>> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
>> +	if (kick_worker)
>> +		schedule_work(&sdev->abort_work);
>> +}
>> +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.
>> 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);
>
> Function declarations in a .c file?  Ick!
>
Sing-along: We didn't start the fire, it always burning ...

I'm just following precedents here.

Thanks for the review.

Cheers,

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

[Jörn Engel]

On Thu, 6 June 2013 22:39:14 +0200, Hannes Reinecke wrote:
> 
> >>+		spin_unlock_irqrestore(&sdev->list_lock, flags);
> >>+		SCSI_LOG_ERROR_RECOVERY(3,
> >>+			scmd_printk(KERN_INFO, scmd,
> >>+				    "aborting command %p\n", scmd));
> >>+		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
> >>+		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
> >>+			if (((scmd->request->cmd_flags & REQ_FAILFAST_DEV) ||
> >
> >Am I being stupid again or should this be negated?
> >
> Knowing you I would think the former; where do you see the negation?

If REQ_FAILFAST_DEV is set, this runs scsi_queue_insert(), which I
would expect it should run scsi_finish_command().

> >>+			     (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);
> >>+			}
> >>+		} else {
> >>+			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);
> >>+			}
> >>+		}
> >>+		spin_lock_irqsave(&sdev->list_lock, flags);
> >>+	}
> >>+	spin_unlock_irqrestore(&sdev->list_lock, flags);
...
> >>@@ -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);
> >
> >Function declarations in a .c file?  Ick!
> >
> Sing-along: We didn't start the fire, it always burning ...
> 
> I'm just following precedents here.

Fair enough.

Jörn

--
The cost of changing business rules is much more expensive for software
than for a secretary.
-- unknown
--
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/4] scsi: improved eh timeout handler

[Ren Mingxin]

Hi, Hannes:

On 06/07/2013 04:28 AM, Jörn Engel wrote:
> On Thu, 6 June 2013 22:39:14 +0200, Hannes Reinecke wrote:
>>>> +		spin_unlock_irqrestore(&sdev->list_lock, flags);
>>>> +		SCSI_LOG_ERROR_RECOVERY(3,
>>>> +			scmd_printk(KERN_INFO, scmd,
>>>> +				    "aborting command %p\n", scmd));
>>>> +		rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
>>>> +		if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
>>>> +			if (((scmd->request->cmd_flags&  REQ_FAILFAST_DEV) ||
>>>
>>> Am I being stupid again or should this be negated?
>>>
>> Knowing you I would think the former; where do you see the negation?
>
> If REQ_FAILFAST_DEV is set, this runs scsi_queue_insert(), which I
> would expect it should run scsi_finish_command().

I also think (scmd->request->cmd_flags & REQ_FAILFAST_DEV) and
(scmd->request->cmd_type == REQ_TYPE_BLOCK_PC) should be negated.
I'm confused why not use !scsi_noretry_cmd(scmd) directly as your
former patch here?

>>>> +			     (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);
>>>> +			}
>>>> +		} else {
>>>> +			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);
>>>> +			}
>>>> +		}
>>>> +		spin_lock_irqsave(&sdev->list_lock, flags);
>>>> +	}
>>>> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> ...
>>>> +/**
>>>> + * scsi_abort_command - schedule a command abort
>>>> + * @scmd:	scmd to abort.
>>>> + *
>>>> + * We only need to abort commands after a command timeout
>>>> + */
>>>> +void
>>>> +scsi_abort_command(struct scsi_cmnd *scmd)
>>>> +{
>>>> +	unsigned long flags;
>>>> +	int kick_worker = 0;
>>>> +	struct scsi_device *sdev = scmd->device;
>>>> +
>>>> +	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);
>>>> +	SCSI_LOG_ERROR_RECOVERY(3,
>>>> +		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
>>>> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
>>>> +	if (kick_worker)
>>>> +		schedule_work(&sdev->abort_work);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(scsi_abort_command);

Should the name of function above be more ideographic/understandable?
For example, scsi_abort_scmd_add? I was bewildered among functions
named scsi_abort_eh_cmnd, scsi_eh_abort_cmds...

Thanks,
Ren
--
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/4] scsi: improved eh timeout handler

[Hannes Reinecke]

On 06/07/2013 08:25 AM, Ren Mingxin wrote:
> Hi, Hannes:
> 
> On 06/07/2013 04:28 AM, Jörn Engel wrote:
>> On Thu, 6 June 2013 22:39:14 +0200, Hannes Reinecke wrote:
>>>>> +        spin_unlock_irqrestore(&sdev->list_lock, flags);
>>>>> +        SCSI_LOG_ERROR_RECOVERY(3,
>>>>> +            scmd_printk(KERN_INFO, scmd,
>>>>> +                    "aborting command %p\n", scmd));
>>>>> +        rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
>>>>> +        if (rtn == SUCCESS || rtn == FAST_IO_FAIL) {
>>>>> +            if (((scmd->request->cmd_flags&  REQ_FAILFAST_DEV) ||
>>>>
>>>> Am I being stupid again or should this be negated?
>>>>
>>> Knowing you I would think the former; where do you see the negation?
>>
>> If REQ_FAILFAST_DEV is set, this runs scsi_queue_insert(), which I
>> would expect it should run scsi_finish_command().
> 
> I also think (scmd->request->cmd_flags & REQ_FAILFAST_DEV) and
> (scmd->request->cmd_type == REQ_TYPE_BLOCK_PC) should be negated.
Bummer. You are correct.

So far I've only encountered the 'BLOCK_PC' condition, which worked.

> I'm confused why not use !scsi_noretry_cmd(scmd) directly as your
> former patch here?
> 
Hehe. I've fallen into the trap myself.
scsi_noretry_cmd() only works if you have a status for the command,
and will only evaluate REQ_FAILFAST_DEV or BLOCK_PC if the command
has a CHECK CONDITION status.
As this is the timeout handler we do _not_ have any status, so
scsi_noretry_cmd() will always tell us that the command should be
retried.

>>>>> +                 (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);
>>>>> +            }
>>>>> +        } else {
>>>>> +            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);
>>>>> +            }
>>>>> +        }
>>>>> +        spin_lock_irqsave(&sdev->list_lock, flags);
>>>>> +    }
>>>>> +    spin_unlock_irqrestore(&sdev->list_lock, flags);
>> ...
>>>>> +/**
>>>>> + * scsi_abort_command - schedule a command abort
>>>>> + * @scmd:    scmd to abort.
>>>>> + *
>>>>> + * We only need to abort commands after a command timeout
>>>>> + */
>>>>> +void
>>>>> +scsi_abort_command(struct scsi_cmnd *scmd)
>>>>> +{
>>>>> +    unsigned long flags;
>>>>> +    int kick_worker = 0;
>>>>> +    struct scsi_device *sdev = scmd->device;
>>>>> +
>>>>> +    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);
>>>>> +    SCSI_LOG_ERROR_RECOVERY(3,
>>>>> +        scmd_printk(KERN_INFO, scmd, "adding to
>>>>> eh_abort_list\n"));
>>>>> +    spin_unlock_irqrestore(&sdev->list_lock, flags);
>>>>> +    if (kick_worker)
>>>>> +        schedule_work(&sdev->abort_work);
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(scsi_abort_command);
> 
> Should the name of function above be more ideographic/understandable?
> For example, scsi_abort_scmd_add? I was bewildered among functions
> named scsi_abort_eh_cmnd, scsi_eh_abort_cmds...
> 
scsi_abort_scmd_add()? That's even more confusing.

scsi_abort_command() does exactly this, it aborts a command.
Yeah, the individual wrapper/callback function naming might be
improved, but this is the one function which actually does what it
advertises ...

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

[Jörn Engel]

On Thu, 6 June 2013 11:43:54 +0200, Hannes Reinecke wrote:
> 
> +	spin_lock_irqsave(&sdev->list_lock, flags);
> +	list_for_each_entry_safe(scmd, tmp, &sdev->eh_abort_list, eh_entry) {
> +		list_del_init(&scmd->eh_entry);
> +		spin_unlock_irqrestore(&sdev->list_lock, flags);

I never liked the list_for_each_entry_safe() loop but until this
morning couldn't really say why.  The problem is that it can finish
with commands left on the list.  And since your kick_worker checks for
list_empty, there wouldn't be any more aborts.

Thread1		list state		Thread2
		head->1->2->head
spin_lock_irqsave();
scmd = 1; tmp = 2;
list_del_init(scmd);
spin_unlock_irqrestore();
		head->2->head
					spin_lock_irqsave();
					kick_worker = 0;
					list_add();
					spin_unlock_irqrestore();
		head->3->2->head	
spin_lock_irqsave();
scmd = 2; tmp = head;
list_del_init(scmd);
spin_unlock_irqrestore();
		head->3->head

And now that I think about nasty races, you could also do
schedule_work() when the list is empty, but the worker thread is still
running.  I've had to debug similar cases and the approach is to waste
a day or five, despair and eventually get lucky half a year later when
you happen to notice the race.  Rare random crashes in the kworker
code a not my favorite sight.

> +void
> +scsi_abort_command(struct scsi_cmnd *scmd)
> +{
> +	unsigned long flags;
> +	int kick_worker = 0;
> +	struct scsi_device *sdev = scmd->device;
> +
> +	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);
> +	SCSI_LOG_ERROR_RECOVERY(3,
> +		scmd_printk(KERN_INFO, scmd, "adding to eh_abort_list\n"));
> +	spin_unlock_irqrestore(&sdev->list_lock, flags);
> +	if (kick_worker)
> +		schedule_work(&sdev->abort_work);
> +}
> +EXPORT_SYMBOL_GPL(scsi_abort_command);

Jörn

--
Time? What's that? Time is only worth what you do with it.
-- Theo de Raadt
--
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/4] scsi: improved eh timeout handler

[Baruch Even]

On Thu, Jun 6, 2013 at 12:43 PM, Hannes Reinecke <hare@suse.de> 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.
> For any other return code from 'eh_abort_handler' the command
> will be pushed onto the existing SCSI EH handler, or aborted
> with DID_TIME_OUT if that fails.
>

Why would you do a retry at this low level? We already have a bad path,
the IO may already be rerouted through another path and continuing with this
IO here may interfere with the multipath handling.


Baruch

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

[Hannes Reinecke]

On 06/10/2013 02:12 AM, Baruch Even wrote:
> On Thu, Jun 6, 2013 at 12:43 PM, Hannes Reinecke <hare@suse.de> 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.
>> For any other return code from 'eh_abort_handler' the command
>> will be pushed onto the existing SCSI EH handler, or aborted
>> with DID_TIME_OUT if that fails.
>>
> 
> Why would you do a retry at this low level? We already have a bad path,
> the IO may already be rerouted through another path and continuing with this
> IO here may interfere with the multipath handling.
> 
For multipathing no retries would be set, so the command would
be returned immediately.

I checked.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] virtio_scsi: use improved eh timeout handler

[Hannes Reinecke]

Hook in the improved eh timeout handler for virtio.

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

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index 2168258..3fb588d 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -654,6 +654,12 @@ 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)
+{
+	scsi_abort_command(scmd);
+	return BLK_EH_SCHEDULED;
+}
+
 static int virtscsi_target_alloc(struct scsi_target *starget)
 {
 	struct virtio_scsi_target_state *tgt =
@@ -683,6 +689,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 +706,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

Re: [PATCH 0/4] New SCSI command timeout handler

[Ren Mingxin]

Hi, Hannes:

On 06/06/2013 05:43 PM, Hannes Reinecke wrote:
> 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.

So this 31.5s is tested on virtio disks, right? Much faster than your
former test via fc.

This approach may not work for some LLDDs as you said, but I wonder
whether SAS is applicable(whether there will be later patches for
SAS).

Thanks,
Ren

Re: [PATCH 0/4] New SCSI command timeout handler

[Hannes Reinecke]

On 06/07/2013 08:54 AM, Ren Mingxin wrote:
> Hi, Hannes:
> 
> On 06/06/2013 05:43 PM, Hannes Reinecke wrote:
>> 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.
> 
> So this 31.5s is tested on virtio disks, right? Much faster than your
> former test via fc.
> 
No. This was done on HP Proliant /QLogic FC running against HP P2000.

There is a reason why I posted this new patchset :-)

(The old patchset had a bug where it wouldn't abort the TUR send by
multipaths path-checker. Which were causing delays during
switchover. This version now does terminate them properly)
(Incidentally, it was the non-working scsi_noretry_cmd() which
caused this bug :-)

> This approach may not work for some LLDDs as you said, but I wonder
> whether SAS is applicable(whether there will be later patches for
> SAS).
> 
SAS should be applicable for this, too. I'll check.

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 0/4] New SCSI command timeout handler

[Jörn Engel]

On Fri, 7 June 2013 14:54:16 +0800, Ren Mingxin wrote:
> On 06/06/2013 05:43 PM, Hannes Reinecke wrote:
> 
> This approach may not work for some LLDDs as you said, but I wonder
> whether SAS is applicable(whether there will be later patches for
> SAS).

We have tested a derived patch on SAS and it did improve things.

Jörn

--
You ain't got no problem, Jules. I'm on the motherfucker. Go back in
there, chill them niggers out and wait for the Wolf, who should be
coming directly.
-- Marsellus Wallace
--
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