2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24

Note: description below applies to 2.6.24 as that's what was used to isolate the problem being described. There are comments at the end with regard to 2.6.25-rc4-git3. The below email thread directly applies to this bug/patch.

http://marc.info/?l=linux-scsi&m=120222578814366&w=2

--

While performing raid reset testing using 2.6.24, every once in the while I noticed that one of the raid's management tools would not exit. I tracked this down to it waiting on an inquiry command's completion.

An infinite loop occurs because there is an occasional inquiry command submitted to the host driver with the associated bio having a bi_size field of 512 when the inquiry command's transfer length is 255. This "enlarged" bi_size field causes the completion handler to re-issue the inquiry command because there are "leftovers" following the completion.

scsi_finish_command() calls scsi_io_completion() with good_bytes equal to cmd->request_bufflen.

scsi_io_completion() sets req->data_len to the residual provided by the LLDD.

scsi_io_completion() calls scsi_end_request() with the good_bytes value of 255.

scsi_end_request() calls end_that_request_chunk() which calls __end_that_request_first() which uses bio->bi_size as the size of the request. As bi_size is larger than the number of bytes (nr_bytes) being completed, it returns "1" indicating that there wasn't enough data to finish the request.

scsi_end_request() then calls scsi_requeue_command() to resubmit and fetch that data which wasn't received. This results in the command being reissued using the residual value returned by the driver as the request_bufflen for the next command.

This repeats itself until the residual is zero and bi_size is non-zero at which point the command gets reissued with a request_bufflen of zero and sc_data_direction of DMA_NONE. The subsequent completions are retried infinitely because bi_size is still non-zero.

What drove me crazy is that this didn't happen with every inquiry command issued by the application. It was very infrequent. I finally tracked it down to the sg driver.

sg_ioctl() calls sg_new_write() which calls sg_common_write() which calls sg_start_req().

sg_start_req() either calls sg_link_reserve() or sg_build_indirect() to set up the data transfer. The call to sg_build_indirect() ROUNDS UP THE REQUEST SIZE to a sector boundary, and this is what causes the infinite loop.

I tried removing the round up statement in sg_build_indirect() and forcing sg_start_req() to always call sg_build_indirect() to test the change and the infinite loop did not occur.

To me, it seems that if sg_link_reserve() has no need to round up to a sector boundary then sg_build_indirect() should also not need to round up the request length.

I've included a patch (against 2.6.25-rc4-git3) which removes the rounding in sg_build_indirect() for your comments.

I then tried my test case with 2.6.25-rc4-git3 and it didn't happen. I noticed the change in the routine scsi_req_map_sg() which deletes a line which incremented the "data_len" variable at the start of the for_each_sg() loop. This was also a line which I examined in 6.5.24. But, I noticed comments about sg indicating that it "sends a scatterlist that is larger than the data_len it wants transferred for certain IO sizes", and as that appeared to be what was happening, I went in search of what was up with sg.

So, will the below patch break anything?

Signed-off-by Michael Reed <[email protected]>

All these patches do not solve the root cause of the problem and until solved, (easily) they will keep popping up.

The root cause is that BLOCK_PC commands *can not* be retried in chunks. The midlayer has no way to know how to retry them. and any attempt to do so will result in an illegal scsi command.

I'm really sorry for not seeing this before. I have seen all these emails and patches back and forth but they addressed such remote locations, that I have not paid attention to them.

So lets try to touch base. BLOCK_PC command can only be completed in full always. In case of error or residual this should be properly reported and that is all the midlayer can do. The Initiator That knows what command was sent will then decide on a retry or not. In above case that will be user mode.

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

---
git-diff --stat -p

 drivers/scsi/scsi_lib.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

drivers/scsi/scsi_lib.c +16 -14

index ba21d97..e5d05c6 100644
@@ -784,17 +784,24 @@ EXPORT_SYMBOL(scsi_release_buffers);
  * in req->data_len and req->next_rq->data_len. The upper-layer driver can
  * decide what to do with this information.
  */
