PROBLEM: kernel BUG at block/cfq-iosched.c:3402

PATCH New Hugh Daschbach 4 participants 10 messages Mar 2, 2010 – Mar 19, 2010 SCSI

Share & tools

Hugh Daschbach

author

2010-03-02 00:04 UTC #0 lore raw

Apologies in advance if this seems like nagging. I've posted this before, but never saw it pass through the mailing list.

For long trespass CDBs, the EMC multipath device handler (in get_req()) allocates a "struct request" with READ specified in the I/O direction flag. It then changes this to a WRITE before send_trespass_cmd() calls blk_execute_rq(). This provokes a BUG() in cfq_put_request() after the I/O completes and the request is released.

cfq_set_request() and cfq_put_request() count READ and WRITE requests separately. Changing the I/O request direction after blk_get_request() allocates the request throws off this CFQ accounting.

Here's a traceback of the BUG.

Dec 28 15:06:07 ------------[ cut here ]------------

kernel BUG at block/cfq-iosched.c:3402!
invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
last sysfs file: /sys/devices/pci0000:00/0000:00:03.0/0000:02:00.1/irq CPU 1

Modules linked in: dm_round_robin scsi_dh_emc bnx2fc libfcoe libfc scsi_transport_fc cnic 8021q nfs fscache nfsd lockd bridge nfs_acl auth_rpcgss exportfs stp bnep sco l2cap crc16 bluetooth sunrpc iptable_filter ip_tables ip6table_filter ip6_tables x_tables ipv6 loop dm_multipath scsi_dh uinput sr_mod cdrom bnx2x ata_piix mdio serio_raw pcspkr rtc_cmos libata rtc_core rtc_lib bnx2 sg button joydev dcdbas dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod shpchp mptsas mptscsih mptbase scsi_transport_sas sd_mod scsi_mod ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]
Pid: 1645, comm: kmpath_handlerd Tainted: G        W  2.6.32-next #33 PowerEdge T710
RIP: 0010:[<ffffffff81168581>]  [<ffffffff81168581>] cfq_put_request+0x28/0x66
RSP: 0018:ffff880128e4dc50  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff8800ace10ea0 RCX: ffff880128ded1e0
RDX: 0000000000000001 RSI: ffff8800ace10ea0 RDI: ffff8800ace10ea0
RBP: ffff880128e4dc60 R08: ffff880128e4db90 R09: ffff880128ded218
R10: ffff880128e4dbe0 R11: ffffffff8115d3d9 R12: ffff880091b37ee0
R13: ffff8801124f3460 R14: 0000000000002000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff88002f200000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000931000 CR3: 000000010f220000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process kmpath_handlerd (pid: 1645, threadinfo ffff880128e4c000, task ffff880128decb40)
Stack:
 ffff8800ace10ea0 0000000001282c4f ffff880128e4dc70 ffffffff8115667d
<0> ffff880128e4dca0 ffffffff8115ce21 ffff8801124f3460 ffff8800ace10ea0
<0> 0000000000000282 ffff8801124f27f0 ffff880128e4dcd0 ffffffff8115d3e7
Call Trace:
 [<ffffffff8115667d>] elv_put_request+0x19/0x1b
 [<ffffffff8115ce21>] __blk_put_request+0x88/0xb9
 [<ffffffff8115d3e7>] blk_put_request+0x2e/0x46
 [<ffffffffa00206ed>] send_trespass_cmd+0x24e/0x260 [scsi_dh_emc]
 [<ffffffffa0020d9b>] clariion_activate+0x55/0x13e [scsi_dh_emc]
 [<ffffffff81062818>] ? trace_hardirqs_on_caller+0x113/0x13e
 [<ffffffffa0268cd7>] ? pg_init_done+0x0/0x187 [dm_multipath]
 [<ffffffffa01767ec>] scsi_dh_activate+0x70/0xbd [scsi_dh]
 [<ffffffffa0268cd7>] ? pg_init_done+0x0/0x187 [dm_multipath]
 [<ffffffffa026a4c1>] ? activate_path+0x0/0x2b [dm_multipath]
 [<ffffffffa026a4ea>] activate_path+0x29/0x2b [dm_multipath]
 [<ffffffff810519eb>] worker_thread+0x1af/0x2ae
 [<ffffffff81051992>] ? worker_thread+0x156/0x2ae
 [<ffffffff812f1a9a>] ? schedule+0x8e6/0x996
 [<ffffffff810549e4>] ? autoremove_wake_function+0x0/0x38
 [<ffffffff8105183c>] ? worker_thread+0x0/0x2ae
 [<ffffffff810548eb>] kthread+0x7d/0x86
 [<ffffffff810038fa>] child_rip+0xa/0x20
 [<ffffffff812f487c>] ? restore_args+0x0/0x30
 [<ffffffff81179e0b>] ? debug_object_activate+0x38/0xd8
 [<ffffffff81058de1>] ? sched_clock_cpu+0x42/0xc7
 [<ffffffff8105486e>] ? kthread+0x0/0x86
 [<ffffffff810038f0>] ? child_rip+0x0/0x20
