* [RFC PATCH 0/3] scsi: make asynchronous aborts mandatory
@ 2014-11-04 12:11 Hannes Reinecke
2014-11-04 12:11 ` [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed Hannes Reinecke
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Hannes Reinecke @ 2014-11-04 12:11 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, Ewan Milne, Robert Elliott, linux-scsi,
Hannes Reinecke
Hi all,
asynchronous abort have been activated since v3.14, and we haven't
received any failure reports so far. So we should make asynchronous
aborts mandatory and remove the no_async_aborts flag from the SCSI
host structure.
With this we can also cleanup the failure path in SCSI EH.
Hannes Reinecke (3):
scsi: make scsi_eh_scmd_add() always succeed
scsi: make eh_eflags persistent
scsi: make asynchronous aborts mandatory
Documentation/scsi/scsi_eh.txt | 31 +++++------
drivers/scsi/scsi_error.c | 113 +++++++----------------------------------
drivers/scsi/scsi_lib.c | 4 +-
drivers/scsi/scsi_priv.h | 3 +-
include/scsi/scsi_eh.h | 1 +
include/scsi/scsi_host.h | 5 --
6 files changed, 34 insertions(+), 123 deletions(-)
--
1.8.5.2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed
2014-11-04 12:11 [RFC PATCH 0/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
@ 2014-11-04 12:11 ` Hannes Reinecke
2014-11-04 18:36 ` Christoph Hellwig
2014-11-04 12:11 ` [PATCH 2/3] scsi: make eh_eflags persistent Hannes Reinecke
2014-11-04 12:11 ` [PATCH 3/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
2 siblings, 1 reply; 7+ messages in thread
From: Hannes Reinecke @ 2014-11-04 12:11 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, Ewan Milne, Robert Elliott, linux-scsi,
Hannes Reinecke
BLK_EH_HANDLED does not work with scsi commands, as we need
to release the associated buffers correctly.
And scsi_eh_scmd_add() currently only will fail if no
error handler thread is started (which will never be the
case) or if the state machine encounters an illegal transition.
As state machine transitions don't have any real meaning
we can also force setting of the new state and
make scsi_dh_scmd_add() a void function.
With that we'll never have to resort to return
BLK_EH_HANDLED.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/scsi/scsi_error.c | 30 +++++++++---------------------
drivers/scsi/scsi_lib.c | 4 ++--
drivers/scsi/scsi_priv.h | 2 +-
3 files changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index fa7b5ec..c71d61f 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -163,14 +163,7 @@ scmd_eh_abort_handler(struct work_struct *work)
}
}
- if (!scsi_eh_scmd_add(scmd, 0)) {
- SCSI_LOG_ERROR_RECOVERY(3,
- scmd_printk(KERN_WARNING, scmd,
- "scmd %p terminate "
- "aborted command\n", scmd));
- set_host_byte(scmd, DID_TIME_OUT);
- scsi_finish_command(scmd);
- }
+ scsi_eh_scmd_add(scmd, 0);
}
/**
@@ -228,37 +221,33 @@ scsi_abort_command(struct scsi_cmnd *scmd)
* scsi_eh_scmd_add - add scsi cmd to error handling.
* @scmd: scmd to run eh on.
* @eh_flag: optional SCSI_EH flag.
- *
- * Return value:
- * 0 on failure.
*/
-int scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
{
struct Scsi_Host *shost = scmd->device->host;
unsigned long flags;
- int ret = 0;
- if (!shost->ehandler)
- return 0;
+ WARN_ON(!shost->ehandler);
spin_lock_irqsave(shost->host_lock, flags);
+ /*
+ * Force SHOST_RECOVERY even for illegal state transitions;
+ * we cannot handle failed commands otherwise.
+ */
if (scsi_host_set_state(shost, SHOST_RECOVERY))
if (scsi_host_set_state(shost, SHOST_CANCEL_RECOVERY))
- goto out_unlock;
+ shost->shost_state = SHOST_RECOVERY;
if (shost->eh_deadline != -1 && !shost->last_reset)
shost->last_reset = jiffies;
- ret = 1;
if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
eh_flag &= ~SCSI_EH_CANCEL_CMD;
scmd->eh_eflags |= eh_flag;
list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
shost->host_failed++;
scsi_eh_wakeup(shost);
- out_unlock:
spin_unlock_irqrestore(shost->host_lock, flags);
- return ret;
}
/**
@@ -294,8 +283,7 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
return BLK_EH_NOT_HANDLED;
set_host_byte(scmd, DID_TIME_OUT);
- if (!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))
- rtn = BLK_EH_HANDLED;
+ scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
}
return rtn;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fc0a8a0..26f7dcf 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1647,8 +1647,8 @@ static void scsi_softirq_done(struct request *rq)
scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
break;
default:
- if (!scsi_eh_scmd_add(cmd, 0))
- scsi_finish_command(cmd);
+ scsi_eh_scmd_add(cmd, 0);
+ break;
}
}
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 12b8e1b..b2d1a2b 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -72,7 +72,7 @@ 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);
extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern int scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
void scsi_eh_ready_devs(struct Scsi_Host *shost,
struct list_head *work_q,
struct list_head *done_q);
--
1.8.5.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] scsi: make eh_eflags persistent
2014-11-04 12:11 [RFC PATCH 0/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
2014-11-04 12:11 ` [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed Hannes Reinecke
@ 2014-11-04 12:11 ` Hannes Reinecke
2014-11-04 12:11 ` [PATCH 3/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
2 siblings, 0 replies; 7+ messages in thread
From: Hannes Reinecke @ 2014-11-04 12:11 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, Ewan Milne, Robert Elliott, linux-scsi,
Hannes Reinecke
To detect if a failed command has been retried we must not
clear scmd->eh_eflags when EH finishes.
The flag should be persistent throughout the lifetime
of the command.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
Documentation/scsi/scsi_eh.txt | 3 ---
drivers/scsi/scsi_error.c | 4 ++--
include/scsi/scsi_eh.h | 1 +
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index a0c8511..011c03d 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -264,7 +264,6 @@ scmd->allowed.
3. scmd recovered
ACTION: scsi_eh_finish_cmd() is invoked to EH-finish scmd
- shost->host_failed--
- - clear scmd->eh_eflags
- scsi_setup_cmd_retry()
- move from local eh_work_q to local eh_done_q
LOCKING: none
@@ -452,8 +451,6 @@ except for #1 must be implemented by eh_strategy_handler().
- shost->host_failed is zero.
- - Each scmd's eh_eflags field is cleared.
-
- Each scmd is in such a state that scsi_setup_cmd_retry() on the
scmd doesn't make any difference.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c71d61f..27cb30d 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -183,7 +183,6 @@ scsi_abort_command(struct scsi_cmnd *scmd)
/*
* Retry after abort failed, escalate to next level.
*/
- scmd->eh_eflags &= ~SCSI_EH_ABORT_SCHEDULED;
SCSI_LOG_ERROR_RECOVERY(3,
scmd_printk(KERN_INFO, scmd,
"scmd %p previous abort failed\n", scmd));
@@ -926,6 +925,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
ses->result = scmd->result;
ses->underflow = scmd->underflow;
ses->prot_op = scmd->prot_op;
+ ses->eh_eflags = scmd->eh_eflags;
scmd->prot_op = SCSI_PROT_NORMAL;
scmd->eh_eflags = 0;
@@ -989,6 +989,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
scmd->result = ses->result;
scmd->underflow = ses->underflow;
scmd->prot_op = ses->prot_op;
+ scmd->eh_eflags = ses->eh_eflags;
}
EXPORT_SYMBOL(scsi_eh_restore_cmnd);
@@ -1122,7 +1123,6 @@ static int scsi_eh_action(struct scsi_cmnd *scmd, int rtn)
void scsi_eh_finish_cmd(struct scsi_cmnd *scmd, struct list_head *done_q)
{
scmd->device->host->host_failed--;
- scmd->eh_eflags = 0;
list_move_tail(&scmd->eh_entry, done_q);
}
EXPORT_SYMBOL(scsi_eh_finish_cmd);
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 2562481..4973844 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -78,6 +78,7 @@ extern int scsi_reset_provider(struct scsi_device *, int);
struct scsi_eh_save {
/* saved state */
int result;
+ int eh_eflags;
enum dma_data_direction data_direction;
unsigned underflow;
unsigned char cmd_len;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] scsi: make asynchronous aborts mandatory
2014-11-04 12:11 [RFC PATCH 0/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
2014-11-04 12:11 ` [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed Hannes Reinecke
2014-11-04 12:11 ` [PATCH 2/3] scsi: make eh_eflags persistent Hannes Reinecke
@ 2014-11-04 12:11 ` Hannes Reinecke
2014-11-04 18:52 ` Christoph Hellwig
2 siblings, 1 reply; 7+ messages in thread
From: Hannes Reinecke @ 2014-11-04 12:11 UTC (permalink / raw)
To: James Bottomley
Cc: Christoph Hellwig, Ewan Milne, Robert Elliott, linux-scsi,
Hannes Reinecke
There hasn't been any reports for HBAs where asynchronous abort
would not work, so we should make it mandatory and remove
the fallback.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
Documentation/scsi/scsi_eh.txt | 28 +++++++-------
drivers/scsi/scsi_error.c | 85 +++++-------------------------------------
drivers/scsi/scsi_lib.c | 2 +-
drivers/scsi/scsi_priv.h | 3 +-
include/scsi/scsi_host.h | 5 ---
5 files changed, 24 insertions(+), 99 deletions(-)
diff --git a/Documentation/scsi/scsi_eh.txt b/Documentation/scsi/scsi_eh.txt
index 011c03d..cbb07f2 100644
--- a/Documentation/scsi/scsi_eh.txt
+++ b/Documentation/scsi/scsi_eh.txt
@@ -70,7 +70,7 @@ with the command.
scmd is requeued to blk queue.
- otherwise
- scsi_eh_scmd_add(scmd, 0) is invoked for the command. See
+ scsi_eh_scmd_add(scmd) is invoked for the command. See
[1-3] for details of this function.
@@ -103,13 +103,15 @@ function
eh_timed_out() callback did not handle the command.
Step #2 is taken.
- 2. If the host supports asynchronous completion (as indicated by the
- no_async_abort setting in the host template) scsi_abort_command()
- is invoked to schedule an asynchrous abort. If that fails
- Step #3 is taken.
+ 2. scsi_abort_command() is invoked to schedule an asynchrous abort
+ (Seee [1-3] for more information).
+ Asynchronous abort are not invoked for commands which have
+ SCSI_EH_ABORT_SCHEDULED set (this indicates that the command
+ already had been aborted once, and this is a retry which failed),
+ or when the EH deadline is expired. In these case Step #3 is taken.
- 2. scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD) is invoked for the
- command. See [1-3] for more information.
+ 3. scsi_eh_scmd_add(scmd) is invoked for the
+ command. See [1-4] for more information.
[1-3] Asynchronous command aborts
@@ -124,16 +126,13 @@ function
scmds enter EH via scsi_eh_scmd_add(), which does the following.
- 1. Turns on scmd->eh_eflags as requested. It's 0 for error
- completions and SCSI_EH_CANCEL_CMD for timeouts.
+ 1. Links scmd->eh_entry to shost->eh_cmd_q
- 2. Links scmd->eh_entry to shost->eh_cmd_q
+ 2. Sets SHOST_RECOVERY bit in shost->shost_state
- 3. Sets SHOST_RECOVERY bit in shost->shost_state
+ 3. Increments shost->host_failed
- 4. Increments shost->host_failed
-
- 5. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
+ 4. Wakes up SCSI EH thread if shost->host_busy == shost->host_failed
As can be seen above, once any scmd is added to shost->eh_cmd_q,
SHOST_RECOVERY shost_state bit is turned on. This prevents any new
@@ -249,7 +248,6 @@ scmd->allowed.
1. Error completion / time out
ACTION: scsi_eh_scmd_add() is invoked for scmd
- - set scmd->eh_eflags
- add scmd to shost->eh_cmd_q
- set SHOST_RECOVERY
- shost->host_failed++
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 27cb30d..cee85e9 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -163,7 +163,7 @@ scmd_eh_abort_handler(struct work_struct *work)
}
}
- scsi_eh_scmd_add(scmd, 0);
+ scsi_eh_scmd_add(scmd);
}
/**
@@ -219,9 +219,8 @@ scsi_abort_command(struct scsi_cmnd *scmd)
/**
* scsi_eh_scmd_add - add scsi cmd to error handling.
* @scmd: scmd to run eh on.
- * @eh_flag: optional SCSI_EH flag.
*/
-void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
+void scsi_eh_scmd_add(struct scsi_cmnd *scmd)
{
struct Scsi_Host *shost = scmd->device->host;
unsigned long flags;
@@ -240,9 +239,6 @@ void scsi_eh_scmd_add(struct scsi_cmnd *scmd, int eh_flag)
if (shost->eh_deadline != -1 && !shost->last_reset)
shost->last_reset = jiffies;
- if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
- eh_flag &= ~SCSI_EH_CANCEL_CMD;
- scmd->eh_eflags |= eh_flag;
list_add_tail(&scmd->eh_entry, &shost->eh_cmd_q);
shost->host_failed++;
scsi_eh_wakeup(shost);
@@ -277,12 +273,10 @@ enum blk_eh_timer_return scsi_times_out(struct request *req)
rtn = host->hostt->eh_timed_out(scmd);
if (rtn == BLK_EH_NOT_HANDLED) {
- if (!host->hostt->no_async_abort &&
- scsi_abort_command(scmd) == SUCCESS)
- return BLK_EH_NOT_HANDLED;
-
- set_host_byte(scmd, DID_TIME_OUT);
- scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD);
+ if (scsi_abort_command(scmd) != SUCCESS) {
+ set_host_byte(scmd, DID_TIME_OUT);
+ scsi_eh_scmd_add(scmd);
+ }
}
return rtn;
@@ -334,7 +328,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
list_for_each_entry(scmd, work_q, eh_entry) {
if (scmd->device == sdev) {
++total_failures;
- if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD)
+ if (scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED)
++cmd_cancel;
else
++cmd_failed;
@@ -1155,7 +1149,7 @@ int scsi_eh_get_sense(struct list_head *work_q,
int rtn;
list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
- if ((scmd->eh_eflags & SCSI_EH_CANCEL_CMD) ||
+ if ((scmd->eh_eflags & SCSI_EH_ABORT_SCHEDULED) ||
SCSI_SENSE_VALID(scmd))
continue;
@@ -1296,61 +1290,6 @@ static int scsi_eh_test_devices(struct list_head *cmd_list,
return list_empty(work_q);
}
-
-/**
- * scsi_eh_abort_cmds - abort pending commands.
- * @work_q: &list_head for pending commands.
- * @done_q: &list_head for processed commands.
- *
- * Decription:
- * Try and see whether or not it makes sense to try and abort the
- * running command. This only works out to be the case if we have one
- * command that has timed out. If the command simply failed, it makes
- * no sense to try and abort the command, since as far as the shost
- * adapter is concerned, it isn't running.
- */
-static int scsi_eh_abort_cmds(struct list_head *work_q,
- struct list_head *done_q)
-{
- struct scsi_cmnd *scmd, *next;
- LIST_HEAD(check_list);
- int rtn;
- struct Scsi_Host *shost;
-
- list_for_each_entry_safe(scmd, next, work_q, eh_entry) {
- if (!(scmd->eh_eflags & SCSI_EH_CANCEL_CMD))
- continue;
- shost = scmd->device->host;
- if (scsi_host_eh_past_deadline(shost)) {
- list_splice_init(&check_list, work_q);
- SCSI_LOG_ERROR_RECOVERY(3,
- scmd_printk(KERN_INFO, scmd,
- "%s: skip aborting cmd, past eh deadline\n",
- current->comm));
- return list_empty(work_q);
- }
- SCSI_LOG_ERROR_RECOVERY(3,
- scmd_printk(KERN_INFO, scmd,
- "%s: aborting cmd\n", current->comm));
- rtn = scsi_try_to_abort_cmd(shost->hostt, scmd);
- if (rtn == FAILED) {
- SCSI_LOG_ERROR_RECOVERY(3,
- scmd_printk(KERN_INFO, scmd,
- "%s: aborting cmd failed\n",
- current->comm));
- list_splice_init(&check_list, work_q);
- return list_empty(work_q);
- }
- scmd->eh_eflags &= ~SCSI_EH_CANCEL_CMD;
- if (rtn == FAST_IO_FAIL)
- scsi_eh_finish_cmd(scmd, done_q);
- else
- list_move_tail(&scmd->eh_entry, &check_list);
- }
-
- return scsi_eh_test_devices(&check_list, work_q, done_q, 0);
-}
-
/**
* scsi_eh_try_stu - Send START_UNIT to device.
* @scmd: &scsi_cmnd to send START_UNIT
@@ -1693,11 +1632,6 @@ static void scsi_eh_offline_sdevs(struct list_head *work_q,
sdev_printk(KERN_INFO, scmd->device, "Device offlined - "
"not ready after error recovery\n");
scsi_device_set_state(scmd->device, SDEV_OFFLINE);
- if (scmd->eh_eflags & SCSI_EH_CANCEL_CMD) {
- /*
- * FIXME: Handle lost cmds.
- */
- }
scsi_eh_finish_cmd(scmd, done_q);
}
return;
@@ -2139,8 +2073,7 @@ static void scsi_unjam_host(struct Scsi_Host *shost)
SCSI_LOG_ERROR_RECOVERY(1, scsi_eh_prt_fail_stats(shost, &eh_work_q));
if (!scsi_eh_get_sense(&eh_work_q, &eh_done_q))
- if (!scsi_eh_abort_cmds(&eh_work_q, &eh_done_q))
- scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
+ scsi_eh_ready_devs(shost, &eh_work_q, &eh_done_q);
spin_lock_irqsave(shost->host_lock, flags);
if (shost->eh_deadline != -1)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 26f7dcf..9cbe33b 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1647,7 +1647,7 @@ static void scsi_softirq_done(struct request *rq)
scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
break;
default:
- scsi_eh_scmd_add(cmd, 0);
+ scsi_eh_scmd_add(cmd);
break;
}
}
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index b2d1a2b..1248e72 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -18,7 +18,6 @@ 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) \
@@ -72,7 +71,7 @@ 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);
extern void scsi_eh_wakeup(struct Scsi_Host *shost);
-extern void scsi_eh_scmd_add(struct scsi_cmnd *, int);
+extern void scsi_eh_scmd_add(struct scsi_cmnd *);
void scsi_eh_ready_devs(struct Scsi_Host *shost,
struct list_head *work_q,
struct list_head *done_q);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5e36248..4f4db75 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -460,11 +460,6 @@ struct scsi_host_template {
unsigned no_write_same:1;
/*
- * True if asynchronous aborts are not supported
- */
- unsigned no_async_abort:1;
-
- /*
* Countdown for host blocking with no commands outstanding.
*/
unsigned int max_host_blocked;
--
1.8.5.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed
2014-11-04 12:11 ` [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed Hannes Reinecke
@ 2014-11-04 18:36 ` Christoph Hellwig
2014-11-05 7:44 ` Hannes Reinecke
0 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2014-11-04 18:36 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, Christoph Hellwig, Ewan Milne, Robert Elliott,
linux-scsi
On Tue, Nov 04, 2014 at 01:11:11PM +0100, Hannes Reinecke wrote:
> BLK_EH_HANDLED does not work with scsi commands, as we need
> to release the associated buffers correctly.
Which particular error do you see? The ->eh_timed_out routines can
return BLK_EH_HANDLED, and libata, iscsi and fc actually do, so we'll
need to fix these issues either way.
> And scsi_eh_scmd_add() currently only will fail if no
> error handler thread is started (which will never be the
> case)
Yes.
> or if the state machine encounters an illegal transition.
> As state machine transitions don't have any real meaning
> we can also force setting of the new state and
> make scsi_dh_scmd_add() a void function.
> With that we'll never have to resort to return
> BLK_EH_HANDLED.
Which kind of transition did you see rejected? Neither a CREATED or
DEL_(RECOVERY) state should be possible when invoking the error handler,
so I'd rather see asserts here than overriding the state machine.
te RECOVERY state from a RUNNING or existing RECOVERY state, which
leaves CREATED, CANCEL, DEL, CANCEL_RECOVERY or DEL_RECOVERY.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] scsi: make asynchronous aborts mandatory
2014-11-04 12:11 ` [PATCH 3/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
@ 2014-11-04 18:52 ` Christoph Hellwig
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2014-11-04 18:52 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Bottomley, Christoph Hellwig, Ewan Milne, Robert Elliott,
linux-scsi
This leaves a stale comment reference to scsi_eh_abort_cmds in libsas.
Also while grepping for that I found out that sas aborts don't work
anymore with the async abort code due to this beautiful code in
sas_eh_abort_handler:
if (current != host->ehandler)
return FAILED;
so you'll need another patch to fix that up. In the longer run we
really should both kill sas_eh_abort_handler and bring SAS EH
more in line with the rest of the world.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed
2014-11-04 18:36 ` Christoph Hellwig
@ 2014-11-05 7:44 ` Hannes Reinecke
0 siblings, 0 replies; 7+ messages in thread
From: Hannes Reinecke @ 2014-11-05 7:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: James Bottomley, Ewan Milne, Robert Elliott, linux-scsi
On 11/04/2014 07:36 PM, Christoph Hellwig wrote:
> On Tue, Nov 04, 2014 at 01:11:11PM +0100, Hannes Reinecke wrote:
>> BLK_EH_HANDLED does not work with scsi commands, as we need
>> to release the associated buffers correctly.
>
> Which particular error do you see? The ->eh_timed_out routines can
> return BLK_EH_HANDLED, and libata, iscsi and fc actually do, so we'll
> need to fix these issues either way.
>
Hmm. Looking closer it seem that you are correct.
However, the main point still stands; there is no way
scsi_eh_scmd_add() can legitimately fail.
>> And scsi_eh_scmd_add() currently only will fail if no
>> error handler thread is started (which will never be the
>> case)
>
> Yes.
>
>> or if the state machine encounters an illegal transition.
>> As state machine transitions don't have any real meaning
>> we can also force setting of the new state and
>> make scsi_dh_scmd_add() a void function.
>> With that we'll never have to resort to return
>> BLK_EH_HANDLED.
>
> Which kind of transition did you see rejected? Neither a CREATED or
> DEL_(RECOVERY) state should be possible when invoking the error handler,
> so I'd rather see asserts here than overriding the state machine.
> te RECOVERY state from a RUNNING or existing RECOVERY state, which
> leaves CREATED, CANCEL, DEL, CANCEL_RECOVERY or DEL_RECOVERY.
>
I did not see any rejection so far.
However, we're calling scsi_host_set_state(), which _might_ fail.
And we need to have a way of either
a) make sure that it cannot fail prior to calling it
or
b) handling the failure.
So I'm fine with using asserts here.
I'll be updating the patch (and description)
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (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] 7+ messages in thread
end of thread, other threads:[~2014-11-05 7:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-04 12:11 [RFC PATCH 0/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
2014-11-04 12:11 ` [PATCH 1/3] scsi: make scsi_eh_scmd_add() always succeed Hannes Reinecke
2014-11-04 18:36 ` Christoph Hellwig
2014-11-05 7:44 ` Hannes Reinecke
2014-11-04 12:11 ` [PATCH 2/3] scsi: make eh_eflags persistent Hannes Reinecke
2014-11-04 12:11 ` [PATCH 3/3] scsi: make asynchronous aborts mandatory Hannes Reinecke
2014-11-04 18:52 ` Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.