scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

PATCH In review Hannes Reinecke 4 participants 5 messages Apr 25, 2013 – May 4, 2013 SCSI

Share & tools

Hannes Reinecke

author

2013-04-25 06:10 UTC #0 lore raw

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

scsi_send_eh_cmnd() is calling queuecommand() directly, so it needs to check the return value here.
The only valid return codes for queuecommand() are 'busy' states, so we need to wait for a bit to allow the LLDD to recover.

Based on an earlier patch from Wen Xiong.

Cc Wen Xiong <[email protected]>

Cc Brian King <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

drivers/scsi/scsi_error.c +16 -6

index d58db32..6a3c1d2 100644
@@ -889,22 +889,32 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	struct scsi_device *sdev = scmd->device;
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
-	unsigned long timeleft;
+	unsigned long timeleft = timeout;
 	struct scsi_eh_save ses;
+	const int stall_for = min(HZ/10, 1);
 	int rtn;
 
+retry:
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
 
 	scsi_log_send(scmd);
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
-
-	timeleft = wait_for_completion_timeout(&done, timeout);
+	rtn = shost->hostt->queuecommand(shost, scmd);
+	if (rtn) {
+		if (timeleft) {
+			scsi_eh_restore_cmnd(scmd, &ses);
+			timeleft -= stall_for;
+			msleep(stall_for);
+			goto retry;
+		}
+		rtn = NEEDS_RETRY;
+	} else
+		timeleft = wait_for_completion_timeout(&done, timeout);
 
 	shost->eh_action = NULL;
 
-	scsi_log_completion(scmd, SUCCESS);
+	scsi_log_completion(scmd, rtn);
 
 	SCSI_LOG_ERROR_RECOVERY(3,
 		printk("%s: scmd: %p, timeleft: %ld\n",

@@ -935,7 +945,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			rtn = FAILED;
 			break;
 		}
-	} else {
+	} else if (!rtn) {
 		scsi_abort_eh_cmnd(scmd);
 		rtn = FAILED;
 	}

[email protected]

2013-05-03 14:24 UTC #1 lore raw

Quoting Hannes Reinecke <[email protected]>:

Quoted 6 lines

scsi_send_eh_cmnd() is calling queuecommand() directly, so it needs to check the return value here.
The only valid return codes for queuecommand() are 'busy' states, so we need to wait for a bit to allow the LLDD to recover.

Quoted 6 lines

Based on an earlier patch from Wen Xiong.
Cc Wen Xiong <[email protected]>
Cc Brian King <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>

Hi James,

I have verified this patch with two new ipr adapters. EEH error can be recoery successfully. Do you have any question about this new patch?

Thanks,
Wendy

Quoted 53 lines

drivers/scsi/scsi_error.c +15 -5

index d58db32..6a3c1d2 100644
@@ -889,22 +889,32 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd  
 *scmd, unsigned char *cmnd,
 	struct scsi_device *sdev = scmd->device;
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
-	unsigned long timeleft;
+	unsigned long timeleft = timeout;
 	struct scsi_eh_save ses;
+	const int stall_for = min(HZ/10, 1);
 	int rtn;
 
+retry:
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
 
 	scsi_log_send(scmd);
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
-
-	timeleft = wait_for_completion_timeout(&done, timeout);
+	rtn = shost->hostt->queuecommand(shost, scmd);
+	if (rtn) {
+		if (timeleft) {
+			scsi_eh_restore_cmnd(scmd, &ses);
+			timeleft -= stall_for;
+			msleep(stall_for);
+			goto retry;
+		}
+		rtn = NEEDS_RETRY;
+	} else
+		timeleft = wait_for_completion_timeout(&done, timeout);
 
 	shost->eh_action = NULL;
 
-	scsi_log_completion(scmd, SUCCESS);
+	scsi_log_completion(scmd, rtn);
 
 	SCSI_LOG_ERROR_RECOVERY(3,

drivers/scsi/scsi_error.c +1 -1

 		printk("%s: scmd: %p, timeleft: %ld\n",

@@ -935,7 +945,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd  
 *scmd, unsigned char *cmnd,
 			rtn = FAILED;
 			break;
 		}
-	} else {
+	} else if (!rtn) {
 		scsi_abort_eh_cmnd(scmd);
 		rtn = FAILED;

}

