i2o_block Fix, possible CFQ elevator problem?

Test Systems:
x86 AMD Duron with PM3754 DPT controller, Two disks x86-64 2x Opteron with ASR-3010S Adaptec controller, Four disks

Software:
Gentoo 1.4.3.13 x86 with gcc-3.3.2
Fedora Core 2 x86-64 development with gcc-3.3.3-7

The i2o_block driver currently in kernel-2.6.5 has a problem when there is more than one logical block device on the I2O controller. Two or more devices causes kernel memory corruption,[1] sometimes accompanied by an oops, sometimes fatally in IRQ context and livelocks. In SMP mode this tended to cause panic=5 reboot to fail.

http://lwn.net/Articles/58199/
Markus finally figured out what was going wrong when he read in this LWN article that in the 2.6 kernel, the generic block layer handles partition code and there is almost nothing the individual drivers have to do. The attached patch from Markus removes this unused partition logic and seemed to make things work extremely well.

After some testing we began to realize why the kernel memory corruption that we saw with 2 or more block arrays was happening. The first array always had a valid partition table, while second and more arrays never did after they were split from the original single large array. The unused partition logic was copying the corrupted partition table from the 2nd and higher arrays into kernel memory, causing the trouble in kernel-2.6.5.

Subsequent testing was going extremely well, with simultaneous heavy disk usage on two I2O block devices with bonnie++ going stable. We then chopped the controller into four I2O block devices, but unfortunately this ran us into trouble. bonnie++ on three devices simultaneously would panic. Below is a photo of the call trace from this panic.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie.jpg

I noticed that the call trace contained "cfq_next_request", so I was curious what would happened if we changed to the deadline scheduler. Booted with the same kernel but with "elevator=deadline". To our surprise, bonnie++ ran simultaneously on all four I2O block devices without crashing the server. For another test we tried "elevator=as" and it too remained stable.

Possible CFQ I/O scheduler problem?

Quoted 10 lines

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie.jpg

I noticed that the call trace contained "cfq_next_request", so I was curious what would happened if we changed to the deadline scheduler. Booted with the same kernel but with "elevator=deadline". To our surprise, bonnie++ ran simultaneously on all four I2O block devices without crashing the server. For another test we tried "elevator=as" and it too remained stable.

Quoted 1 line

Possible CFQ I/O scheduler problem?

That looks pretty damn strange. Any chance you can capture a full oops, with a serial console or similar?

A quick look at i2o_block doesn't show anything that should cause this. There are a number of request handling badnesses in there, though:

- kill check fo RQ_INACTIVE, it's pointless. even if it wasn't, the inactive check return is buggy.

- you must not just clear req->waiting! remove that code.

- ->queue_depth should not be an expensive atomic type, you have the device lock when you look at/inc/dec it.

Review

Quoted 8 lines

	/*

(quoted diff) +7 -0

+	 *	Set up the queue
+	 */
+	for(i = 0; i < MAX_I2O_CONTROLLERS; i++)
+	{
+		i2ob_queues[i] = NULL;
+		i2ob_init_iop(i);
+	}

Only initialize queues that will be used. Each allocated but unusued queues wastes memory.

Quoted 20 lines · Warren Togami

Quoted 10 lines · Jens Axboe

Quoted 10 lines

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie.jpg

I noticed that the call trace contained "cfq_next_request", so I was curious what would happened if we changed to the deadline scheduler. Booted with the same kernel but with "elevator=deadline". To our surprise, bonnie++ ran simultaneously on all four I2O block devices without crashing the server. For another test we tried "elevator=as" and it too remained stable.

Quoted 6 lines

Quoted 1 line

Possible CFQ I/O scheduler problem?

That looks pretty damn strange. Any chance you can capture a full oops, with a serial console or similar?

Quoted 5 lines

Fedora Core 2 x86_64 SMP (2 x Opteron) with kernel-2.6.5-1.326 (based on 2.6.6-rc1)
gcc-3.3.3-7 plus attached patch. This panic was captured by netconsole.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie.txt

Next we tested cfq with the following section of code commented out. With this change the kernel no longer panics and seems to survive with four simultaneous bonnie++'s on all four block devices.

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +0 -0

14:09:43.000000000 -1000

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +0 -0

@@ -401,10 +401,12 @@
 dispatch:
                rq = list_entry_rq(cfqd->dispatch->next);

