On 02/01/2010 01:21 PM, Mike Christie wrote:
On 02/01/2010 11:52 AM, Mike Christie wrote:
On 02/01/2010 05:14 AM, Erez Zilber wrote:
When iscsi_eh_cmd_timed_out gets called, we can ask scsi-ml to give us
more time if the cmd is making progress (i.e. if there was some data
transfer since the last timeout).
The problem is that task->last_xfer& task->last_timeout are set to
the value of 'jiffies' when allocating the task. If the target machine
is already dead when we send the cmd, no progress will be made. Still,
when iscsi_eh_cmd_timed_out will be called, it will think that data
was sent since the last timeout and reset the timer (and waste time).
In order to solve that, iscsi_eh_cmd_timed_out should also check if
there was any data transfer after the task was allocated.
I agree it is a problem with the code.
The problem is that the check also handled the case where we are so
backed up that we cannot even send a cmd/data within the cmd timeout.
For that case, the check was giving it a extra cmd timeout seconds to
get it off. That code is not really good though. It should probably just
loop over all the cmds there and see if any cmds have made progress. If
so give the cmd more time, if not then fail.
I was not sure though if I should check if any cmds to the target made
progress or if any cmds to the same disk. It could be that just one disk
went bad, so we might want to check per disk. However, this could be the
first IO to the disk and it just got stuck behind a bunch of other IO to
other disks, so in that case we want to check per target.
Give me until tomorrow. I think I can cook up patch. Before when
deciding when to check for dev vs target, I was mixed up with some
reordering stuff, but I think I have a patch that should work for both
of us.
I think the attached patch should do what we both want.
Instead of always waiting an extra cmd timeout seconds, we will only get
extra time if:
1. The command has made progress. (changed test to time_after)
2. Commands queued before the timed out one have made transfers since we
started the task or since it last timedout.
Patch was made over scsi-misc.
--
You received this message because you are subscribed to the Google Groups
"open-iscsi" group.
To post to this group, send email to open-is...@googlegroups.com.
To unsubscribe from this group, send email to
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/open-iscsi?hl=en.
>From 5eb20f1e53e3b2ed770fe9b410c26e45cdb2d55e Mon Sep 17 00:00:00 2001
From: Mike Christie <micha...@cs.wisc.edu>
Date: Mon, 1 Feb 2010 22:37:07 -0600
Subject: [PATCH 4/4] libiscsi: reset cmd timer if cmds are making progress
This patch resets the cmd timer if cmds started before
the timedout command are making progress. The idea is
that the cmd probably timed out because we are trying
to exeucte too many commands. If it turns out that the
device the IO timedout on was bad or the cmd just got
screwed up but other IO/devs were ok then we will
will figure this out when the cmds ahead of the timed
out one complete ok.
This also fixes a bug where we were sort of detecting
this by setting the last_timeout and last_xfer to the
same value when the task was allocated. That caught
the case where we never got to send any IO for it. However,
if the problem had started right before we started the
new task, then we were forced to wait an extra cmd
timeout seconds to start the scsi eh.
Signed-off-by: Mike Christie <micha...@cs.wisc.edu>
---
drivers/scsi/libiscsi.c | 53 +++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index c28a712..e556f52 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1919,10 +1919,11 @@ static int iscsi_has_ping_timed_out(struct iscsi_conn *conn)
static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
{
enum blk_eh_timer_return rc = BLK_EH_NOT_HANDLED;
- struct iscsi_task *task = NULL;
+ struct iscsi_task *task = NULL, *running_task;
struct iscsi_cls_session *cls_session;
struct iscsi_session *session;
struct iscsi_conn *conn;
+ int i;
cls_session = starget_to_session(scsi_target(sc->device));
session = cls_session->dd_data;
@@ -1947,8 +1948,15 @@ static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
}
task = (struct iscsi_task *)sc->SCp.ptr;
- if (!task)
+ if (!task) {
+ /*
+ * Raced with completion. Just reset timer, and let it
+ * complete normally
+ */
+ rc = BLK_EH_RESET_TIMER;
goto done;
+ }
+
/*
* If we have sent (at least queued to the network layer) a pdu or
* recvd one for the task since the last timeout ask for
@@ -1956,10 +1964,10 @@ static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
* we can check if it is the task or connection when we send the
* nop as a ping.
*/
- if (time_after_eq(task->last_xfer, task->last_timeout)) {
+ if (time_after(task->last_xfer, task->last_timeout)) {
ISCSI_DBG_EH(session, "Command making progress. Asking "
"scsi-ml for more time to complete. "
- "Last data recv at %lu. Last timeout was at "
+ "Last data xfer at %lu. Last timeout was at "
"%lu\n.", task->last_xfer, task->last_timeout);
task->have_checked_conn = false;
rc = BLK_EH_RESET_TIMER;
@@ -1977,6 +1985,43 @@ static enum blk_eh_timer_return iscsi_eh_cmd_timed_out(struct scsi_cmnd *sc)
goto done;
}
+ for (i = 0; i < conn->session->cmds_max; i++) {
+ running_task = conn->session->cmds[i];
+ if (!running_task->sc || running_task == task ||
+ running_task->state != ISCSI_TASK_RUNNING)
+ continue;
+
+ /*
+ * Only check if cmds started before this one have made
+ * progress, or this could never fail
+ */
+ if (time_after_eq(running_task->sc->jiffies_at_alloc,
+ task->sc->jiffies_at_alloc))
+ continue;
+
+ if (time_after(running_task->last_xfer, task->last_timeout)) {
+ /*
+ * This task has not made progress, but a task
+ * started before us has transferred data since
+ * we started/last-checked. We could be queueing
+ * too many tasks or the LU is bad.
+ *
+ * If the device is bad the cmds ahead of us on
+ * other devs will complete, and this loop will
+ * eventually fail starting the scsi eh.
+ */
+ ISCSI_DBG_EH(session, "Command has not made progress "
+ "but commands ahead of it have. "
+ "Asking scsi-ml for more time to "
+ "complete. Our last xfer vs running task "
+ "last xfer %lu/%lu. Last check %lu.\n",
+ task->last_xfer, running_task->last_xfer,
+ task->last_timeout);
+ rc = BLK_EH_RESET_TIMER;
+ goto done;
+ }
+ }
+
/* Assumes nop timeout is shorter than scsi cmd timeout */
if (task->have_checked_conn)
goto done;
--
1.6.5.2