James Bottomley

2013-05-03 18:23 UTC #2 lore raw

Quoted 14 lines · On Fri, 2013-05-03 at 10:24 -0400, [email protected] wrote:

Quoting Hannes Reinecke <[email protected]>:
Quoted 6 lines
scsi_send_eh_cmnd() is calling queuecommand() directly, so it needs to check the return value here.
The only valid return codes for queuecommand() are 'busy' states, so we need to wait for a bit to allow the LLDD to recover.
Quoted 6 lines
Based on an earlier patch from Wen Xiong.
Cc Wen Xiong <[email protected]>
Cc Brian King <[email protected]>
Signed-off-by Hannes Reinecke <[email protected]>

Quoted 5 lines

Hi James,
I have verified this patch with two new ipr adapters. EEH error can be recoery successfully.
Do you have any question about this new patch?

Yes, it's not correct: stall_for is in jiffies not msec, so msleep(stall_for) is taking far too long. Plus you don't know that stall_for is a divisor of timeleft, so timeleft -= stall_for could wrap and cause the retry loop to go on effectively forever.

I think the below is the correct patch, so I'll commit it unless there are objections.

James

---

Quoted 1 line

From a74498dda7acf6f5fb99e43df54f2c1f5c6beec9 Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <[email protected]>
Date: Thu, 25 Apr 2013 08:10:00 +0200
Subject: [PATCH] [SCSI] Handle MLQUEUE busy response in scsi_send_eh_cmnd

Based on an earlier patch from Wen Xiong.

[jejb: fix confusion between msec and jiffies values and other issues]

Cc Wen Xiong <[email protected]>

Cc Brian King <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

Signed-off-by James Bottomley <[email protected]>

drivers/scsi/scsi_error.c +18 -6

index c1b05a8..cfd1ef2 100644
@@ -25,6 +25,7 @@
 #include <linux/interrupt.h>
 #include <linux/blkdev.h>
 #include <linux/delay.h>
+#include <linux/jiffies.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>

@@ -791,22 +792,33 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	struct scsi_device *sdev = scmd->device;
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
-	unsigned long timeleft;
+	unsigned long timeleft = timeout;
 	struct scsi_eh_save ses;
+	const unsigned long stall_for = min(msecs_to_jiffies(10), 1UL);
 	int rtn;
 
+retry:
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
 
 	scsi_log_send(scmd);
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
-
-	timeleft = wait_for_completion_timeout(&done, timeout);
+	rtn = shost->hostt->queuecommand(shost, scmd);
+	if (rtn) {
+		if (timeleft > stall_for) {
+			scsi_eh_restore_cmnd(scmd, &ses);
+			timeleft -= stall_for;
+			msleep(jiffies_to_msecs(stall_for));
+			goto retry;
+		}
+		timeleft = 0;
+		rtn = NEEDS_RETRY;
+	} else
+		timeleft = wait_for_completion_timeout(&done, timeout);
 
 	shost->eh_action = NULL;
 
-	scsi_log_completion(scmd, SUCCESS);
+	scsi_log_completion(scmd, rtn);
 
 	SCSI_LOG_ERROR_RECOVERY(3,
 		printk("%s: scmd: %p, timeleft: %ld\n",

@@ -837,7 +849,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			rtn = FAILED;
 			break;
 		}
-	} else {
+	} else if (!rtn) {
 		scsi_abort_eh_cmnd(scmd);
 		rtn = FAILED;
 	}

Bart Van Assche

2013-05-04 17:02 UTC #3 lore raw

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 797

Quoted 1 line · James Bottomley

drivers/scsi/scsi_error.c +1 -0

+	const unsigned long stall_for = min(msecs_to_jiffies(10), 1UL);

Hello James,

Can you please clarify what the intention of this statement is ? Is the purpose of this statement to avoid that stall_for would be zero in case HZ < 100 ? If that is the case, maybe you meant max() instead of min() ? Also, are you aware that msecs_to_jiffies() already rounds up the result of the division ?