-void scsi_end_bidi_request(struct scsi_cmnd *cmd)
+static void scsi_end_blk_request(struct scsi_cmnd *cmd)
 {
 	struct request *req = cmd->request;
 	unsigned int dlen = req->data_len;
-	unsigned int next_dlen = req->next_rq->data_len;
+	unsigned int next_dlen;
 
-	req->data_len = scsi_out(cmd)->resid;
-	req->next_rq->data_len = scsi_in(cmd)->resid;
+	if (blk_bidi_rq(req)) {
+		next_dlen = req->next_rq->data_len;
+		req->data_len = scsi_out(cmd)->resid;
+		req->next_rq->data_len = scsi_in(cmd)->resid;
+	} else {
+		next_dlen = 0;
+		req->data_len = scsi_get_resid(cmd);
+	}
 
-	/* The req and req->next_rq have not been completed */
-	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));
+	if (blk_end_bidi_request(req, 0, dlen, next_dlen))
+		/* The req and req->next_rq have not been completed */
+		BUG_ON();
 
 	scsi_release_buffers(cmd);
 

@@ -866,15 +873,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		if (scsi_bidi_cmnd(cmd)) {
-			/* will also release_buffers */
-			scsi_end_bidi_request(cmd);
-			return;
-		}
-		req->data_len = scsi_get_resid(cmd);
+		/* will also release_buffers */
+		scsi_end_blk_request(cmd);
+		return;
 	}
 
-	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
 	scsi_release_buffers(cmd);
 
 	/*
 
 
 
 
 
 
 
--

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

Boaz Harrosh wrote:

<snip>

Quoted 2 lines

All these patches do not solve the root cause of the problem and until solved, (easily) they will keep popping up.

I will ask that the correctness of the sg driver's construction of a scatter list also be considered here. Is it ever correct for bi_size to wind up larger than the request size? Why is rounding needed in one case (sg_build_indirect) but not the other (sg_link_reserve)?

Quoted 8 lines

The root cause is that BLOCK_PC commands *can not* be retried in chunks. The midlayer has no way to know how to retry them. and any attempt to do so will result in an illegal scsi command.

I'm really sorry for not seeing this before. I have seen all these emails and patches back and forth but they addressed such remote locations, that I have not paid attention to them.

Quoted 5 lines

So lets try to touch base. BLOCK_PC command can only be completed in full always. In case of error or residual this should be properly reported and that is all the midlayer can do. The Initiator That knows what command was sent will then decide on a retry or not. In above case that will be user mode.

Those who truly understand this should comment. It makes sense to this novice.

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Quoted 6 lines

Quoted 3 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

---
From: Boaz Harrosh <[email protected]>
Date: Wed, 12 Mar 2008 13:18:49 +0200
Subject: [PATCH] scsi: Always complete BLOCK_PC commands

Current code, in some IO combinations could wrongly retry a BLOCK_PC command in chunks. This was never intended. Fix it that BLOCK_PC will always complete atomically

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/scsi_lib.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

drivers/scsi/scsi_lib.c +13 -11

index ba21d97..4b8b57c 100644
@@ -784,14 +784,20 @@ EXPORT_SYMBOL(scsi_release_buffers);
  * in req->data_len and req->next_rq->data_len. The upper-layer driver can
  * decide what to do with this information.
  */
-void scsi_end_bidi_request(struct scsi_cmnd *cmd)
+static void scsi_end_blk_request(struct scsi_cmnd *cmd)
 {
 	struct request *req = cmd->request;
 	unsigned int dlen = req->data_len;
-	unsigned int next_dlen = req->next_rq->data_len;
+	unsigned int next_dlen;
 
-	req->data_len = scsi_out(cmd)->resid;
-	req->next_rq->data_len = scsi_in(cmd)->resid;
+	if (blk_bidi_rq(req)) {
+		next_dlen = req->next_rq->data_len;
+		req->data_len = scsi_out(cmd)->resid;
+		req->next_rq->data_len = scsi_in(cmd)->resid;
+	} else {
+		next_dlen = 0;
+		req->data_len = scsi_get_resid(cmd);
+	}
 
 	/* The req and req->next_rq have not been completed */
 	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));

@@ -866,15 +872,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		if (scsi_bidi_cmnd(cmd)) {
-			/* will also release_buffers */
-			scsi_end_bidi_request(cmd);
-			return;
-		}
-		req->data_len = scsi_get_resid(cmd);
+		/* will also release_buffers */
+		scsi_end_blk_request(cmd);
+		return;
 	}
 
