* [PATCH 1/4] scsi: Fix erratic device offline during EH
2013-10-30 8:37 [PATCHv7 0/4] New EH command timeout handler Hannes Reinecke
@ 2013-10-30 8:37 ` Hannes Reinecke
2013-10-30 8:37 ` [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code Hannes Reinecke
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-10-30 8:37 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, linux-scsi, Ren Mingxin, Joern Engel,
James Smart, Hannes Reinecke
From: James Bottomley <jbottomley@parallels.com>
Commit 18a4d0a22ed6c54b67af7718c305cd010f09ddf8
(Handle disk devices which can not process medium access commands)
was introduced to offline any device which cannot process medium
access commands.
However, commit 3eef6257de48ff84a5d98ca533685df8a3beaeb8
(Reduce error recovery time by reducing use of TURs) reduced
the number of TURs by sending it only on the first failing
command, which might or might not be a medium access command.
So in combination this results in an erratic device offlining
during EH; if the command where the TUR was sent upon happens
to be a medium access command the device will be set offline,
if not everything proceeds as normal.
This patch moves the check to the final test, eliminating
this problem.
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: James Bottomley <jbottomley@parallels.com>
---
drivers/scsi/scsi_error.c | 26 +++++++++++++++++---------
drivers/scsi/sd.c | 26 ++++++++++++--------------
include/scsi/scsi_driver.h | 2 +-
3 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index e8bee9f..67c0014 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -941,12 +941,6 @@ retry:
scsi_eh_restore_cmnd(scmd, &ses);
- if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
- struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
- if (sdrv->eh_action)
- rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
- }
-
return rtn;
}
@@ -964,6 +958,16 @@ static int scsi_request_sense(struct scsi_cmnd *scmd)
return scsi_send_eh_cmnd(scmd, NULL, 0, scmd->device->eh_timeout, ~0);
}
+static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
+{
+ if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+ struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+ if (sdrv->eh_action)
+ rtn = sdrv->eh_action(scmd, rtn);
+ }
+ return rtn;
+}
+
/**
* scsi_eh_finish_cmd - Handle a cmd that eh is finished with.
* @scmd: Original SCSI cmd that eh has finished.
@@ -1142,7 +1146,9 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
list_for_each_entry_safe(scmd, next, cmd_list, eh_entry)
if (scmd->device == sdev) {
- if (finish_cmds)
+ if (finish_cmds &&
+ (try_stu ||
+ scsi_eh_action(scmd, SUCCESS) == SUCCESS))
scsi_eh_finish_cmd(scmd, done_q);
else
list_move_tail(&scmd->eh_entry, work_q);
@@ -1283,7 +1289,8 @@ static int scsi_eh_stu(struct Scsi_Host *shost,
!scsi_eh_tur(stu_scmd)) {
list_for_each_entry_safe(scmd, next,
work_q, eh_entry) {
- if (scmd->device == sdev)
+ if (scmd->device == sdev &&
+ scsi_eh_action(scmd, SUCCESS) == SUCCESS)
scsi_eh_finish_cmd(scmd, done_q);
}
}
@@ -1350,7 +1357,8 @@ static int scsi_eh_bus_device_reset(struct Scsi_Host *shost,
!scsi_eh_tur(bdr_scmd)) {
list_for_each_entry_safe(scmd, next,
work_q, eh_entry) {
- if (scmd->device == sdev)
+ if (scmd->device == sdev &&
+ scsi_eh_action(scmd, rtn) != FAILED)
scsi_eh_finish_cmd(scmd,
done_q);
}
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fd874b8..1929838 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -110,7 +110,7 @@ static int sd_suspend_runtime(struct device *);
static int sd_resume(struct device *);
static void sd_rescan(struct device *);
static int sd_done(struct scsi_cmnd *);
-static int sd_eh_action(struct scsi_cmnd *, unsigned char *, int, int);
+static int sd_eh_action(struct scsi_cmnd *, int);
static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
static void scsi_disk_release(struct device *cdev);
static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
@@ -1551,23 +1551,23 @@ static const struct block_device_operations sd_fops = {
/**
* sd_eh_action - error handling callback
* @scmd: sd-issued command that has failed
- * @eh_cmnd: The command that was sent during error handling
- * @eh_cmnd_len: Length of eh_cmnd in bytes
* @eh_disp: The recovery disposition suggested by the midlayer
*
- * This function is called by the SCSI midlayer upon completion of
- * an error handling command (TEST UNIT READY, START STOP UNIT,
- * etc.) The command sent to the device by the error handler is
- * stored in eh_cmnd. The result of sending the eh command is
- * passed in eh_disp.
+ * This function is called by the SCSI midlayer upon completion of an
+ * error test command (currently TEST UNIT READY). The result of sending
+ * the eh command is passed in eh_disp. We're looking for devices that
+ * fail medium access commands but are OK with non access commands like
+ * test unit ready (so wrongly see the device as having a successful
+ * recovery)
**/
-static int sd_eh_action(struct scsi_cmnd *scmd, unsigned char *eh_cmnd,
- int eh_cmnd_len, int eh_disp)
+static int sd_eh_action(struct scsi_cmnd *scmd, int eh_disp)
{
struct scsi_disk *sdkp = scsi_disk(scmd->request->rq_disk);
if (!scsi_device_online(scmd->device) ||
- !scsi_medium_access_command(scmd))
+ !scsi_medium_access_command(scmd) ||
+ host_byte(scmd->result) != DID_TIME_OUT ||
+ eh_disp != SUCCESS)
return eh_disp;
/*
@@ -1577,9 +1577,7 @@ static int sd_eh_action(struct scsi_cmnd *scmd, unsigned char *eh_cmnd,
* process of recovering or has it suffered an internal failure
* that prevents access to the storage medium.
*/
- if (host_byte(scmd->result) == DID_TIME_OUT && eh_disp == SUCCESS &&
- eh_cmnd_len && eh_cmnd[0] == TEST_UNIT_READY)
- sdkp->medium_access_timed_out++;
+ sdkp->medium_access_timed_out++;
/*
* If the device keeps failing read/write commands but TEST UNIT
diff --git a/include/scsi/scsi_driver.h b/include/scsi/scsi_driver.h
index d443aa0..20fdfc2 100644
--- a/include/scsi/scsi_driver.h
+++ b/include/scsi/scsi_driver.h
@@ -16,7 +16,7 @@ struct scsi_driver {
void (*rescan)(struct device *);
int (*done)(struct scsi_cmnd *);
- int (*eh_action)(struct scsi_cmnd *, unsigned char *, int, int);
+ int (*eh_action)(struct scsi_cmnd *, int);
};
#define to_scsi_driver(drv) \
container_of((drv), struct scsi_driver, gendrv)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code
2013-10-30 8:37 [PATCHv7 0/4] New EH command timeout handler Hannes Reinecke
2013-10-30 8:37 ` [PATCH 1/4] scsi: Fix erratic device offline during EH Hannes Reinecke
@ 2013-10-30 8:37 ` Hannes Reinecke
2013-10-30 8:43 ` Christoph Hellwig
2013-10-30 8:37 ` [PATCH 3/4] scsi: improved eh timeout handler Hannes Reinecke
2013-10-30 8:37 ` [PATCH 4/4] scsi: Update documentation Hannes Reinecke
3 siblings, 1 reply; 16+ messages in thread
From: Hannes Reinecke @ 2013-10-30 8:37 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, linux-scsi, Ren Mingxin, Joern Engel,
James Smart, 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 | 4 ++++
include/linux/blkdev.h | 1 +
2 files changed, 5 insertions(+)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 67c0014..af51a9c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -161,6 +161,10 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
else if (host->hostt->eh_timed_out)
rtn = host->hostt->eh_timed_out(scmd);
+ /* Check for asynchronous command aborts */
+ if (rtn == BLK_EH_SCHEDULED)
+ return BLK_EH_NOT_HANDLED;
+
scmd->result |= DID_TIME_OUT << 16;
if (unlikely(rtn == BLK_EH_NOT_HANDLED &&
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 0e6f765..afec57c 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.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code
2013-10-30 8:37 ` [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code Hannes Reinecke
@ 2013-10-30 8:43 ` Christoph Hellwig
2013-10-30 19:15 ` Hannes Reinecke
0 siblings, 1 reply; 16+ messages in thread
From: Christoph Hellwig @ 2013-10-30 8:43 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, Christoph Hellwig, linux-scsi, Ren Mingxin,
Joern Engel, James Smart
On Wed, Oct 30, 2013 at 09:37:50AM +0100, Hannes Reinecke wrote:
> Add a 'BLK_EH_SCHEDULED' return code for blk-timeout to indicate
> that a delayed error recovery has been initiated.
The user introduces here gets immediately reverted in the next patch,
which means this patch can simply be dropped.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code
2013-10-30 8:43 ` Christoph Hellwig
@ 2013-10-30 19:15 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-10-30 19:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: James Bottomley, linux-scsi, Ren Mingxin, Joern Engel,
James Smart
On 10/30/2013 09:43 AM, Christoph Hellwig wrote:
> On Wed, Oct 30, 2013 at 09:37:50AM +0100, Hannes Reinecke wrote:
>> Add a 'BLK_EH_SCHEDULED' return code for blk-timeout to indicate
>> that a delayed error recovery has been initiated.
>
> The user introduces here gets immediately reverted in the next patch,
> which means this patch can simply be dropped.
>
Rah. You are right.
Will be redoing the 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/4] scsi: improved eh timeout handler
2013-10-30 8:37 [PATCHv7 0/4] New EH command timeout handler Hannes Reinecke
2013-10-30 8:37 ` [PATCH 1/4] scsi: Fix erratic device offline during EH Hannes Reinecke
2013-10-30 8:37 ` [PATCH 2/4] blk-timeout: add BLK_EH_SCHEDULED return code Hannes Reinecke
@ 2013-10-30 8:37 ` Hannes Reinecke
2013-10-30 8:37 ` [PATCH 4/4] scsi: Update documentation Hannes Reinecke
3 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-10-30 8:37 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, linux-scsi, Ren Mingxin, Joern Engel,
James Smart, 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 | 9 +--
drivers/scsi/scsi_error.c | 154 +++++++++++++++++++++++++++++++++++++++++-----
drivers/scsi/scsi_priv.h | 2 +
include/scsi/scsi_cmnd.h | 2 +
include/scsi/scsi_host.h | 5 ++
5 files changed, 152 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index fe0bcb1..d8afec8 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);
@@ -742,15 +745,13 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
}
/**
- * scsi_done - Enqueue the finished SCSI command into the done queue.
+ * scsi_done - Invoke completion on finished SCSI command.
* @cmd: The SCSI Command for which a low-level device driver (LLDD) gives
* ownership back to SCSI Core -- i.e. the LLDD has finished with it.
*
* Description: This function is the mid-level's (SCSI Core) interrupt routine,
* which regains ownership of the SCSI command (de facto) from a LLDD, and
- * enqueues the command to the done queue for further processing.
- *
- * This is the producer of the done queue who enqueues at the tail.
+ * calls blk_complete_request() for further processing.
*
* This function is interrupt context safe.
*/
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index af51a9c..8bd2615 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -53,6 +53,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 *,
+ struct scsi_cmnd *);
/* called with shost->host_lock held */
void scsi_eh_wakeup(struct Scsi_Host *shost)
@@ -100,6 +102,117 @@ 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
+ */
+int
+scsi_abort_command(struct scsi_cmnd *scmd)
+{
+ struct scsi_device *sdev = scmd->device;
+ struct Scsi_Host *shost = sdev->host;
+ unsigned long flags;
+
+ if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) {
+ /*
+ * command abort timed out.
+ */
+ SCSI_LOG_ERROR_RECOVERY(3,
+ scmd_printk(KERN_INFO, scmd,
+ "scmd %p abort timeout\n", scmd));
+ cancel_delayed_work(&scmd->abort_work);
+ return FAILED;
+ }
+
+ /*
+ * Do not try a command abort if
+ * SCSI EH has already started.
+ */
+ spin_lock_irqsave(shost->host_lock, flags);
+ if (scsi_host_in_recovery(shost)) {
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ SCSI_LOG_ERROR_RECOVERY(3,
+ scmd_printk(KERN_INFO, scmd,
+ "scmd %p not aborting, host in recovery\n",
+ scmd));
+ return FAILED;
+ }
+
+ if (shost->eh_deadline && !shost->last_reset)
+ shost->last_reset = jiffies;
+ spin_unlock_irqrestore(shost->host_lock, flags);
+
+ scmd->eh_eflags |= SCSI_EH_ABORT_SCHEDULED;
+ SCSI_LOG_ERROR_RECOVERY(3,
+ scmd_printk(KERN_INFO, scmd,
+ "scmd %p abort scheduled\n", scmd));
+ schedule_delayed_work(&scmd->abort_work, HZ / 100);
+ return SUCCESS;
+}
+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 +238,8 @@ int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
shost->last_reset = jiffies;
ret = 1;
+ if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
+ eh_flag &= ~SCSI_EH_CANCEL_CMD;
scmd->eh_eflags |= eh_flag;
list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
shost->host_failed++;
@@ -161,9 +276,9 @@ 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);
- /* Check for asynchronous command aborts */
- if (rtn == BLK_EH_SCHEDULED)
- return BLK_EH_NOT_HANDLED;
+ if (rtn == BLK_EH_NOT_HANDLED && !host->hostt->no_async_abort)
+ if (scsi_abort_command(scmd) == SUCCESS)
+ return BLK_EH_NOT_HANDLED;
scmd->result |= DID_TIME_OUT << 16;
@@ -1581,7 +1696,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 +1704,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 +1719,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 +1781,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);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5460849..85430ae 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -476,6 +476,11 @@ struct scsi_host_template {
unsigned ordered_tag:1;
/*
+ * True if asynchronous aborts are not supported
+ */
+ unsigned no_async_abort:1;
+
+ /*
* Countdown for host blocking with no commands outstanding.
*/
unsigned int max_host_blocked;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/4] scsi: Update documentation
2013-10-30 8:37 [PATCHv7 0/4] New EH command timeout handler Hannes Reinecke
` (2 preceding siblings ...)
2013-10-30 8:37 ` [PATCH 3/4] scsi: improved eh timeout handler Hannes Reinecke
@ 2013-10-30 8:37 ` Hannes Reinecke
3 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-10-30 8:37 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, linux-scsi, Ren Mingxin, Joern Engel,
James Smart, Hannes Reinecke
The documentation has gone out-of-sync, so update it to
the current status.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
Documentation/scsi/scsi_eh.txt | 69 +++++++++++++++++++--------------
Documentation/scsi/scsi_mid_low_api.txt | 9 ++++-
2 files changed, 46 insertions(+), 32 deletions(-)
diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 6ff16b6..a0c8511 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -42,20 +42,14 @@ discussion.
Once LLDD gets hold of a scmd, either the LLDD will complete the
command by calling scsi_done callback passed from midlayer when
-invoking hostt->queuecommand() or SCSI midlayer will time it out.
+invoking hostt->queuecommand() or the block layer will time it out.
[1-2-1] Completing a scmd w/ scsi_done
For all non-EH commands, scsi_done() is the completion callback. It
-does the following.
-
- 1. Delete timeout timer. If it fails, it means that timeout timer
- has expired and is going to finish the command. Just return.
-
- 2. Link scmd to per-cpu scsi_done_q using scmd->en_entry
-
- 3. Raise SCSI_SOFTIRQ
+just calls blk_complete_request() to delete the block layer timer and
+raise SCSI_SOFTIRQ
SCSI_SOFTIRQ handler scsi_softirq calls scsi_decide_disposition() to
determine what to do with the command. scsi_decide_disposition()
@@ -64,10 +58,12 @@ with the command.
- SUCCESS
scsi_finish_command() is invoked for the command. The
- function does some maintenance choirs and notify completion by
- calling scmd->done() callback, which, for fs requests, would
- be HLD completion callback - sd:sd_rw_intr, sr:rw_intr,
- st:st_intr.
+ function does some maintenance chores and then calls
+ scsi_io_completion() to finish the I/O.
+ scsi_io_completion() then notifies the block layer on
+ the completed request by calling blk_end_request and
+ friends or figures out what to do with the remainder
+ of the data in case of an error.
- NEEDS_RETRY
- ADD_TO_MLQUEUE
@@ -86,33 +82,45 @@ function
1. invokes optional hostt->eh_timed_out() callback. Return value can
be one of
- - EH_HANDLED
- This indicates that eh_timed_out() dealt with the timeout. The
- scmd is passed to __scsi_done() and thus linked into per-cpu
- scsi_done_q. Normal command completion described in [1-2-1]
- follows.
+ - BLK_EH_HANDLED
+ This indicates that eh_timed_out() dealt with the timeout.
+ The command is passed back to the block layer and completed
+ via __blk_complete_requests().
+
+ *NOTE* After returning BLK_EH_HANDLED the SCSI layer is
+ assumed to be finished with the command, and no other
+ functions from the SCSI layer will be called. So this
+ should typically only be returned if the eh_timed_out()
+ handler raced with normal completion.
- - EH_RESET_TIMER
+ - BLK_EH_RESET_TIMER
This indicates that more time is required to finish the
command. Timer is restarted. This action is counted as a
retry and only allowed scmd->allowed + 1(!) times. Once the
- limit is reached, action for EH_NOT_HANDLED is taken instead.
+ limit is reached, action for BLK_EH_NOT_HANDLED is taken instead.
- *NOTE* This action is racy as the LLDD could finish the scmd
- after the timeout has expired but before it's added back. In
- such cases, scsi_done() would think that timeout has occurred
- and return without doing anything. We lose completion and the
- command will time out again.
-
- - EH_NOT_HANDLED
- This is the same as when eh_timed_out() callback doesn't exist.
+ - BLK_EH_NOT_HANDLED
+ eh_timed_out() callback did not handle the command.
Step #2 is taken.
+ 2. If the host supports asynchronous completion (as indicated by the
+ no_async_abort setting in the host template) scsi_abort_command()
+ is invoked to schedule an asynchrous abort. If that fails
+ Step #3 is taken.
+
2. scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) is invoked for the
command. See [1-3] for more information.
+[1-3] Asynchronous command aborts
+
+ After a timeout occurs a command abort is scheduled from
+ scsi_abort_command(). If the abort is successful the command
+ will either be retried (if the number of retries is not exhausted)
+ or terminated with DID_TIME_OUT.
+ Otherwise scsi_eh_scmd_add() is invoked for the command.
+ See [1-4] for more information.
-[1-3] How EH takes over
+[1-4] How EH takes over
scmds enter EH via scsi_eh_scmd_add(), which does the following.
@@ -320,7 +328,8 @@ scmd->allowed.
<<scsi_eh_abort_cmds>>
- This action is taken for each timed out command.
+ This action is taken for each timed out command when
+ no_async_abort is enabled in the host template.
hostt->eh_abort_handler() is invoked for each scmd. The
handler returns SUCCESS if it has succeeded to make LLDD and
all related hardware forget about the scmd.
diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
index 2b06aba..d6a9bde 100644
--- a/Documentation/scsi/scsi_mid_low_api.txt
+++ b/Documentation/scsi/scsi_mid_low_api.txt
@@ -882,8 +882,11 @@ Details:
*
* Calling context: kernel thread
*
- * Notes: Invoked from scsi_eh thread. No other commands will be
- * queued on current host during eh.
+ * Notes: If 'no_async_abort' is defined this callback
+ * will be invoked from scsi_eh thread. No other commands
+ * will then be queued on current host during eh.
+ * Otherwise it will be called whenever scsi_times_out()
+ * is called due to a command timeout.
*
* Optionally defined in: LLD
**/
@@ -1257,6 +1260,8 @@ of interest:
address space
use_clustering - 1=>SCSI commands in mid level's queue can be merged,
0=>disallow SCSI command merging
+ no_async_abort - 1=>Asynchronous aborts are not supported
+ 0=>Timed-out commands will be aborted asynchronously
hostt - pointer to driver's struct scsi_host_template from which
this struct Scsi_Host instance was spawned
hostt->proc_name - name of LLD. This is the driver name that sysfs uses
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 9:43 [PATCH 0/4] New SCSI command timeout handler Hannes Reinecke
@ 2013-06-06 9:43 ` Hannes Reinecke
2013-06-06 16:23 ` Jörn Engel
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-06-06 9:43 UTC (permalink / raw)
To: James Bottomley
Cc: linux-scsi, Joern Engel, Ewan Milne, James Smart, Ren Mingxin,
Roland Dreier, Bryn Reeves, Christoph Hellwig, 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 9:43 ` [PATCH 3/4] scsi: improved eh " Hannes Reinecke
@ 2013-06-06 16:23 ` Jörn Engel
2013-06-06 20:39 ` Hannes Reinecke
2013-06-07 16:21 ` Jörn Engel
2013-06-10 0:12 ` Baruch Even
2 siblings, 1 reply; 16+ messages in thread
From: Jörn Engel @ 2013-06-06 16:23 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, linux-scsi, Ewan Milne, James Smart, Ren Mingxin,
Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 16:23 ` Jörn Engel
@ 2013-06-06 20:39 ` Hannes Reinecke
2013-06-06 20:28 ` Jörn Engel
0 siblings, 1 reply; 16+ messages in thread
From: Hannes Reinecke @ 2013-06-06 20:39 UTC (permalink / raw)
To: Jörn Engel
Cc: James Bottomley, linux-scsi, Ewan Milne, James Smart, Ren Mingxin,
Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 20:39 ` Hannes Reinecke
@ 2013-06-06 20:28 ` Jörn Engel
2013-06-07 6:25 ` Ren Mingxin
0 siblings, 1 reply; 16+ messages in thread
From: Jörn Engel @ 2013-06-06 20:28 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, linux-scsi, Ewan Milne, James Smart, Ren Mingxin,
Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 20:28 ` Jörn Engel
@ 2013-06-07 6:25 ` Ren Mingxin
2013-06-07 6:42 ` Hannes Reinecke
0 siblings, 1 reply; 16+ messages in thread
From: Ren Mingxin @ 2013-06-07 6:25 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Jörn Engel, James Bottomley, linux-scsi, Ewan Milne,
James Smart, Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-07 6:25 ` Ren Mingxin
@ 2013-06-07 6:42 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-06-07 6:42 UTC (permalink / raw)
To: Ren Mingxin
Cc: Jörn Engel, James Bottomley, linux-scsi, Ewan Milne,
James Smart, Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 9:43 ` [PATCH 3/4] scsi: improved eh " Hannes Reinecke
2013-06-06 16:23 ` Jörn Engel
@ 2013-06-07 16:21 ` Jörn Engel
2013-06-10 0:12 ` Baruch Even
2 siblings, 0 replies; 16+ messages in thread
From: Jörn Engel @ 2013-06-07 16:21 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, linux-scsi, Ewan Milne, James Smart, Ren Mingxin,
Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-06 9:43 ` [PATCH 3/4] scsi: improved eh " Hannes Reinecke
2013-06-06 16:23 ` Jörn Engel
2013-06-07 16:21 ` Jörn Engel
@ 2013-06-10 0:12 ` Baruch Even
2013-06-10 5:48 ` Hannes Reinecke
2 siblings, 1 reply; 16+ messages in thread
From: Baruch Even @ 2013-06-10 0:12 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, linux-scsi, Joern Engel, Ewan Milne, James Smart,
Ren Mingxin, Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] scsi: improved eh timeout handler
2013-06-10 0:12 ` Baruch Even
@ 2013-06-10 5:48 ` Hannes Reinecke
0 siblings, 0 replies; 16+ messages in thread
From: Hannes Reinecke @ 2013-06-10 5:48 UTC (permalink / raw)
To: Baruch Even
Cc: James Bottomley, linux-scsi, Joern Engel, Ewan Milne, James Smart,
Ren Mingxin, Roland Dreier, Bryn Reeves, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 16+ messages in thread