Bart.

James Bottomley

2013-05-04 18:20 UTC #4 lore raw

Quoted 10 lines · Bart Van Assche

Review
Quoted 1 line · James Bottomley
(quoted diff) +1 -0
+	const unsigned long stall_for = min(msecs_to_jiffies(10), 1UL);
Hello James,
Can you please clarify what the intention of this statement is ? Is the purpose of this statement to avoid that stall_for would be zero in case HZ < 100 ? If that is the case, maybe you meant max() instead of min() ? Also, are you aware that msecs_to_jiffies() already rounds up the result of the division ?

Yes, I thought afterwards I should dump the bogus min statement as well. Plus HZ/10 is actually 100ms, so the value is 10x wrong. I've fixed it up below (plus a bit of comment rework and some style fixes).

Thanks,

James

---

Quoted 1 line

From 4bd9ef9789ad86656d8e52e8fff5422b741097e1 Mon Sep 17 00:00:00 2001

From: Hannes Reinecke <[email protected]>
Date: Thu, 25 Apr 2013 08:10:00 +0200
Subject: [PATCH] [SCSI] Handle MLQUEUE busy response in scsi_send_eh_cmnd

Based on an earlier patch from Wen Xiong.

[jejb: fix confusion between msec and jiffies values and other issues]

[bvanassche: correct stall_for interval]

Cc Wen Xiong <[email protected]>

Cc Brian King <[email protected]>

Signed-off-by Hannes Reinecke <[email protected]>

Signed-off-by James Bottomley <[email protected]>

drivers/scsi/scsi_error.c +27 ~~-10~~

index c1b05a8..f43de1e 100644
@@ -25,6 +25,7 @@
 #include <linux/interrupt.h>
 #include <linux/blkdev.h>
 #include <linux/delay.h>
+#include <linux/jiffies.h>
 
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>

@@ -791,32 +792,48 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	struct scsi_device *sdev = scmd->device;
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
-	unsigned long timeleft;
+	unsigned long timeleft = timeout;
 	struct scsi_eh_save ses;
+	const unsigned long stall_for = msecs_to_jiffies(100);
 	int rtn;
 
+retry:
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
 
 	scsi_log_send(scmd);
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
-
-	timeleft = wait_for_completion_timeout(&done, timeout);
+	rtn = shost->hostt->queuecommand(shost, scmd);
+	if (rtn) {
+		if (timeleft > stall_for) {
+			scsi_eh_restore_cmnd(scmd, &ses);
+			timeleft -= stall_for;
+			msleep(jiffies_to_msecs(stall_for));
+			goto retry;
+		}
+		/* signal not to enter either branch of the if () below */
+		timeleft = 0;
+		rtn = NEEDS_RETRY;
+	} else {
+		timeleft = wait_for_completion_timeout(&done, timeout);
+	}
 
 	shost->eh_action = NULL;
 
-	scsi_log_completion(scmd, SUCCESS);
+	scsi_log_completion(scmd, rtn);
 
 	SCSI_LOG_ERROR_RECOVERY(3,
 		printk("%s: scmd: %p, timeleft: %ld\n",
 			__func__, scmd, timeleft));
 
 	/*
-	 * If there is time left scsi_eh_done got called, and we will
-	 * examine the actual status codes to see whether the command
-	 * actually did complete normally, else tell the host to forget
-	 * about this command.
+	 * If there is time left scsi_eh_done got called, and we will examine
+	 * the actual status codes to see whether the command actually did
+	 * complete normally, else if we have a zero return and no time left,
+	 * the command must still be pending, so abort it and return FAILED.
+	 * If we never actually managed to issue the command, because
+	 * ->queuecommand() kept returning non zero, use the rtn = FAILED
+	 * value above (so don't execute either branch of the if)
 	 */
 	if (timeleft) {
 		rtn = scsi_eh_completed_normally(scmd);

@@ -837,7 +854,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			rtn = FAILED;
 			break;
 		}
-	} else {
+	} else if (!rtn) {
 		scsi_abort_eh_cmnd(scmd);
 		rtn = FAILED;
 	}