-	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
 	scsi_release_buffers(cmd);
 
 	/*

Boaz Harrosh wrote:
<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Mike

Quoted 52 lines

---
From: Boaz Harrosh <[email protected]>
Date: Wed, 12 Mar 2008 13:18:49 +0200
Subject: [PATCH] scsi: Always complete BLOCK_PC commands

Current code, in some IO combinations could wrongly retry a BLOCK_PC command in chunks. This was never intended. Fix it that BLOCK_PC will always complete atomically

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/scsi_lib.c |   24 +++++++++++++-----------
 1 files changed, 13 insertions(+), 11 deletions(-)

drivers/scsi/scsi_lib.c +10 -9

index ba21d97..4b8b57c 100644
@@ -784,14 +784,20 @@ EXPORT_SYMBOL(scsi_release_buffers);
  * in req->data_len and req->next_rq->data_len. The upper-layer driver can
  * decide what to do with this information.
  */
-void scsi_end_bidi_request(struct scsi_cmnd *cmd)
+static void scsi_end_blk_request(struct scsi_cmnd *cmd)
 {
 	struct request *req = cmd->request;
 	unsigned int dlen = req->data_len;
-	unsigned int next_dlen = req->next_rq->data_len;
+	unsigned int next_dlen;
 
-	req->data_len = scsi_out(cmd)->resid;
-	req->next_rq->data_len = scsi_in(cmd)->resid;
+	if (blk_bidi_rq(req)) {
+		next_dlen = req->next_rq->data_len;
+		req->data_len = scsi_out(cmd)->resid;
+		req->next_rq->data_len = scsi_in(cmd)->resid;
+	} else {
+		next_dlen = 0;
+		req->data_len = scsi_get_resid(cmd);
+	}
 
 	/* The req and req->next_rq have not been completed */
 	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));

@@ -866,15 +872,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		if (scsi_bidi_cmnd(cmd)) {
-			/* will also release_buffers */
-			scsi_end_bidi_request(cmd);
-			return;
-		}

drivers/scsi/scsi_lib.c void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) · line 882

Quoted 10 lines

drivers/scsi/scsi_lib.c +3 -2

-		req->data_len = scsi_get_resid(cmd);
+		/* will also release_buffers */
+		scsi_end_blk_request(cmd);
+		return;
 	}
 
