2.6.27 and good_bytes -= scsi_get_resid(cmd)

PATCH Miquel van Smoorenburg 2 participants 3 messages Nov 3, 2008 – Nov 4, 2008 SCSI

Share & tools

Miquel van Smoorenburg

author

2008-11-03 22:03 UTC #0 lore raw

I upgraded to 2.6.27 on a machine with the dpt_i2o scsi driver and I'm seeing some weird number from iostat -x 2. Basically it thinks that the size of every request is 32640 sectors (0xff0000 bytes), and that throws off the stats considerably.

By using git-bisect I found out that the cause is this commit:

commit 427e59f09fdba387547106de7bab980b7fff77be

Author: James Bottomley <[email protected]>
Date:   Sat Mar 8 18:24:17 2008 -0600

[SCSI] make use of the residue value

USB sometimes doesn't return an error but instead returns a residue value indicating part (or all) of the command wasn't completed. So if the driver _done() error processing indicates the command was fully processed, subtract off the residue so that this USB error gets propagated.

Cc Alan Stern <[email protected]>

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

drivers/scsi/scsi.c +9 -0

index 110e776..36c92f9 100644
@@ -855,9 +855,18 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 
 	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		int old_good_bytes = good_bytes;
 		drv = scsi_cmd_to_driver(cmd);
 		if (drv->done)
 			good_bytes = drv->done(cmd);
+		/*
+		 * USB may not give sense identifying bad sector and
+		 * simply return a residue instead, so subtract off the
+		 * residue if drv->done() error processing indicates no
+		 * change to the completion length.
+		 */
+		if (good_bytes == old_good_bytes)
+			good_bytes -= scsi_get_resid(cmd);
 	}
 	scsi_io_completion(cmd, good_bytes);
 }

Now the dpt_i2o driver calls scsi_set_resid unconditionally after a command completes and comments it as '// calculate resid for sg' .

Reverting the above commit fixes my problem. Applying the following patch to dpt_i2o.c fixes it as well. If the patch below is actually considered the right way to fix this (I'm not really sure what I'm doing here) then I think there might be more drivers that need a similar fix.

[PATCH] dpt_i2o.c: calculate resid for sg only

dpt_i2o.c calls scsi_set_resid() unconditionally. But it should only do that for sg requests. This became apparent after commit 427e59f09fdba387547106de7bab980b7fff77be .

Signed-off-by Miquel vam Smoorenburg <[email protected]>

drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.ORIG +2 -1

@@ -2445,7 +2445,8 @@
 	hba_status = detailed_status >> 8;
 
 	// calculate resid for sg 
-	scsi_set_resid(cmd, scsi_bufflen(cmd) - readl(reply+5));
+	if (cmd->request->cmd_type == REQ_TYPE_BLOCK_PC)
+		scsi_set_resid(cmd, scsi_bufflen(cmd) - readl(reply+5));
 
 	pHba = (adpt_hba*) cmd->device->host->hostdata[0];

Mike.

Boaz Harrosh

2008-11-04 08:32 UTC #1 lore raw

Quoted 32 lines · Miquel van Smoorenburg

I upgraded to 2.6.27 on a machine with the dpt_i2o scsi driver and I'm seeing some weird number from iostat -x 2. Basically it thinks that the size of every request is 32640 sectors (0xff0000 bytes), and that throws off the stats considerably.
By using git-bisect I found out that the cause is this commit:
commit 427e59f09fdba387547106de7bab980b7fff77be
Author: James Bottomley <[email protected]>
Date:   Sat Mar 8 18:24:17 2008 -0600
[SCSI] make use of the residue value
USB sometimes doesn't return an error but instead returns a residue value indicating part (or all) of the command wasn't completed. So if the driver _done() error processing indicates the command was fully processed, subtract off the residue so that this USB error gets propagated.
Cc Alan Stern <[email protected]>
Signed-off-by James Bottomley <[email protected]>
drivers/scsi/scsi.c +1 -0
index 110e776..36c92f9 100644
@@ -855,9 +855,18 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
 
 	good_bytes = scsi_bufflen(cmd);
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		int old_good_bytes = good_bytes;
 		drv = scsi_cmd_to_driver(cmd);

