Skip to contentSkip to conversation

scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

PATCH In review [email protected] 2 participants 3 messages Apr 16, 2013 – Apr 23, 2013 SCSI

lore
Share & tools
lore

[email protected]

author
#0 lore raw

v2 cover scsi: Handle MLQUEUE busy response in scsi_send_eh_cmnd

After we discussed your comments, We intergated your patch and generated this updated patch.

Please help us review the new patch. If it is ok, you are free to add your name in sign-off list.

Thanks for your help!
Wendy

author
#1 lore raw

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

We discussed James's concern. We intergated James's patch and generated this updated patch.

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]>
Signed-off-by Brian King <[email protected]>
---
 drivers/scsi/scsi_error.c |   34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)
drivers/scsi/scsi_error.c +25 -9
Index: b/drivers/scsi/scsi_error.c
===================================================================
@@ -791,18 +791,21 @@ static int scsi_send_eh_cmnd(struct scsi
 	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;
 
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
+retry:
 	shost->eh_action = &done;
 
 	scsi_log_send(scmd);
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
+	rtn = shost->hostt->queuecommand(shost, scmd);
 
-	timeleft = wait_for_completion_timeout(&done, timeout);
+	if (!rtn)
+		timeleft = wait_for_completion_timeout(&done, timeout);
 
 	shost->eh_action = NULL;
 

@@ -819,10 +822,19 @@ static int scsi_send_eh_cmnd(struct scsi
 	 * about this command.
 	 */
 	if (timeleft) {
-		rtn = scsi_eh_completed_normally(scmd);
-		SCSI_LOG_ERROR_RECOVERY(3,
-			printk("%s: scsi_eh_completed_normally %x\n",
-			       __func__, rtn));
+		switch (rtn) {
+		case 0:
+			rtn = scsi_eh_completed_normally(scmd);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				printk("%s: scsi_eh_completed_normally %x\n",
+				       __func__, rtn));
+			break;
+		case FAILED:
+			break;
+		default:
+			rtn = ADD_TO_MLQUEUE;
+			break;
+		}
 
 		switch (rtn) {
 		case SUCCESS:

@@ -831,8 +843,12 @@ static int scsi_send_eh_cmnd(struct scsi
 		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;
#2 lore raw
Quoted 35 lines · On 04/16/2013 10:26 PM, [email protected] wrote:

We discussed James's concern. We intergated James's patch and generated this updated patch.

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]>
Signed-off-by Brian King <[email protected]>
---
 drivers/scsi/scsi_error.c |   34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

Index: b/drivers/scsi/scsi_error.c
===================================================================

drivers/scsi/scsi_error.c +3 -1
@@ -791,18 +791,21 @@ static int scsi_send_eh_cmnd(struct scsi
 	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;
 
 	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
+retry:
 	shost->eh_action = &done;
 
 	scsi_log_send(scmd);

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi · line 811

Quoted 10 lines
drivers/scsi/scsi_error.c +3 -2
 	scmd->scsi_done = scsi_eh_done;
-	shost->hostt->queuecommand(shost, scmd);
+	rtn = shost->hostt->queuecommand(shost, scmd);
 
-	timeleft = wait_for_completion_timeout(&done, timeout);
+	if (!rtn)
+		timeleft = wait_for_completion_timeout(&done, timeout);
 
 	shost->eh_action = NULL;

Hmm. This seems to be a generic bug fix here; it is perfectly ok for queuecommand() to return a non-zero value without calling ->scsi_done. At which point ->complete is pointless to try as no completion ever would be invoked.

Mind separating that out as a separate patch?

Quoted 14 lines
drivers/scsi/scsi_error.c +6 -4
@@ -819,10 +822,19 @@ static int scsi_send_eh_cmnd(struct scsi
 	 * about this command.
 	 */
 	if (timeleft) {
-		rtn = scsi_eh_completed_normally(scmd);
-		SCSI_LOG_ERROR_RECOVERY(3,
-			printk("%s: scsi_eh_completed_normally %x\n",
-			       __func__, rtn));
+		switch (rtn) {
+		case 0:
+			rtn = scsi_eh_completed_normally(scmd);
+			SCSI_LOG_ERROR_RECOVERY(3,
+				printk("%s: scsi_eh_completed_normally %x\n",
+				       __func__, rtn));

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi · line 840

Quoted 10 lines
drivers/scsi/scsi_error.c +7 -0
+			break;
+		case FAILED:
+			break;
+		default:
+			rtn = ADD_TO_MLQUEUE;
+			break;
+		}
 
 		switch (rtn) {
 		case SUCCESS:

Bzzt.
'FAILED' is a valid response for scsi_eh_completed_normally, but not for ->queuecommand.

Quoted 6 lines
drivers/scsi/scsi_error.c +0 -1
@@ -831,8 +843,12 @@ static int scsi_send_eh_cmnd(struct scsi
 		case TARGET_ERROR:
 			break;
 		case ADD_TO_MLQUEUE:
-			rtn = NEEDS_RETRY;

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi · line 854

Quoted 10 lines
drivers/scsi/scsi_error.c +6 -1
-			break;
+			if (timeleft > stall_for) {
+				timeout = timeleft - stall_for;
+				msleep(stall_for);
+				goto retry;
+			}
+			/* fall through */
 		default:
 			rtn = FAILED;
 			break;

We're already calling 'wait_for_completion_timeout'. So normally the 'msleep' wouldn't be necessary. It's only required for the case when ->queuecommand returns non-zero.
So I'd rather see to have the msleep into section where the return command from queuecommand is evaluated; here we'll actually decrease responsiveness as we're always waiting for X seconds, even if the command would've been completed during that time.

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html