-	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
 	scsi_release_buffers(cmd);
 
 	/*

--
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

Quoted 7 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Quoted 2 lines

Quoted 1 line

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Mike

Quoted 15 lines · Michael Reed

Quoted 9 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

and apply this patch, does it solve your original infinite loop?

Thanks
Boaz

Quoted 16 lines · Boaz Harrosh

Quoted 15 lines · Michael Reed

Quoted 9 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

and apply this patch, does it solve your original infinite loop?

By removing a fix in scsi_req_map_sg and forcing sg_start_req() to always call sg_build_indirect() (and not applying my fix to sg.c) I'm able to reproduce the problem without crashing the system.

With your patch applied to 2.6.25-rc4-git3 I get this.... (The mptscsih_qcmd output is evidence that the condition was generated which would have caused the infinite loop.)

mptscsih_qcmd: cmd e0000070845e0f00 / 18, dd 2, sg_count 1, sglist e00000709a785600, bufflen 255, bi_size 512
mptscsih_qcmd: cmd e0000070845e1500 / 18, dd 2, sg_count 1, sglist e00000709a785500, bufflen 255, bi_size 512
Pid: 0, CPU 10, comm: swapper
psr : 0000101008026038 ifs : 800000000000058f ip : [<a000000100554a00>] Not tainted (2.6.25-rc4-git3)
ip is at scsi_io_completion+0x2e0/0x900
unat: 0000000000000000 pfs : 000000000000058f rsc : 0000000000000003
rnat: 0bad0bad0baea565 bsps: a000000100094fe0 pr : 0bad0bad0bae9965
ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c0270033f
csd : 0000000000000000 ssd : 0000000000000000
b0 : a000000100554a00 b6 : a000000100090aa0 b7 : a0000001000a2640
f6 : 1003e000000000000b080 f7 : 1003e0000000000000000
f8 : 1003e00000000a066a81a f9 : 1003e000000080dc98009
f10 : 1003e0bd8b82c4612e8ea f11 : 1003e0000000000000005
r1 : a000000100eee010 r2 : ffffffffffff9400 r3 : a000000100c89348
r8 : 000000000000002e r9 : a000000100c89348 r10 : a000000100d58f30
r11 : e000007082368d54 r12 : e00000708236fb90 r13 : e000007082368000
r14 : 0000000000004000 r15 : a000000100c89348 r16 : a000000100c89330
r17 : e0000170bd607e18 r18 : 0000000000004000 r19 : 0000000000000000
r20 : 0000000000004000 r21 : e000007082368d50 r22 : 0000000000000000
r23 : 0000000000000001 r24 : 0000000000000000 r25 : 0000000000000000
r26 : 0000000000000002 r27 : 0000000000000000 r28 : 000000000000000a
r29 : e000007082368d54 r30 : a000000100ce4ef8 r31 : a000000100ce4e98

Call Trace:
 [<a0000001000128a0>] show_stack+0x40/0xa0
                                sp=e00000708236f760 bsp=e000007082369178
 [<a0000001000131b0>] show_regs+0x850/0x8a0
                                sp=e00000708236f930 bsp=e000007082369120
 [<a000000100033d10>] die+0x1b0/0x2e0
                                sp=e00000708236f930 bsp=e0000070823690d8
 [<a000000100033e90>] die_if_kernel+0x50/0x80
                                sp=e00000708236f930 bsp=e0000070823690a8
 [<a0000001000355f0>] ia64_bad_break+0x230/0x520
                                sp=e00000708236f930 bsp=e000007082369080
 [<a00000010000a260>] ia64_leave_kernel+0x0/0x270
                                sp=e00000708236f9c0 bsp=e000007082369080
 [<a000000100554a00>] scsi_io_completion+0x2e0/0x900
                                sp=e00000708236fb90 bsp=e000007082369008
 [<a000000100546570>] scsi_finish_command+0x1d0/0x200
                                sp=e00000708236fba0 bsp=e000007082368fd0

Entering kdb (current=0xe000007082368000, pid 0) on processor 10 Oops: <NULL> due to oops @ 0xa000000100554a00
psr: 0x0000101008026038 ifs: 0x800000000000058f ip: 0xa000000100554a00
unat: 0x0000000000000000 pfs: 0x000000000000058f rsc: 0x0000000000000003
rnat: 0x0bad0bad0baea565 bsps: 0xa000000100094fe0 pr: 0x0bad0bad0bae9965
ldrs: 0x0000000000000000 ccv: 0x0000000000000000 fpsr: 0x0009804c0270033f

  b0: 0xa000000100554a00    b6: 0xa000000100090aa0    b7: 0xa0000001000a2640
  r1: 0xa000000100eee010    r2: 0xffffffffffff9400    r3: 0xa000000100c89348
  r8: 0x000000000000002e    r9: 0xa000000100c89348   r10: 0xa000000100d58f30
 r11: 0xe000007082368d54   r12: 0xe00000708236fb90   r13: 0xe000007082368000
 r14: 0x0000000000004000   r15: 0xa000000100c89348   r16: 0xa000000100c89330
 r17: 0xe0000170bd607e18   r18: 0x0000000000004000   r19: 0x0000000000000000
 r20: 0x0000000000004000   r21: 0xe000007082368d50   r22: 0x0000000000000000
 r23: 0x0000000000000001   r24: 0x0000000000000000   r25: 0x0000000000000000
 r26: 0x0000000000000002   r27: 0x0000000000000000   r28: 0x000000000000000a
 r29: 0xe000007082368d54   r30: 0xa000000100ce4ef8   r31: 0xa000000100ce4e98

&regs = e00000708236f9d0

[10]kdb> bt
Stack traceback for pid 0
0xe000007082368000 0 1 1 10 R 0xe0000070823683b0 *swapper
0xa000000100554a00 scsi_io_completion+0x2e0

        args (0xe0000070845e0600, 0xff, 0x0, 0xe0000070845ddf38, 0x0, 0x0, 0xe000027085dfd368, 0xff, 0xa000000100546570)
0xa000000100546570 scsi_finish_command+0x1d0
        args (0xe0000070845e0600, 0xe000027085de5140, 0xe000027085de7800, 0xa0000001005556b0, 0x30a, 0xa000000100eee010)
0xa0000001005556b0 scsi_softirq_done+0x270
        args (0xe0000070845e0600, 0x2002, 0x0, 0xa0000001003aba60, 0x184, 0xe0000070845e0718)
0xa0000001003aba60 blk_done_softirq+0x140
        args (0xa0000001000b60b0, 0x790, 0xa000000100eee010)
0xa0000001000b60b0 __do_softirq+0xf0
        args (0xe0000270822784d0, 0xe000027082278480, 0xffffffff, 0xe000027085e0d880, 0xa00000010010af80, 0x40b, 0xa000000100eee010, 0xa00000010010aba0, 0x1)
0xa0000001000b6270 do_softirq+0x70
        args (0xa000000100bb8708, 0x0, 0xa00000010000ff70, 0x30a, 0xa000000100eee010, 0x218, 0xa000000100d0aac8, 0xa00000010010b040, 0x1008022038)
0xa0000001000b6560 irq_exit+0x80
        args (0xa00000010000fff0, 0x30a, 0x0)
0xa00000010000fff0 ia64_handle_irq+0x2f0
        args (0xf, 0x0, 0x0, 0xa00000010000a260, 0x2, 0xa000000100eee010)
0xa00000010000a260 ia64_leave_kernel
        args (0xf, 0x0)
0xa000000100013550 default_idle+0x110
        args (0xe00000708236fdc0, 0xa0000001000125e0, 0x40c, 0x10)
0xa0000001000125e0 cpu_idle+0x1e0
        args (0xa000000100940330, 0xa000000100d0aa48, 0xa, 0xa000000100dc69e8, 0xa0000001009a3b50, 0x40b, 0xa000000100eee010, 0xbad0bad0badaa65)
0xa0000001009a3b50 start_secondary+0x4d0
        args (0x20000500, 0x6e65470020000504, 0x400, 0xffffffff00, 0x3ff, 0xa000000100769fa0, 0x0, 0x3)

0xa000000100769fa0 __kprobes_text_end+0x340

Mike

Quoted 29 lines · Michael Reed

Quoted 16 lines · Boaz Harrosh

Quoted 15 lines · Michael Reed

Quoted 9 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Mike

<patch snipped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

and apply this patch, does it solve your original infinite loop?

By removing a fix in scsi_req_map_sg and forcing sg_start_req() to always call sg_build_indirect() (and not applying my fix to sg.c) I'm able to reproduce the problem without crashing the system.

Quoted 5 lines

With your patch applied to 2.6.25-rc4-git3 I get this.... (The mptscsih_qcmd output is evidence that the condition was generated which would have caused the infinite loop.)

Quoted 3 lines

Mike

I don't understand is that a NULL dereference due to my patch? did you manage to find what is the line of code that dereferences the NULL pointer.

Thanks
Boaz

Quoted 30 lines · Boaz Harrosh

Quoted 29 lines · Michael Reed

Quoted 16 lines · Boaz Harrosh

Quoted 15 lines · Michael Reed

Quoted 9 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Mike

<patch snipped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

and apply this patch, does it solve your original infinite loop?

By removing a fix in scsi_req_map_sg and forcing sg_start_req() to always call sg_build_indirect() (and not applying my fix to sg.c) I'm able to reproduce the problem without crashing the system.

Quoted 5 lines

Quoted 5 lines

With your patch applied to 2.6.25-rc4-git3 I get this.... (The mptscsih_qcmd output is evidence that the condition was generated which would have caused the infinite loop.)

< snip backtrace >

Quoted 4 lines

Quoted 2 lines

Mike

I don't understand is that a NULL dereference due to my patch? did you manage to find what is the line of code that dereferences the NULL pointer.

I'm going to say "yes", it's due to your patch. It's happened twice in a row.

Disabling inline functions gets me a better backtrace. And dumping the dmesg buffer I see the BUG in scsi_end_blk_request().

        BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));

I guess this is what I would expect to happen.

	blk_end_bidi_request ->
	  blk_end_io ->
	    __end_that_request_first

__end_that_request_first returns "1" indicating that the request wasn't completely finished.

I guess it could be argued that this really is a bug and that the buffer length and bi_size should always be the same. Would the same thing happen if a command returned a residual or an i/o error?

Mike

Quoted 15 lines · Michael Reed

Quoted 9 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

and apply this patch, does it solve your original infinite loop?

Thanks
Boaz

Quoted 23 lines · Boaz Harrosh

Quoted 8 lines · Michael Reed

Quoted 7 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Quoted 9 lines

Quoted 2 lines

Quoted 1 line

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

Quoted 1 line

and apply this patch, does it solve your original infinite loop?

Unfortunately, the infinite loop only occurred at 2.6.24. I'll see if I can break the current rc by removing some known fixes.

Care to gen a patch for 2.6.24?

Mike

Quoted 3 lines

Thanks
Boaz

Quoted 24 lines · Michael Reed

Quoted 23 lines · Boaz Harrosh

Quoted 8 lines · Michael Reed

Quoted 7 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Quoted 9 lines

Quoted 2 lines

Quoted 1 line

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

Quoted 11 lines

Quoted 1 line

and apply this patch, does it solve your original infinite loop?

Unfortunately, the infinite loop only occurred at 2.6.24. I'll see if I can break the current rc by removing some known fixes.

Care to gen a patch for 2.6.24?

Mike

Quoted 3 lines

Thanks
Boaz

here is the same principle on Linux 2.6.24
---
[PATCH] scsi: Always complete BLOCK_PC commands

Current code, in some IO combinations could wrongly retry a BLOCK_PC command in chunks. This was never intended. Fix it that BLOCK_PC will always complete atomically

Signed-off-by Boaz Harrosh <[email protected]>

---
git-diff --stat -p

 drivers/scsi/scsi_lib.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

drivers/scsi/scsi_lib.c +2 -0

index a9ac5b1..ae32012 100644
@@ -966,7 +966,16 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
+		good_bytes = req->data_len;
 		req->data_len = cmd->resid;
+retry_end_req:

New patch at end....

Quoted 61 lines · Boaz Harrosh

Quoted 24 lines · Michael Reed

Quoted 23 lines · Boaz Harrosh

Quoted 8 lines · Michael Reed

Quoted 7 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Quoted 9 lines

Quoted 2 lines

Quoted 1 line

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

Quoted 11 lines

Quoted 1 line

and apply this patch, does it solve your original infinite loop?

Unfortunately, the infinite loop only occurred at 2.6.24. I'll see if I can break the current rc by removing some known fixes.

Care to gen a patch for 2.6.24?

Mike

Quoted 3 lines

Thanks
Boaz

here is the same principle on Linux 2.6.24
---
[PATCH] scsi: Always complete BLOCK_PC commands

Current code, in some IO combinations could wrongly retry a BLOCK_PC command in chunks. This was never intended. Fix it that BLOCK_PC will always complete atomically

Signed-off-by Boaz Harrosh <[email protected]>

---
git-diff --stat -p

 drivers/scsi/scsi_lib.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

drivers/scsi/scsi_lib.c +2 -0

index a9ac5b1..ae32012 100644
@@ -966,7 +966,16 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
+		good_bytes = req->data_len;
 		req->data_len = cmd->resid;
+retry_end_req:

drivers/scsi/scsi_lib.c void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) · line 981

Quoted 10 lines

drivers/scsi/scsi_lib.c +7 -0

+		if (scsi_end_request(cmd, 1, good_bytes, 0) != NULL) {
+			WARN_ON(1);
+			good_bytes = req->data_len;
+			goto retry_end_req;
+		}
+		
+		return;
 	}
 
 	/*

I applied the patch to 2.6.24 and here's the result. It first completes 255 bytes, then repeatedly completes 56 bytes until it's all completed. This seems rather cumbersome. And points out that the rounding in sg_build_indirect() needs to be fixed.

scsi_execute_async: inquiry buf length 255, use_sg 1, bio e0000170820fae00, bi_size 512
mptscsih_qcmd: cmd e0000170b06e1980 / 18, dd 2, sg_count 1, sglist e0000170b340e900, bufflen 255, bi_size 512
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 255, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0

WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70

scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0

How 'bout this instead? It completes what finished and then ends the request. I think. I tried it using 2.6.24 and got no infinite loop.

drivers/scsi/scsi_lib.c +3 -4

@@ -657,13 +657,12 @@ static struct scsi_cmnd *scsi_end_reques
 	/*
 	 * If there are blocks left over at the end, set up the command
 	 * to queue the remainder of them.
+	 * blk_pc_requests are never requeued.
 	 */