drivers/scsi/scsi.c void scsi_finish_command(struct scsi_cmnd *cmd) · line 869

Quoted 10 lines

drivers/scsi/scsi.c +8 -0

 		if (drv->done)
 			good_bytes = drv->done(cmd);
+		/*
+		 * USB may not give sense identifying bad sector and
+		 * simply return a residue instead, so subtract off the
+		 * residue if drv->done() error processing indicates no
+		 * change to the completion length.
+		 */
+		if (good_bytes == old_good_bytes)
+			good_bytes -= scsi_get_resid(cmd);

James hi. looking at this again it looks wrong to me. What are we doing here? we are asking the remainder to be requeued again. Is that the right thing to do? should we not raise an -EIO instead. As the comments above states it is usually do to a bad sector. I think the all decision should be moved into the drv->done function, only it knows what the command was and what's relevant.

Quoted 25 lines

drivers/scsi/scsi.c +0 -0
 	}
 	scsi_io_completion(cmd, good_bytes);
 }
Now the dpt_i2o driver calls scsi_set_resid unconditionally after a command completes and comments it as '// calculate resid for sg' .
Reverting the above commit fixes my problem. Applying the following patch to dpt_i2o.c fixes it as well. If the patch below is actually considered the right way to fix this (I'm not really sure what I'm doing here) then I think there might be more drivers that need a similar fix.
[PATCH] dpt_i2o.c: calculate resid for sg only
dpt_i2o.c calls scsi_set_resid() unconditionally. But it should only do that for sg requests. This became apparent after commit 427e59f09fdba387547106de7bab980b7fff77be .
Signed-off-by Miquel vam Smoorenburg <[email protected]>

drivers/scsi/dpt_i2o.c line 2450

Quoted 7 lines

drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.ORIG +2 -1

@@ -2445,7 +2445,8 @@
 	hba_status = detailed_status >> 8;
 
 	// calculate resid for sg 
-	scsi_set_resid(cmd, scsi_bufflen(cmd) - readl(reply+5));
+	if (cmd->request->cmd_type == REQ_TYPE_BLOCK_PC)
+		scsi_set_resid(cmd, scsi_bufflen(cmd) - readl(reply+5));

Why does the readl(reply+5) does not return the same number as scsi_bufflen(cmd). This is the real problem.
What you have done is just shove the problem away and never set residual for normal read/write commands.
Someone that knows the HW should check out, how to read from the device the real bytes transferred. Now if the real bytes transferred is different then what was requested in CDB, the HW should raise a CHECK_CONDITION status. If it does not then this is the same broken state as those USB stuff, and perhaps it should be looked into.

But I hope this is just bad programing on the driver's part, which interprets wrongly the return value from the register. Maybe the HW protocol mandates that the readl(reply+5) register. (nice descriptive name) should only be read if the HW returned CHECK_CONDITION, other wise it contains something else.

Quoted 6 lines

drivers/scsi/dpt_i2o.c renamed from drivers/scsi/dpt_i2o.c.ORIG +0 -0
 	pHba = (adpt_hba*) cmd->device->host->hostdata[0];
 
Mike.
--

Scsi-ml has changed, before it would disregard the residual if no CHECK_CONDITION was raised. Now it will look at it all the time. Which means drivers can not be sloppy about the resid value any more.

Boaz

Miquel van Smoorenburg

author

2008-11-04 23:05 UTC #2 lore raw

Quoted 5 lines · Boaz Harrosh

Quoted 4 lines · Miquel van Smoorenburg
I upgraded to 2.6.27 on a machine with the dpt_i2o scsi driver and I'm seeing some weird number from iostat -x 2. Basically it thinks that the size of every request is 32640 sectors (0xff0000 bytes), and that throws off the stats considerably.

[... dpt_i2o.c patch ...]

Quoted 2 lines

Why does the readl(reply+5) does not return the same number as scsi_bufflen(cmd). This is the real problem.

You're right, ofcourse. Thanks. I had a good look over the code and reply+5 is wrong. It should be reply + 5*sizeof(u32). In the same function all other values read from an offset which is a multiple of 4, which I should have noticed earlier I guess.

I'll send a new patch in a seperate mail.

Mike.