Code: 5c c9 c3 55 48 89 e5 41 54 53 4c 8b a7 a8 00 00 00 48 89 fb 4d 85 e4 74 4b 8b 47 48 83 e0 01 48 63 d0 41 8b 44 94 68 85 c0 75 04 <0f> 0b eb fe ff c8 41 89 44 94 68 48 8b 87 a0 00 00 00 48 8b 78
RIP  [<ffffffff81168581>] cfq_put_request+0x28/0x66
 RSP <ffff880128e4dc50>
---[ end trace fd0fef9ed4a171a1 ]---

Hugh Daschbach (1):
Don't change direction flags in struct request.

 drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Hugh Daschbach

author

2010-03-02 00:04 UTC #1 lore raw

PATCH 1/1 Don't change direction flags in struct request.

The EMC multipath device handler should not change the I/O direction flags of its trespass command request.

The CFQ elevator may BUG if the direction flags on an I/O request are changed after allocation. cfq_set_request() and cfq_put_request() count READ and WRITE requests separately. Changing the I/O request direction after blk_get_request() allocates the request throws off this CFQ accounting.

Signed-off-by Hugh Daschbach <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/scsi/device_handler/scsi_dh_emc.c +4 -4

index 6196675..3709342 100644
@@ -269,10 +269,12 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 				unsigned char *buffer)
 {
 	struct request *rq;
+	int mode = READ;
 	int len = 0;
 
-	rq = blk_get_request(sdev->request_queue,
-			(cmd == MODE_SELECT) ? WRITE : READ, GFP_NOIO);
+	if (cmd == MODE_SELECT || cmd == MODE_SELECT_10)
+		mode = WRITE;
+	rq = blk_get_request(sdev->request_queue, mode, GFP_NOIO);
 	if (!rq) {
 		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
 		return NULL;

@@ -284,12 +286,10 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 	switch (cmd) {
 	case MODE_SELECT:
 		len = sizeof(short_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		break;
 	case MODE_SELECT_10:
 		len = sizeof(long_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		break;
 	case INQUIRY:

Signature · 1 line

1.6.6.196.g1f735

Hugh Daschbach

author

2010-03-02 00:04 UTC #2 lore raw

PATCH 1/1 Don't change direction flags in struct request.

In reply to Hugh Daschbach

The EMC multipath device handler should not change the I/O direction flags of its trespass command request.

Signed-off-by Hugh Daschbach <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/scsi/device_handler/scsi_dh_emc.c +4 -4

index 6196675..3709342 100644
@@ -269,10 +269,12 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 				unsigned char *buffer)
 {
 	struct request *rq;
+	int mode = READ;
 	int len = 0;
 
-	rq = blk_get_request(sdev->request_queue,
-			(cmd == MODE_SELECT) ? WRITE : READ, GFP_NOIO);
+	if (cmd == MODE_SELECT || cmd == MODE_SELECT_10)
+		mode = WRITE;
+	rq = blk_get_request(sdev->request_queue, mode, GFP_NOIO);
 	if (!rq) {
 		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
 		return NULL;

@@ -284,12 +286,10 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 	switch (cmd) {
 	case MODE_SELECT:
 		len = sizeof(short_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		break;
 	case MODE_SELECT_10:
 		len = sizeof(long_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		break;
 	case INQUIRY:

Signature · 1 line

1.6.6.196.g1f735

James Bottomley

2010-03-02 05:26 UTC #3 lore raw

Re: [PATCH] Don't change direction flags in struct request.

Quoted 8 lines · Hugh Daschbach

The EMC multipath device handler should not change the I/O direction flags of its trespass command request.
The CFQ elevator may BUG if the direction flags on an I/O request are changed after allocation. cfq_set_request() and cfq_put_request() count READ and WRITE requests separately. Changing the I/O request direction after blk_get_request() allocates the request throws off this CFQ accounting.

This description doesn't really match what the problem seems to be below:

Quoted 11 lines

Signed-off-by Hugh Daschbach <[email protected]>

---
 drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

drivers/scsi/device_handler/scsi_dh_emc.c +0 -0

index 6196675..3709342 100644
@@ -269,10 +269,12 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 				unsigned char *buffer)

drivers/scsi/device_handler/scsi_dh_emc.c static struct request *get_req(struct scsi_device *sdev, int cmd, · line 277

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_emc.c +4 -2

 {
 	struct request *rq;
+	int mode = READ;
 	int len = 0;
 
-	rq = blk_get_request(sdev->request_queue,
-			(cmd == MODE_SELECT) ? WRITE : READ, GFP_NOIO);
+	if (cmd == MODE_SELECT || cmd == MODE_SELECT_10)
+		mode = WRITE;
+	rq = blk_get_request(sdev->request_queue, mode, GFP_NOIO);

So the actual bug is failure to set WRITE for MODE_SELECT_10.

drivers/scsi/device_handler/scsi_dh_emc.c static struct request *get_req(struct scsi_device *sdev, int cmd, · line 287

Quoted 8 lines

drivers/scsi/device_handler/scsi_dh_emc.c +0 -1

 	if (!rq) {
 		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
 		return NULL;

@@ -284,12 +286,10 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 	switch (cmd) {
 	case MODE_SELECT:
 		len = sizeof(short_trespass);
-		rq->cmd_flags |= REQ_RW;

And this is just cosmetic ... the flags already having been set by the allocation, they don't need to be set again.

drivers/scsi/device_handler/scsi_dh_emc.c static struct request *get_req(struct scsi_device *sdev, int cmd, · line 295

Quoted 8 lines

drivers/scsi/device_handler/scsi_dh_emc.c +0 -1

 		rq->cmd[1] = 0x10;
 		break;
 	case MODE_SELECT_10:
 		len = sizeof(long_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		break;
 	case INQUIRY:

Chandra, can you tidy this up and take it, please?

Thanks,

James

Mike Christie

2010-03-02 07:52 UTC #4 lore raw

Re: [PATCH] Don't change direction flags in struct request.

Quoted 29 lines · James Bottomley

Quoted 8 lines · Hugh Daschbach
The EMC multipath device handler should not change the I/O direction flags of its trespass command request.
The CFQ elevator may BUG if the direction flags on an I/O request are changed after allocation. cfq_set_request() and cfq_put_request() count READ and WRITE requests separately. Changing the I/O request direction after blk_get_request() allocates the request throws off this CFQ accounting.
This description doesn't really match what the problem seems to be below:
Quoted 16 lines
Signed-off-by Hugh Daschbach<[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
  1 files changed, 4 insertions(+), 4 deletions(-)
drivers/scsi/device_handler/scsi_dh_emc.c +1 -0
index 6196675..3709342 100644
@@ -269,10 +269,12 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
  				unsigned char *buffer)
  {
  	struct request *rq;
+	int mode = READ;
  	int len = 0;
 

Quoted 8 lines

drivers/scsi/device_handler/scsi_dh_emc.c static struct request *get_req(struct scsi_device *sdev, int cmd, · line 277
Quoted 5 lines
drivers/scsi/device_handler/scsi_dh_emc.c +3 -2
-	rq = blk_get_request(sdev->request_queue,
-			(cmd == MODE_SELECT) ? WRITE : READ, GFP_NOIO);
+	if (cmd == MODE_SELECT || cmd == MODE_SELECT_10)
+		mode = WRITE;
+	rq = blk_get_request(sdev->request_queue, mode, GFP_NOIO);
So the actual bug is failure to set WRITE for MODE_SELECT_10.

I think we have a fix for this and some len issues in that module that was sent here:
http://marc.info/?l=linux-scsi&m=125978574800618&w=2

James Bottomley

2010-03-03 10:29 UTC #5 lore raw

Re: [PATCH] Don't change direction flags in struct request.

Quoted 39 lines · Mike Christie

Quoted 29 lines · James Bottomley
Quoted 8 lines · Hugh Daschbach
The EMC multipath device handler should not change the I/O direction flags of its trespass command request.
The CFQ elevator may BUG if the direction flags on an I/O request are changed after allocation. cfq_set_request() and cfq_put_request() count READ and WRITE requests separately. Changing the I/O request direction after blk_get_request() allocates the request throws off this CFQ accounting.
This description doesn't really match what the problem seems to be below:
Quoted 16 lines
Signed-off-by Hugh Daschbach<[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
  1 files changed, 4 insertions(+), 4 deletions(-)
drivers/scsi/device_handler/scsi_dh_emc.c +1 -0
index 6196675..3709342 100644
@@ -269,10 +269,12 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
  				unsigned char *buffer)
  {
  	struct request *rq;
+	int mode = READ;
  	int len = 0;
 
Quoted 8 lines
drivers/scsi/device_handler/scsi_dh_emc.c static struct request *get_req(struct scsi_device *sdev, int cmd, · line 277
Quoted 5 lines
drivers/scsi/device_handler/scsi_dh_emc.c +3 -2
-	rq = blk_get_request(sdev->request_queue,
-			(cmd == MODE_SELECT) ? WRITE : READ, GFP_NOIO);
+	if (cmd == MODE_SELECT || cmd == MODE_SELECT_10)
+		mode = WRITE;
+	rq = blk_get_request(sdev->request_queue, mode, GFP_NOIO);
So the actual bug is failure to set WRITE for MODE_SELECT_10.

Quoted 3 lines

I think we have a fix for this and some len issues in that module that was sent here:
http://marc.info/?l=linux-scsi&m=125978574800618&w=2

Yes, you did ... the need to change the From: field caused me to mark it in a different way and ultimately lose it ... I'll add it this time ...

James

Mike Christie

2010-03-12 22:01 UTC #6 lore raw

Re: [PATCH] Don't change direction flags in struct request.

In reply to Hugh Daschbach

Quoted 36 lines · Hugh Daschbach

The EMC multipath device handler should not change the I/O direction flags of its trespass command request.
The CFQ elevator may BUG if the direction flags on an I/O request are changed after allocation. cfq_set_request() and cfq_put_request() count READ and WRITE requests separately. Changing the I/O request direction after blk_get_request() allocates the request throws off this CFQ accounting.
Signed-off-by Hugh Daschbach<[email protected]>
---
  drivers/scsi/device_handler/scsi_dh_emc.c |    8 ++++----
  1 files changed, 4 insertions(+), 4 deletions(-)
drivers/scsi/device_handler/scsi_dh_emc.c +4 -2
index 6196675..3709342 100644
@@ -269,10 +269,12 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
  				unsigned char *buffer)
  {
  	struct request *rq;
+	int mode = READ;
  	int len = 0;
 
-	rq = blk_get_request(sdev->request_queue,
-			(cmd == MODE_SELECT) ? WRITE : READ, GFP_NOIO);
+	if (cmd == MODE_SELECT || cmd == MODE_SELECT_10)
+		mode = WRITE;
+	rq = blk_get_request(sdev->request_queue, mode, GFP_NOIO);
  	if (!rq) {
  		sdev_printk(KERN_INFO, sdev, "get_req: blk_get_request failed");
  		return NULL;
@@ -284,12 +286,10 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
  	switch (cmd) {
  	case MODE_SELECT:

drivers/scsi/device_handler/scsi_dh_emc.c static struct request *get_req(struct scsi_device *sdev, int cmd, · line 295

Quoted 10 lines

drivers/scsi/device_handler/scsi_dh_emc.c +0 -2

  		len = sizeof(short_trespass);
-		rq->cmd_flags |= REQ_RW;
  		rq->cmd[1] = 0x10;
  		break;
  	case MODE_SELECT_10:
  		len = sizeof(long_trespass);
-		rq->cmd_flags |= REQ_RW;
  		rq->cmd[1] = 0x10;
  		break;
  	case INQUIRY:

Did you try the patch I linked to the last time you posted about this?

I think there should be a patch which fixes this problem here:
http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commitdiff;h=6c71dcb28ff9b63b814a0b76a256f5dae08d3e0d;hp=3a5b27bf6f29574d667230c7e76e4b83fe3014e0
I think it got into 2.6.34-rc1.

With that patch mode selects should get set up as writes.

Does it work? If it does then I think we just need the attached cleanup patch which removes the cmd_flags setting. The patch that got merged fixed the problem of initializing the request properly, but it forgot to remove the cmd_flags setting.

blk_get_request sets the cmd_flags, so we should not and do not need to set them. If we did set them to a different value then it can cause a oops in the elevator code.

Signed-off-by Mike Christie <[email protected]>

drivers/scsi/device_handler/scsi_dh_emc.c +0 -2

index 63032ec..2752892 100644
@@ -284,13 +284,11 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
 	switch (cmd) {
 	case MODE_SELECT:
 		len = sizeof(short_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		rq->cmd[4] = len;
 		break;
 	case MODE_SELECT_10:
 		len = sizeof(long_trespass);
-		rq->cmd_flags |= REQ_RW;
 		rq->cmd[1] = 0x10;
 		rq->cmd[8] = len;
 		break;

Hugh Daschbach

author

2010-03-12 22:26 UTC #7 lore raw

RE: [PATCH] Don't change direction flags in struct request.

Quoted 3 lines · Mike Christie [mailto:[email protected]] writes:

Quoted 2 lines · Hugh Daschbach
The EMC multipath device handler should not change the I/O direction flags of its trespass command request.

...

Quoted 1 line

Did you try the patch I linked to the last time you posted about this?

I have not. When I read it, I believed it would fix the issue.

Quoted 3 lines

I think there should be a patch which fixes this problem here:
http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commitdiff;h=6c71dcb28ff9b63b814a0b76a256f5dae08d3e0d;hp=3a5b27bf6f29574d667230c7e76e4b83fe3014e0
I think it got into 2.6.34-rc1.

Then I'll pull it and test it.

Quoted 6 lines

With that patch mode selects should get set up as writes.
Does it work? If it does then I think we just need the attached cleanup patch which removes the cmd_flags setting. The patch that got merged fixed the problem of initializing the request properly, but it forgot to remove the cmd_flags setting.

Agree. I'll test it early next week and confirm.

Thanks,
Hugh

Hugh Daschbach

author

2010-03-19 02:21 UTC #8 lore raw

RE: [PATCH] Don't change direction flags in struct request.

In reply to Mike Christie

Quoted 5 lines · Hugh Daschbach [mailto:[email protected]] wrote:

Quoted 3 lines · Mike Christie [mailto:[email protected]] writes:
Quoted 2 lines · Hugh Daschbach
The EMC multipath device handler should not change the I/O direction flags of its trespass command request.

...

Quoted 10 lines

Quoted 1 line
Did you try the patch I linked to the last time you posted about this?
I have not. When I read it, I believed it would fix the issue.
Quoted 3 lines
I think there should be a patch which fixes this problem here:
http://git.kernel.org/?p=linux/kernel/git/jejb/scsi-misc-2.6.git;a=commitdiff;h=6c71dcb28ff9b63b814a0b76a256f5dae08d3e0d;hp=3a5b27bf6f29574d667230c7e76e4b83fe3014e0
I think it got into 2.6.34-rc1.
Then I'll pull it and test it.

Quoted 8 lines

Quoted 6 lines
With that patch mode selects should get set up as writes.
Does it work? If it does then I think we just need the attached cleanup patch which removes the cmd_flags setting. The patch that got merged fixed the problem of initializing the request properly, but it forgot to remove the cmd_flags setting.
Agree. I'll test it early next week and confirm.

I have pulled and tested this patch. It does fix my issue.

Thanks,
Hugh

Dmitry Torokhov

2010-03-02 00:20 UTC #9 lore raw

In reply to Hugh Daschbach

Hi,

On Thu, Mar 18, 2010 at 10:31:29PM -0700, Hugh Daschbach wrote:

	^^^^^^^^^^^^

Quoted 3 lines

Apologies in advance if this seems like nagging. I've posted this before, but never saw it pass through the mailing list.

Could it be that they were sent from more distant future and not reached us yet? ;)

Signature · 1 line

Dmitry