Tejun Heo wrote:
libata needs to invoke EH without scmd. This patch adds
shost->host_eh_scheduled to implement such behavior.
As this is a temporary hack for libata, no general interface is
defined. This patch simply adds handling for host_eh_scheduled where
needed and exports scsi_eh_wakeup() to modules. The rest is upto
libata.
Signed-off-by: Tejun Heo <htejun@xxxxxxxxx>
I ACK the change (the important part), but not the description or comments.
Rationale for ACK (if it matters): Back when the first version of this
was posted, after our initial discussion I reviewed the uses of
->host_failed and ->host_busy, and had considered twiddling those rather
than adding another state variable. Being in EH when host_failed==0
made me nervous. However, after reviewing the uses, I now feel that
your approach presented probably creates less complex code.
b3095135fd8d02d5b31591d87243afb3aeb217da
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 1c75646..442ae59 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -56,6 +56,7 @@ void scsi_eh_wakeup(struct Scsi_Host *sh
printk("Waking error handler thread\n"));
}
}
+EXPORT_SYMBOL_GPL(scsi_eh_wakeup); /* exported only for libata */
/**
* scsi_eh_scmd_add - add scsi cmd to error handling.
@@ -1517,7 +1518,7 @@ int scsi_error_handler(void *data)
*/
set_current_state(TASK_INTERRUPTIBLE);
while (!kthread_should_stop()) {
- if (shost->host_failed == 0 ||
+ if ((shost->host_failed == 0 && shost->host_eh_scheduled == 0) ||
shost->host_failed != shost->host_busy) {
SCSI_LOG_ERROR_RECOVERY(1,
printk("Error handler scsi_eh_%d sleeping\n",
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7b0f9a3..c55d195 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -566,7 +566,7 @@ void scsi_device_unbusy(struct scsi_devi
spin_lock_irqsave(shost->host_lock, flags);
shost->host_busy--;
if (unlikely(scsi_host_in_recovery(shost) &&
- shost->host_failed))
+ (shost->host_failed || shost->host_eh_scheduled)))
scsi_eh_wakeup(shost);
spin_unlock(shost->host_lock);
spin_lock(sdev->request_queue->queue_lock);
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index d160880..e31ada2 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -35,6 +35,7 @@ static inline int scsi_sense_valid(struc
}
+extern void scsi_eh_wakeup(struct Scsi_Host *shost); /* libata only */
extern void scsi_eh_finish_cmd(struct scsi_cmnd *scmd,
struct list_head *done_q);
extern void scsi_eh_flush_done_q(struct list_head *done_q);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index de6ce54..3b454c2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -472,6 +472,7 @@ struct Scsi_Host {
*/
unsigned int host_busy; /* commands actually active on low-level */
unsigned int host_failed; /* commands that failed. */
+ unsigned int host_eh_scheduled; /* XXX - libata hack, do NOT use */
However, the nitpicks I have with the comments and patch description:
* Although your comments about "libata only" are true, it is more
correct to say that these are exclusive to ->eh_strategy_handler()
users. It just so happens that libata is the only in-tree user...
however when making changes to the SCSI driver API, you should be in a
"I'm updating the SCSI EH API" frame of mine as well.
* In your patch description you mention this change (or a subset
thereof) is temporary, but do not explain why it is temporary, and what
will be the eventual replacement.
* In general this change should be described as an addition to the SCSI
LLDD EH API... I even disagree to some extent of calling it a hack.
One must Do What Needs To Be Done, And No More. There is no shame in
that :)
The ->eh_strategy_handler API is guaranteed to be rough, because there
weren't really any in-tree users until libata came along. Now we're
exposing the rough edges, and fixing them.
Jeff
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