-	if (end_that_request_chunk(req, uptodate, bytes)) {
+	if (end_that_request_chunk(req, uptodate, bytes) &&
+	    !blk_pc_request(req)) {
 		int leftover = (req->hard_nr_sectors << 9);
 
-		if (blk_pc_request(req))
-			leftover = req->data_len;
-
 		/* kill remainder if no retrys */
 		if (!uptodate && blk_noretry_request(req))
 			end_that_request_chunk(req, 0, leftover);

Signed-off-by Michael Reed <[email protected]>

Quoted 279 lines · Michael Reed

New patch at end....

Quoted 61 lines · Boaz Harrosh

Quoted 24 lines · Michael Reed

Quoted 23 lines · Boaz Harrosh

Quoted 8 lines · Michael Reed

Quoted 7 lines · Michael Reed

Boaz Harrosh wrote:
<snip>

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Just to demonstrate what I mean a patch is attached. Just as an RFC, totally untested.

I can try this out and see what happens.

Quoted 9 lines

Quoted 2 lines

Quoted 1 line

Will not compile here is a cleaner one

Still in my queue. Hopefully I'll get to poke at this today.

Patch compiles cleanly and appears to have no effect on the misc. sg_* commands I've executed including sg_dd, sg_inq, sg_luns, sg_readcap.

Mike

Quoted 2 lines

Mike

<patch sniped>

If you remove the original fix to sg.c
([PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24)

Quoted 11 lines

Quoted 1 line

and apply this patch, does it solve your original infinite loop?

Unfortunately, the infinite loop only occurred at 2.6.24. I'll see if I can break the current rc by removing some known fixes.

Care to gen a patch for 2.6.24?

Mike

Quoted 3 lines

Thanks
Boaz

here is the same principle on Linux 2.6.24
---
[PATCH] scsi: Always complete BLOCK_PC commands

Current code, in some IO combinations could wrongly retry a BLOCK_PC command in chunks. This was never intended. Fix it that BLOCK_PC will always complete atomically

Signed-off-by Boaz Harrosh <[email protected]>

---
git-diff --stat -p

 drivers/scsi/scsi_lib.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

drivers/scsi/scsi_lib.c +2 -0

index a9ac5b1..ae32012 100644
@@ -966,7 +966,16 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
+		good_bytes = req->data_len;
 		req->data_len = cmd->resid;
+retry_end_req:

drivers/scsi/scsi_lib.c void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes) · line 981

Quoted 10 lines

drivers/scsi/scsi_lib.c +7 -0

+		if (scsi_end_request(cmd, 1, good_bytes, 0) != NULL) {
+			WARN_ON(1);
+			good_bytes = req->data_len;
+			goto retry_end_req;
+		}
+		
+		return;
 	}
 
 	/*

I applied the patch to 2.6.24 and here's the result. It first completes 255 bytes, then repeatedly completes 56 bytes until it's all completed. This seems rather cumbersome. And points out that the rounding in sg_build_indirect() needs to be fixed.

scsi_execute_async: inquiry buf length 255, use_sg 1, bio e0000170820fae00, bi_size 512
mptscsih_qcmd: cmd e0000170b06e1980 / 18, dd 2, sg_count 1, sglist e0000170b340e900, bufflen 255, bi_size 512
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 255, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0

WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0
WARNING: at drivers/scsi/scsi_lib.c:1025 scsi_io_completion()

Call Trace:
 [<a000000100013ac0>] show_stack+0x40/0xa0
                                sp=e0000170821c79b0 bsp=e0000170821c1050
 [<a000000100013b50>] dump_stack+0x30/0x60
                                sp=e0000170821c7b80 bsp=e0000170821c1038
 [<a0000001005628b0>] scsi_io_completion+0x350/0x980
                                sp=e0000170821c7b80 bsp=e0000170821c0fd0
 [<a0000001005547b0>] scsi_finish_command+0x1d0/0x200
                                sp=e0000170821c7ba0 bsp=e0000170821c0fa0
 [<a000000100564070>] scsi_softirq_done+0x270/0x2a0
                                sp=e0000170821c7ba0 bsp=e0000170821c0f70
 [<a0000001003a8480>] blk_done_softirq+0x140/0x1a0
                                sp=e0000170821c7bb0 bsp=e0000170821c0f58
 [<a0000001000b6eb0>] __do_softirq+0xf0/0x240
                                sp=e0000170821c7bc0 bsp=e0000170821c0ee0
 [<a0000001000b7070>] do_softirq+0x70/0xc0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e80
 [<a0000001000b7360>] irq_exit+0x80/0xa0
                                sp=e0000170821c7bc0 bsp=e0000170821c0e68
 [<a000000100011250>] ia64_handle_irq+0x2f0/0x320
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a00000010000b100>] ia64_leave_kernel+0x0/0x270
                                sp=e0000170821c7bc0 bsp=e0000170821c0e38
 [<a000000100014bf0>] default_idle+0x110/0x180
                                sp=e0000170821c7d90 bsp=e0000170821c0df0
 [<a000000100013870>] cpu_idle+0x230/0x360
                                sp=e0000170821c7e30 bsp=e0000170821c0db0
 [<a000000100961910>] start_secondary+0x4d0/0x500
                                sp=e0000170821c7e30 bsp=e0000170821c0d70
 [<a0000001007619c0>] __kprobes_text_end+0x340/0x370
                                sp=e0000170821c7e30 bsp=e0000170821c0d70

scsi_io_completion: e0000170b06e1980: inquiry completion: result 0, good_bytes 56, bufflen 255, sense_valid 0, sense e0000170b34ac5c0 / 0

How 'bout this instead? It completes what finished and then ends the request. I think. I tried it using 2.6.24 and got no infinite loop.

drivers/scsi/scsi_lib.c +3 -4

@@ -657,13 +657,12 @@ static struct scsi_cmnd *scsi_end_reques
 	/*
 	 * If there are blocks left over at the end, set up the command
 	 * to queue the remainder of them.
+	 * blk_pc_requests are never requeued.
 	 */
-	if (end_that_request_chunk(req, uptodate, bytes)) {
+	if (end_that_request_chunk(req, uptodate, bytes) &&
+	    !blk_pc_request(req)) {
 		int leftover = (req->hard_nr_sectors << 9);
 
-		if (blk_pc_request(req))
-			leftover = req->data_len;
-
 		/* kill remainder if no retrys */
 		if (!uptodate && blk_noretry_request(req))
 			end_that_request_chunk(req, 0, leftover);

Quoted 1 line

Signed-off-by Michael Reed <[email protected]>

This will not work. Failing to complete the remaining of the request like that will leak BIOs. The upper layer bugs must be fixed. The WARN_ON is good because it identifies a bug at upper layer that must be fixed but goes on to complete the request. What you could do that will work well is at:

(quoted diff) +3 -0

+			WARN_ON(1);
+			good_bytes = req->data_len;
+			goto retry_end_req;

change that to be:

(quoted diff) +3 -0

+			WARN_ON(1);
+			good_bytes = ~0;
+			goto retry_end_req;

This will always insure a full complete of the request the second round, but will keep the WARN_ON for one time. The patch to scsi_req_map_sg should be applied for the stable 2.6.24.x releases, it is a bug.

drivers/scsi/sg.c line 1839

Quoted 2 lines · Michael Reed

drivers/scsi/sg.c +0 -2

-		++blk_size;	/* don't know why */
-/* round request up to next highest SG_SECTOR_SZ byte boundary */

I think we can remove this code. This was asked before, and here is Doug's reply:
http://www.mail-archive.com/[email protected]/msg11757.html

It was probaly due to some weirdness in that the scatterlist that sg.c made used to be sent directly to the LLD (now it is only used to hold/organize the pages/segments that the sg and st driver manages in their internal reserves), or maybe it had something to do with that and dma alignment problems or something. I really have no idea, but the block layer SG IO code has not been rounding up in its indirect path and it has been fine.