Messages in this thread Patch in this message | | | Date | Mon, 19 Apr 2004 14:42:10 -1000 | From | Warren Togami <> | Subject | Re: [PATCH] i2o_block Fix, possible CFQ elevator problem? |
| |
Jens Axboe wrote: >> >>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? > > > That looks pretty damn strange. Any chance you can capture a full oops, > with a serial console or similar?
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 ----------- [cut here ] --------- [please bite here ] --------- Kernel BUG at cfq_iosched:407 invalid operand: 0000 [1] SMP CPU 0 Pid: 1523, comm: bonnie++ Not tainted 2.6.5-1.326custom RIP: 0010:[<ffffffff80234f93>] <ffffffff80234f93>{cfq_next_request+66} RSP: 0000:ffffffff804a9048 EFLAGS: 00010016 RAX: 000001006cebf578 RBX: 000001007f3f1e00 RCX: 000001006ceb6340 RDX: 000001006cebf540 RSI: 0000000000000001 RDI: 000001007a225000 RBP: 000001007a225000 R08: ffffffff804a8ec8 R09: 0000000000000000 R10: 000001000289cb60 R11: 0000000000000000 R12: 000001007a225000 R13: 000001007a225000 R14: 000001001703bf58 R15: 0000000000509010 FS: 0000002a9555f360(0000) GS:ffffffff804a4e80(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b CR2: 0000002a95558000 CR3: 0000000000101000 CR4: 00000000000006e0 Process bonnie++ (pid: 1523, stackpage=1007fd5e1c0) Stack: 000001007aa01118 000001007d95abc0 000001007aa01118 ffffffff8022b444 000001007d95abc0 000001007aa01118 ffffffffa0137bc0 ffffffffa0133b82 0000000000000001 000001007d95abc0 Call Trace:<IRQ> <ffffffff8022b444>{elv_next_request+238} <ffffffffa0133b82>{:i2o_block:i2ob_request+233} <ffffffffa013370e>{:i2o_block:i2o_block_reply+1162} <ffffffff80113d85>{handle_IRQ_event+44} <ffffffffa00893ac>{:tg3:tg3_enable_ints+23} <ffffffff8013e7f9>{update_wall_time+12} <ffffffffa002908a>{:i2o_core:i2o_run_queue+124} <ffffffffa002b60e>{:i2o_core:i2o_pci_interrupt+9} <ffffffff80113d85>{handle_IRQ_event+44} <ffffffff801140b8>{do_IRQ+285} <ffffffff8013a894>{__do_softirq+76} <ffffffff8011185f>{ret_from_intr+0} <EOI>
Code: 0f 0b 74 3a 32 80 ff ff ff ff 97 01 48 89 c8 eb 0e 48 89 de RIP <ffffffff80234f93>{cfq_next_request+66} RSP <ffffffff804a9048> <0>Kernel panic: Aiee, killing interrupt handler! In interrupt handler - not syncing <0>Rebooting in 5 seconds..Badness in panic at kernel/panic.c:87
Call Trace:<IRQ> <ffffffff80136024>{panic+416} <ffffffff8011411d>{do_IRQ+386} <ffffffff8011185f>{ret_from_intr+0} <ffffffff80138cd3>{do_exit+87} <ffffffff80112ab5>{oops_end+80} <ffffffff80112bc5>{do_trap+0} <ffffffff80112cc2>{do_trap+253} <ffffffff80112f45>{do_invalid_op+145} <ffffffff80234f93>{cfq_next_request+66} <ffffffff8012ff68>{recalc_task_prio+343} <ffffffff8012ff68>{recalc_task_prio+343} <ffffffff80111d31>{error_exit+0} <ffffffff80234f93>{cfq_next_request+66} <ffffffff80156bea>{mempool_free+227} <ffffffff8022b444>{elv_next_request+238} <ffffffffa0133b82>{:i2o_block:i2ob_request+233} <ffffffffa013370e>{:i2o_block:i2o_block_reply+1162} <ffffffff80113d85>{handle_IRQ_event+44} <ffffffffa00893ac>{:tg3:tg3_enable_ints+23} <ffffffff8013e7f9>{update_wall_time+12} <ffffffffa002908a>{:i2o_core:i2o_run_queue+124} <ffffffffa002b60e>{:i2o_core:i2o_pci_interrupt+9} <ffffffff80113d85>{handle_IRQ_event+44} <ffffffff801140b8>{do_IRQ+285} <ffffffff8013a894>{__do_softirq+76} <ffffffff8011185f>{ret_from_intr+0} <EOI>
> > 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. > > >> /* >>+ * 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. >
Markus implemented all of your recommendations except the atomic part, which seemed to cause a kernel panic while loading the i2o_block driver. Attached is the latest i2o_block.c cleanup, while at this below URL is his attempt of removing the atomic type. He is not sure if he implemented it properly.
http://i2o.shadowconnect.com/i2o_block-cleanup.patch-broke
Your recommendations would be greatly appreciated!
Thank you, Warren Togami wtogami@redhat.com Markus Lidel Markus.Lidel@shadowconnect.com --- /root/linux-2.6.5-1.322/drivers/message/i2o/i2o_block.c.old 2004-04-15 12:05:05.000000000 -1000 +++ i2o_block.c 2004-04-20 12:46:38.000000000 -1000 @@ -117,9 +117,6 @@ * Some of these can be made smaller later */ -static int i2ob_media_change_flag[MAX_I2OB]; -static u32 i2ob_max_sectors[MAX_I2OB<<4]; - static int i2ob_context; /* @@ -143,6 +140,8 @@ int wcache; int power; int index; + int media_change_flag; + u32 max_sectors; }; /* @@ -162,9 +161,9 @@ }; /* - * Per IOP requst queue information + * Per IOP request queue information * - * We have a separate requeust_queue_t per IOP so that a heavilly + * We have a separate request_queue_t per IOP so that a heavilly * loaded I2O block device on an IOP does not starve block devices * across all I2O controllers. * @@ -183,7 +182,7 @@ * Each I2O disk is one of these. */ -static struct i2ob_device i2ob_dev[MAX_I2OB<<4]; +static struct i2ob_device i2ob_dev[MAX_I2OB]; static int i2ob_dev_count = 0; static struct gendisk *i2ob_disk[MAX_I2OB]; @@ -312,7 +311,6 @@ /* * Mask out partitions from now on */ - unit &= 0xF0; /* This can be optimised later - just want to be sure its right for starters */ @@ -453,8 +451,8 @@ struct i2ob_request *ireq = NULL; u8 st; u32 *m = (u32 *)msg; - u8 unit = (m[2]>>8)&0xF0; /* low 4 bits are partition */ - struct i2ob_device *dev = &i2ob_dev[(unit&0xF0)]; + u8 unit = m[2]>>8; + struct i2ob_device *dev = &i2ob_dev[unit]; /* * FAILed message @@ -599,7 +597,6 @@ unsigned int evt; unsigned long flags; int unit; - int i; //The only event that has data is the SCSI_SMART event. struct i2o_reply { u32 header[4]; @@ -648,7 +645,7 @@ */ case I2O_EVT_IND_BSA_VOLUME_LOAD: { - struct gendisk *p = i2ob_disk[unit>>4]; + struct gendisk *p = i2ob_disk[unit]; i2ob_install_device(i2ob_dev[unit].i2odev->controller, i2ob_dev[unit].i2odev, unit); add_disk(p); @@ -663,11 +660,10 @@ */ case I2O_EVT_IND_BSA_VOLUME_UNLOAD: { - struct gendisk *p = i2ob_disk[unit>>4]; + struct gendisk *p = i2ob_disk[unit]; del_gendisk(p); - for(i = unit; i <= unit+15; i++) - blk_queue_max_sectors(i2ob_dev[i].req_queue, 0); - i2ob_media_change_flag[unit] = 1; + blk_queue_max_sectors(i2ob_dev[unit].req_queue, 0); + i2ob_dev[unit].media_change_flag = 1; break; } @@ -695,7 +691,7 @@ i2ob_query_device(&i2ob_dev[unit], 0x0000, 4, &size, 8); spin_lock_irqsave(I2O_LOCK(unit), flags); - set_capacity(i2ob_disk[unit>>4], size>>9); + set_capacity(i2ob_disk[unit], size>>9); spin_unlock_irqrestore(I2O_LOCK(unit), flags); break; } @@ -765,14 +761,6 @@ u32 m; while ((req = elv_next_request(q)) != NULL) { - /* - * On an IRQ completion if there is an inactive - * request on the queue head it means it isnt yet - * ready to dispatch. - */ - if(req->rq_status == RQ_INACTIVE) - return; - dev = req->rq_disk->private_data; /* @@ -797,13 +785,12 @@ */ req->errors = 0; blkdev_dequeue_request(req); - req->waiting = NULL; ireq = i2ob_queues[dev->unit]->i2ob_qhead; i2ob_queues[dev->unit]->i2ob_qhead = ireq->next; ireq->req = req; - i2ob_send(m, dev, ireq, (dev->unit&0xF0)); + i2ob_send(m, dev, ireq, dev->index); } } @@ -1065,7 +1052,8 @@ u16 power; u32 flags, status; struct i2ob_device *dev=&i2ob_dev[unit]; - int i; + request_queue_t *q; + int segments; /* * For logging purposes... @@ -1079,8 +1067,8 @@ * before any I/O can be performed. If it fails, this * device is useless. */ - if(!i2ob_queues[unit]) { - if(i2ob_init_iop(unit)) + if(!i2ob_queues[c->unit]) { + if(i2ob_init_iop(c->unit)) return 1; } @@ -1089,11 +1077,11 @@ * code so that we can directly get the queue ptr from the * device instead of having to go the IOP data structure. */ - dev->req_queue = i2ob_queues[unit]->req_queue; + dev->req_queue = i2ob_queues[c->unit]->req_queue; /* initialize gendik structure */ - i2ob_disk[unit>>4]->private_data = dev; - i2ob_disk[unit>>4]->queue = dev->req_queue; + i2ob_disk[unit]->private_data = dev; + i2ob_disk[unit]->queue = dev->req_queue; /* * Ask for the current media data. If that isn't supported @@ -1110,7 +1098,7 @@ power = 0; i2ob_query_device(dev, 0x0000, 5, &flags, 4); i2ob_query_device(dev, 0x0000, 6, &status, 4); - set_capacity(i2ob_disk[unit>>4], size>>9); + set_capacity(i2ob_disk[unit], size>>9); /* * Max number of Scatter-Gather Elements @@ -1119,44 +1107,41 @@ i2ob_dev[unit].power = power; /* Save power state in device proper */ i2ob_dev[unit].flags = flags; - for(i=unit;i<=unit+15;i++) - { - request_queue_t *q = i2ob_dev[unit].req_queue; - int segments = (d->controller->status_block->inbound_frame_size - 7) / 2; + q = i2ob_dev[unit].req_queue; + segments = (d->controller->status_block->inbound_frame_size - 7) / 2; - if(segments > 16) - segments = 16; - - i2ob_dev[i].power = power; /* Save power state */ - i2ob_dev[unit].flags = flags; /* Keep the type info */ + if(segments > 16) + segments = 16; + + i2ob_dev[unit].power = power; /* Save power state */ + i2ob_dev[unit].flags = flags; /* Keep the type info */ - blk_queue_max_sectors(q, 96); /* 256 might be nicer but many controllers + blk_queue_max_sectors(q, 96); /* 256 might be nicer but many controllers explode on 65536 or higher */ - blk_queue_max_phys_segments(q, segments); - blk_queue_max_hw_segments(q, segments); + blk_queue_max_phys_segments(q, segments); + blk_queue_max_hw_segments(q, segments); - i2ob_dev[i].rcache = CACHE_SMARTFETCH; - i2ob_dev[i].wcache = CACHE_WRITETHROUGH; + i2ob_dev[unit].rcache = CACHE_SMARTFETCH; + i2ob_dev[unit].wcache = CACHE_WRITETHROUGH; - if(d->controller->battery == 0) - i2ob_dev[i].wcache = CACHE_WRITETHROUGH; + if(d->controller->battery == 0) + i2ob_dev[unit].wcache = CACHE_WRITETHROUGH; - if(d->controller->promise) - i2ob_dev[i].wcache = CACHE_WRITETHROUGH; + if(d->controller->promise) + i2ob_dev[unit].wcache = CACHE_WRITETHROUGH; - if(d->controller->short_req) - { - blk_queue_max_sectors(q, 8); - blk_queue_max_phys_segments(q, 8); - blk_queue_max_hw_segments(q, 8); - } + if(d->controller->short_req) + { + blk_queue_max_sectors(q, 8); + blk_queue_max_phys_segments(q, 8); + blk_queue_max_hw_segments(q, 8); } - strcpy(d->dev_name, i2ob_disk[unit>>4]->disk_name); - strcpy(i2ob_disk[unit>>4]->devfs_name, i2ob_disk[unit>>4]->disk_name); + strcpy(d->dev_name, i2ob_disk[unit]->disk_name); + strcpy(i2ob_disk[unit]->devfs_name, i2ob_disk[unit]->disk_name); printk(KERN_INFO "%s: Max segments %d, queue depth %d, byte limit %d.\n", - d->dev_name, i2ob_dev[unit].max_segments, i2ob_dev[unit].depth, i2ob_max_sectors[unit]<<9); + d->dev_name, i2ob_dev[unit].max_segments, i2ob_dev[unit].depth, i2ob_dev[unit].max_sectors<<9); i2ob_query_device(dev, 0x0000, 0, &type, 1); @@ -1197,7 +1182,7 @@ } printk(".\n"); printk(KERN_INFO "%s: Maximum sectors/read set to %d.\n", - d->dev_name, i2ob_max_sectors[unit]); + d->dev_name, i2ob_dev[unit].max_sectors); /* * Register for the events we're interested in and that the @@ -1323,7 +1308,7 @@ i2o_release_device(d, &i2o_block_handler); - if(scan_unit<MAX_I2OB<<4) + if(scan_unit<MAX_I2OB) { /* * Get the device and fill in the @@ -1339,8 +1324,8 @@ printk(KERN_WARNING "Could not install I2O block device\n"); else { - add_disk(i2ob_disk[scan_unit>>4]); - scan_unit+=16; + add_disk(i2ob_disk[scan_unit]); + scan_unit++; i2ob_dev_count++; /* We want to know when device goes away */ @@ -1350,7 +1335,7 @@ else { if(!warned++) - printk(KERN_WARNING "i2o_block: too many device, registering only %d.\n", scan_unit>>4); + printk(KERN_WARNING "i2o_block: too many device, registering only %d.\n", scan_unit); } } i2o_unlock_controller(c); @@ -1399,12 +1384,12 @@ printk(KERN_INFO " Controller %d Tid %d\n",c->unit, d->lct_data.tid); /* Check for available space */ - if(i2ob_dev_count>=MAX_I2OB<<4) + if(i2ob_dev_count>=MAX_I2OB) { printk(KERN_ERR "i2o_block: No more devices allowed!\n"); return; } - for(unit = 0; unit < (MAX_I2OB<<4); unit += 16) + for(unit = 0; unit < MAX_I2OB; unit ++) { if(!i2ob_dev[unit].i2odev) break; @@ -1425,7 +1410,7 @@ printk(KERN_ERR "i2o_block: Could not install new device\n"); else { - add_disk(i2ob_disk[unit>>4]); + add_disk(i2ob_disk[unit]); i2ob_dev_count++; i2o_device_notify_on(d, &i2o_block_handler); } @@ -1443,7 +1428,6 @@ void i2ob_del_device(struct i2o_controller *c, struct i2o_device *d) { int unit = 0; - int i = 0; unsigned long flags; spin_lock_irqsave(I2O_LOCK(c->unit), flags); @@ -1456,7 +1440,7 @@ printk(KERN_INFO "I2O Block Device Deleted\n"); - for(unit = 0; unit < MAX_I2OB<<4; unit += 16) + for(unit = 0; unit < MAX_I2OB; unit ++) { if(i2ob_dev[unit].i2odev == d) { @@ -1465,7 +1449,7 @@ break; } } - if(unit >= MAX_I2OB<<4) + if(unit >= MAX_I2OB) { printk(KERN_ERR "i2ob_del_device called, but not in dev table!\n"); spin_unlock_irqrestore(I2O_LOCK(c->unit), flags); @@ -1476,13 +1460,10 @@ * This will force errors when i2ob_get_queue() is called * by the kenrel. */ - del_gendisk(i2ob_disk[unit>>4]); + del_gendisk(i2ob_disk[unit]); i2ob_dev[unit].req_queue = NULL; - for(i = unit; i <= unit+15; i++) - { - i2ob_dev[i].i2odev = NULL; - blk_queue_max_sectors(i2ob_dev[i].req_queue, 0); - } + i2ob_dev[unit].i2odev = NULL; + blk_queue_max_sectors(i2ob_dev[unit].req_queue, 0); spin_unlock_irqrestore(I2O_LOCK(c->unit), flags); /* @@ -1493,14 +1474,13 @@ MOD_DEC_USE_COUNT; i2ob_dev[unit].refcnt = 0; - - i2ob_dev[i].tid = 0; + i2ob_dev[unit].tid = 0; /* * Do we need this? * The media didn't really change...the device is just gone */ - i2ob_media_change_flag[unit] = 1; + i2ob_dev[unit].media_change_flag = 1; i2ob_dev_count--; } @@ -1511,10 +1491,9 @@ static int i2ob_media_change(struct gendisk *disk) { struct i2ob_device *p = disk->private_data; - int i = p->index; - if(i2ob_media_change_flag[i]) + if(p->media_change_flag) { - i2ob_media_change_flag[i]=0; + p->media_change_flag=0; return 1; } return 0; @@ -1523,7 +1502,7 @@ static int i2ob_revalidate(struct gendisk *disk) { struct i2ob_device *p = disk->private_data; - return i2ob_install_device(p->controller, p->i2odev, p->index<<4); + return i2ob_install_device(p->controller, p->i2odev, p->index); } /* @@ -1536,7 +1515,7 @@ for(i=0;i<MAX_I2OB;i++) { - struct i2ob_device *dev=&i2ob_dev[(i<<4)]; + struct i2ob_device *dev=&i2ob_dev[i]; if(dev->refcnt!=0) { @@ -1598,23 +1577,32 @@ if (register_blkdev(MAJOR_NR, "i2o_block")) return -EIO; - for (i = 0; i < MAX_I2OB; i++) { - struct gendisk *disk = alloc_disk(16); - if (!disk) - goto oom; - i2ob_dev[i<<4].index = i; - disk->queue = i2ob_dev[i<<4].req_queue; - i2ob_disk[i] = disk; - } #ifdef MODULE printk(KERN_INFO "i2o_block: registered device at major %d\n", MAJOR_NR); #endif /* + * Set up the queue + */ + for(i = 0; i < MAX_I2O_CONTROLLERS; i++) + i2ob_queues[i] = NULL; + + /* * Now fill in the boiler plate */ - for (i = 0; i < MAX_I2OB << 4; i++) { + for (i = 0; i < MAX_I2OB; i++) { + struct gendisk *disk = alloc_disk(16); + if (!disk) + goto oom; + i2ob_disk[i] = disk; + + disk->major = MAJOR_NR; + disk->first_minor = i<<4; + disk->fops = &i2ob_fops; + sprintf(disk->disk_name, "i2o/hd%c", 'a' + i); + + i2ob_dev[i].index = i; i2ob_dev[i].refcnt = 0; i2ob_dev[i].flags = 0; i2ob_dev[i].controller = NULL; @@ -1623,26 +1614,10 @@ i2ob_dev[i].head = NULL; i2ob_dev[i].tail = NULL; i2ob_dev[i].depth = MAX_I2OB_DEPTH; - i2ob_max_sectors[i] = 2; - } - - for (i = 0; i < MAX_I2OB; i++) { - struct gendisk *disk = i2ob_disk[i]; - disk->major = MAJOR_NR; - disk->first_minor = i<<4; - disk->fops = &i2ob_fops; - sprintf(disk->disk_name, "i2o/hd%c", 'a' + i); + i2ob_dev[i].max_sectors = 2; } /* - * Set up the queue - */ - for(i = 0; i < MAX_I2O_CONTROLLERS; i++) - { - i2ob_queues[i] = NULL; - } - - /* * Register the OSM handler as we will need this to probe for * drives, geometry and other goodies. */ @@ -1701,10 +1676,10 @@ if(i2ob_dev_count) { struct i2o_device *d; for(i = 0; i < MAX_I2OB; i++) - if((d=i2ob_dev[i<<4].i2odev)) { + if((d=i2ob_dev[i].i2odev)) { i2o_device_notify_off(d, &i2o_block_handler); i2o_event_register(d->controller, d->lct_data.tid, - i2ob_context, i<<4, 0); + i2ob_context, i, 0); } } @@ -1726,14 +1701,27 @@ i2o_remove_handler(&i2o_block_handler); - for (i = 0; i < MAX_I2OB; i++) + for (i = 0; i < MAX_I2OB; i++) { + struct gendisk *disk = i2ob_disk[i]; + if(disk->flags & GENHD_FL_UP) + del_gendisk(i2ob_disk[i]); put_disk(i2ob_disk[i]); + } /* * Return the block device */ if (unregister_blkdev(MAJOR_NR, "i2o_block") != 0) printk("i2o_block: cleanup_module failed\n"); + + /* + * release request queue + */ + for (i = 0; i < MAX_I2O_CONTROLLERS; i ++) + if(i2ob_queues[i]) { + blk_cleanup_queue(i2ob_queues[i]->req_queue); + kfree(i2ob_queues[i]); + } } MODULE_AUTHOR("Red Hat"); | |