Quoted 11 lines

Quoted 1 line

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie.txt

Next we tested cfq with the following section of code commented out. With this change the kernel no longer panics and seems to survive with four simultaneous bonnie++'s on all four block devices.

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +0 -0

14:09:43.000000000 -1000

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +0 -0

@@ -401,10 +401,12 @@

dispatch:

               rq = list_entry_rq(cfqd->dispatch->next);

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20

Quoted 9 lines

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +2 -0

+/*
               BUG_ON(q->last_merge == rq);
               crq = RQ_DATA(rq);
               if (crq)
                       BUG_ON(ON_MHASH(crq));
+*/
 
               return rq;
       }

This is not safe, the BUG_ON is there for a reason. If the request in on the merge hash when handed to the driver, you risk corrupting data. The fix would be figuring out why this is happening. Maybe it's looking at bad data, could you test with this patch applied and see if the oops still triggers?

===== drivers/block/cfq-iosched.c 1.1 vs edited =====

Quoted 35 lines · Jens Axboe

Quoted 13 lines

Quoted 1 line

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie.txt

Next we tested cfq with the following section of code commented out. With this change the kernel no longer panics and seems to survive with four simultaneous bonnie++'s on all four block devices.

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +0 -0

14:09:43.000000000 -1000

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +0 -0

@@ -401,10 +401,12 @@

dispatch:

              rq = list_entry_rq(cfqd->dispatch->next);

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20

Quoted 9 lines

root/linux-2.6.5-1.326/drivers/block/cfq-iosched.c 2004-04-20 renamed from cfq-iosched.c 2004-04-20 13:52:55.000000000 -1000 +2 -0

+/*
              BUG_ON(q->last_merge == rq);
              crq = RQ_DATA(rq);
              if (crq)
                      BUG_ON(ON_MHASH(crq));
+*/
 
              return rq;
      }

This is not safe, the BUG_ON is there for a reason. If the request in on the merge hash when handed to the driver, you risk corrupting data. The fix would be figuring out why this is happening. Maybe it's looking at bad data, could you test with this patch applied and see if the oops still triggers?

===== drivers/block/cfq-iosched.c 1.1 vs edited =====

drivers/block/cfq-iosched.c line 409

Quoted 8 lines

drivers/block/cfq-iosched.c +1 -1

@@ -403,7 +403,7 @@
 
 		BUG_ON(q->last_merge == rq);
 		crq = RQ_DATA(rq);
-		if (crq)
+		if (blk_fs_request(rq) && crq)
 			BUG_ON(ON_MHASH(crq));
 
 		return rq;

We figured removing error handling was not safe, the previous post was only reporting test results to ask for more suggestions. I have now tested your suggested patch above and it seems to crash in the same way as originally.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie2.txt

This makes me curious, the other elevators lacked this type of error checking. Did this mean they were possibly allowing data corruption to happen with buggy drivers like this? Kind of scary! We were lucky to test this now, because this was one of the first FC kernels that included cfq by default.

Do you have any advice regarding the atomic type removal problem that we experienced from our previous post?

Quoted 3 lines

We figured removing error handling was not safe, the previous post was only reporting test results to ask for more suggestions. I have now tested your suggested patch above and it seems to crash in the same way as originally.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie2.txt

As a temporary safe work-around, you can apply this patch.

Quoted 1 line

This makes me curious, the other elevators lacked this type of error checking. Did this mean they were possibly allowing data corruption to happen with buggy drivers like this? Kind of scary! We were lucky to test this now, because this was one of the first FC kernels that included cfq by default.

Not necessarily, it's most likely a CFQ bug. Otherwise it would have surfaced before :-)

Quoted 18 lines · Jens Axboe

Quoted 6 lines

We figured removing error handling was not safe, the previous post was only reporting test results to ask for more suggestions. I have now tested your suggested patch above and it seems to crash in the same way as originally.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie2.txt

As a temporary safe work-around, you can apply this patch.

Quoted 5 lines

This makes me curious, the other elevators lacked this type of error checking. Did this mean they were possibly allowing data corruption to happen with buggy drivers like this? Kind of scary! We were lucky to test this now, because this was one of the first FC kernels that included cfq by default.

Quoted 3 lines

