scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

PATCH Superseded [email protected] 3 participants 7 messages Apr 15, 2013 – Apr 16, 2013 SCSI

Share & tools

[email protected]

author

2013-04-15 18:44 UTC #0 lore raw

cover scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd when sending commands and retry for a bit if the driver returns a busy response.

Thanks,
Wendy

[email protected]

author

2013-04-15 18:44 UTC #1 lore raw

PATCH 1/1 scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

Fix scsi_send_eh_cmnd to check the return code of queuecommand when sending commands and retry for a bit if the LLDD returns a busy response. This fixes an issue seen with the ipr driver where an ipr initiated reset immediately following an eh_host_reset caused EH initiated commands to fail, resulting in devices being taken offline. This patch resolves the issue.

Signed-off-by Wen Xiong <[email protected]>

---
 drivers/scsi/scsi_error.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/scsi/scsi_error.c +9 -1

Index: b/drivers/scsi/scsi_error.c
===================================================================
@@ -793,6 +793,7 @@ static int scsi_send_eh_cmnd(struct scsi
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
 	struct scsi_eh_save ses;
+	int attempts = 30;
 	int rtn;
 
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);

@@ -800,7 +801,14 @@ static int scsi_send_eh_cmnd(struct scsi
 
 	scsi_log_send(scmd);
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
+
+	while ((rtn = shost->hostt->queuecommand(shost, scmd)) && attempts) {
+		if (rtn == SCSI_MLQUEUE_DEVICE_BUSY ||
+		    rtn == SCSI_MLQUEUE_TARGET_BUSY ||
+		    rtn == SCSI_MLQUEUE_HOST_BUSY)
+			attempts--;
+		ssleep(1);
+	}
 
 	timeleft = wait_for_completion_timeout(&done, timeout);

James Bottomley

maintainer

2013-04-15 20:45 UTC #2 lore raw

In reply to [email protected]

Quoted 3 lines · On Mon, 2013-04-15 at 13:39 -0500, [email protected] wrote:

In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd when sending commands and retry for a bit if the driver returns a busy response.

This is already handled by the timeout, I think. If a driver continuously returns MLQUEUE BUSY, then we'll fail the request after the timeout on the command expires.

James

Brian King

2013-04-15 21:56 UTC #3 lore raw

Quoted 8 lines · James Bottomley

Quoted 3 lines · On Mon, 2013-04-15 at 13:39 -0500, [email protected] wrote:
In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd when sending commands and retry for a bit if the driver returns a busy response.
This is already handled by the timeout, I think. If a driver continuously returns MLQUEUE BUSY, then we'll fail the request after the timeout on the command expires.

If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the abort works, we return FAILED out of scsi_send_eh_cmnd, which results in no retry being performed, since scsi_eh_tur only retries once and only if NEEDS_RETRY is returned. Or am I missing something?

Thanks,

Brian

Signature · 3 lines

Brian King
Power Linux I/O
IBM Linux Technology Center

James Bottomley

maintainer

2013-04-15 22:33 UTC #4 lore raw

Quoted 6 lines · Brian King

Quoted 5 lines · James Bottomley
Quoted 3 lines · On Mon, 2013-04-15 at 13:39 -0500, [email protected] wrote:
In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd when sending commands and retry for a bit if the driver returns a busy response.

Quoted 8 lines

Quoted 3 lines
This is already handled by the timeout, I think. If a driver continuously returns MLQUEUE BUSY, then we'll fail the request after the timeout on the command expires.
If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the abort works, we return FAILED out of scsi_send_eh_cmnd, which results in no retry being performed, since scsi_eh_tur only retries once and only if NEEDS_RETRY is returned. Or am I missing something?

Sorry, I'm not being clear. It comes with being at a conference. What I mean is that if you do this, the criterion for success or failure should be the amount of time left not the number of retries. This is what the non-eh submission path also does for retries of events that don't count against the retry limit ... so something like this patch (uncompiled and untested #include stddisclaimer.h)

James

----

drivers/scsi/scsi_error.c +9 -2

index c1b05a8..93ab4f4 100644
@@ -793,6 +793,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
 	struct scsi_eh_save ses;
+	const int stall_for = min(HZ/10,1); /* 100 ms */
 	int rtn;
 
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);

@@ -802,6 +803,8 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	scmd->scsi_done = scsi_eh_done;
 	shost->hostt->queuecommand(shost, scmd);
 
+  retry:
+
 	timeleft = wait_for_completion_timeout(&done, timeout);
 
 	shost->eh_action = NULL;

@@ -831,8 +834,12 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 		case TARGET_ERROR:
 			break;
 		case ADD_TO_MLQUEUE:
-			rtn = NEEDS_RETRY;
-			break;
+			if (timeleft > stall_for) {
+				timeout = timeleft - stall_for;
+				msleep(stall_for);
+				goto retry;
+			}
+			/* fall through */
 		default:
 			rtn = FAILED;
 			break;

[email protected]

author Applied

2013-04-16 00:09 UTC #5 lore raw

Quoting James Bottomley <[email protected]>:

Quoted 17 lines · Brian King

Quoted 7 lines · James Bottomley
Quoted 2 lines · On Mon, 2013-04-15 at 13:39 -0500, [email protected] wrote:
Quoted 1 line
In scsi_send_eh_cmnd(), this fix will check the return code of
queuecomamnd
Quoted 3 lines
Quoted 2 lines
when sending commands and retry for a bit if the driver returns a busy response.
Quoted 8 lines
Quoted 3 lines
This is already handled by the timeout, I think. If a driver continuously returns MLQUEUE BUSY, then we'll fail the request after the timeout on the command expires.
If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the abort works, we return FAILED out of scsi_send_eh_cmnd, which results in no retry being performed, since scsi_eh_tur only retries once and only if NEEDS_RETRY is returned. Or am I missing something?

Quoted 8 lines

Sorry, I'm not being clear. It comes with being at a conference. What I mean is that if you do this, the criterion for success or faiure should be the amount of time left not the number of retries. This is what the non-eh submission path also does for retries of events that don't count against the retry limit ... so something like this patch (uncompiled and untested #include stddisclaimer.h)
James

Hi James,

The failing case for us is: Doesn't matter what timeout value we set in wait_for_completion_timeout(), it always returns with timeleft = 0. For example, if I set timeout = 50 secs, wait_for_completion_timeout() always returns with timeleft =0(even adapter is already in good shape in 20 secs). We never gets a chance to call into if (timeleft) leg.

My understanding is: if shost->host->queuecommand() failed with MLQUEUE busy response at the first time, wait_for_completion_timeout() always wakes up by expired.

Here is log when I enabled scsi log:

Apr 15 18:44:35 ltcsatiocp5 kernel: scsi_send_eh_cmnd: scmd: c0000000f88bc980, timeleft: 0

I applied your patch. Because timeleft is always zero, never got a chance to call into if(timeleft) { leg.

Thanks,
Wendy

Brian King

2013-04-16 15:13 UTC #6 lore raw

In reply to James Bottomley

Quoted 16 lines · James Bottomley

Quoted 6 lines · Brian King
Quoted 5 lines · James Bottomley
Quoted 3 lines · On Mon, 2013-04-15 at 13:39 -0500, [email protected] wrote:
In scsi_send_eh_cmnd(), this fix will check the return code of queuecomamnd when sending commands and retry for a bit if the driver returns a busy response.
Quoted 8 lines
Quoted 3 lines
This is already handled by the timeout, I think. If a driver continuously returns MLQUEUE BUSY, then we'll fail the request after the timeout on the command expires.
If we get a timeout in scsi_send_eh_cmnd we call scsi_abort_eh_cmnd. If the abort works, we return FAILED out of scsi_send_eh_cmnd, which results in no retry being performed, since scsi_eh_tur only retries once and only if NEEDS_RETRY is returned. Or am I missing something?

Quoted 6 lines

Sorry, I'm not being clear. It comes with being at a conference. What I mean is that if you do this, the criterion for success or failure should be the amount of time left not the number of retries. This is what the non-eh submission path also does for retries of events that don't count against the retry limit ... so something like this patch (uncompiled and untested #include stddisclaimer.h)

Jams,

Wendy and I discussed this a bit more and I think we understand your concern. Wendy is working on an updated patch.

Thanks,

Brian

Signature · 3 lines

Brian King
Power Linux I/O
IBM Linux Technology Center