Not necessarily, it's most likely a CFQ bug. Otherwise it would have surfaced before :-)

I forgot to mention in the previous reports:

Prior to three of your original suggested cleanups of i2o_block, four simultaneous bonnie++'s on four independent arrays would almost immediately cause the crash while running elevator=cfq. After those three cleanups four simultaneous bonnie++ would survive for a while without crashing... until you run "sync" in another terminal. We however did not test it enough times to determine if without "sync" it can survive the test run. Do you want this tested without "sync"?

With the deadline scheduler "sync" would take maybe 30 seconds and return. With the cfq scheduler "sync" would be stuck there for much longer, then trigger the crash. Markus has suspected that it crashes when sync returns, but we have not confirmed that.

Quoted 4 lines

Quoted 3 lines

Not necessarily, it's most likely a CFQ bug. Otherwise it would have surfaced before :-)

Quoted 3 lines

I forgot to mention in the previous reports:

Prior to three of your original suggested cleanups of i2o_block, four simultaneous bonnie++'s on four independent arrays would almost immediately cause the crash while running elevator=cfq. After those three cleanups four simultaneous bonnie++ would survive for a while without crashing... until you run "sync" in another terminal. We however did not test it enough times to determine if without "sync" it can survive the test run. Do you want this tested without "sync"?

Repeat the tests that made it crash. The last patch I sent should work for you, at least until the real issue is found.

Quoted 15 lines · Jens Axboe

Quoted 13 lines

Quoted 3 lines

Not necessarily, it's most likely a CFQ bug. Otherwise it would have surfaced before :-)

I forgot to mention in the previous reports:

Prior to three of your original suggested cleanups of i2o_block, four simultaneous bonnie++'s on four independent arrays would almost immediately cause the crash while running elevator=cfq. After those three cleanups four simultaneous bonnie++ would survive for a while without crashing... until you run "sync" in another terminal. We however did not test it enough times to determine if without "sync" it can survive the test run. Do you want this tested without "sync"?

Quoted 3 lines

Repeat the tests that made it crash. The last patch I sent should work for you, at least until the real issue is found.

Tested your patch, it indeed does seem to keep the system stable. If I am understanding it right, the patch disables merging in the case where it would have caused a BUG condition? (Less efficiency.)

In any case, for now we are doing our i2o development using the other schedulers like deadline. Let us know if you have updated cfq patches to try, and we will.

Quoted 6 lines

Quoted 4 lines

Repeat the tests that made it crash. The last patch I sent should work for you, at least until the real issue is found.

Tested your patch, it indeed does seem to keep the system stable. If I am understanding it right, the patch disables merging in the case where it would have caused a BUG condition? (Less efficiency.)

Quoted 10 lines · Jens Axboe

Quoted 8 lines

Quoted 4 lines

Repeat the tests that made it crash. The last patch I sent should work for you, at least until the real issue is found.

Tested your patch, it indeed does seem to keep the system stable. If I am understanding it right, the patch disables merging in the case where it would have caused a BUG condition? (Less efficiency.)

Bad news... much later during the test the system locked up. During this test we did not use "sync" but just let all four bonnie++'s run.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie3.txt
----------- [cut here ] --------- [please bite here ] ---------

Kernel BUG at cfq_iosched:404

invalid operand: 0000 [1] SMP

Quoted 12 lines · Warren Togami

Quoted 10 lines · Jens Axboe

Quoted 8 lines

Quoted 4 lines

Repeat the tests that made it crash. The last patch I sent should work for you, at least until the real issue is found.

Tested your patch, it indeed does seem to keep the system stable. If I am understanding it right, the patch disables merging in the case where it would have caused a BUG condition? (Less efficiency.)

Quoted 7 lines

Bad news... much later during the test the system locked up. During this test we did not use "sync" but just let all four bonnie++'s run.

http://togami.com/~warren/archive/2004/i2o_cfq_quad_bonnie3.txt
----------- [cut here ] --------- [please bite here ] ---------

Kernel BUG at cfq_iosched:404

invalid operand: 0000 [1] SMP

Sorry about that, that's actually expected when we know this bug exists. You need to move the cfq_remove_merge_hints(q, crq) before the BUG_ON(q->last_merge == rq) check, or (better) just remove it completely. There's no way that q->last_merge could be set to this request after cfq_remove_merge_hints() was called.