Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[James Bottomley]

On Tue, 2009-03-03 at 23:07 +0100, Thomas Gleixner wrote:
> On Tue, 3 Mar 2009, Thomas Gleixner wrote:
> > My bad. I was playing with that to get rid of the aic7xxx wreckage on
> > one of my test boxen and forgot to remove it.
> 
> While the one below is definitey not my fault. It's on Linus latest:
> 
>  commit 2450cf51a1bdba7037e91b1bcc494b01c58aaf66
> 
> While compiling a kernel I triggerred the BUG below. Not so nice as it
> took a whole filesystem with it. fsck took more than 20 min to recover
> the leftovers :(
> 
> Thanks,
> 
> 	tglx
> 
> 
> ------------[ cut here ]------------
> kernel BUG at /home/tglx/work/kernel/git/linux-2.6/drivers/scsi/scsi_lib.c:1141!

This is BUG_ON(count > sdb->table.nents);

It looks like the sg list got split and grew in size ... I suspect this
might be libata related, so cc'ing the ide list.  I suspect either the
block layer initially parametrised this wrongly (tomo bug) or a sg list
got split then requeued (something in libata?).

James


> invalid opcode: 0000 [#1] SMP 
> last sysfs file: /sys/devices/pci0000:00/0000:00:1c.5/0000:03:00.0/irq
> CPU 0 
> Modules linked in: ext2 ipt_MASQUERADE iptable_nat nf_nat bridge stp autofs4 coretemp lm85 hwmon_vid hwmon nfs lockd nfs_acl auth_rpcgss fuse sunrpc 8139too mii ipt_REJECT nf_conntrack_ipv4 nf_defrag_ipv4 iptable_filter ip_tables ip6t_REJECT ip6t_ipv6header xt_tcpudp nf_conntrack_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables x_tables ipv6 dm_mirror dm_region_hash dm_log dm_multipath dm_mod raid0 kvm_intel kvm uinput snd_hda_codec_idt snd_hda_intel snd_hda_codec snd_hwdep snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd ppdev i2c_i801 firewire_ohci soundcore parport_pc i82975x_edac e1000e firewire_core snd_page_alloc i2c_core iTCO_wdt sr_mod edac_core parport sg floppy crc_itu_t iTCO_vendor_support pcspkr serio_raw
  cdrom ata_piix sata_sil ahci libata sd_mod scsi_mod raid456 async_xor async_memcpy async_tx xor raid1 ext3 jbd mbcache uhci_hcd ohci_hcd ehci_hcd [last unloaded: freq_table]
> Pid: 16556, comm: cc1 Not tainted 2.6.29-rc6 #155         
> RIP: 0010:[<ffffffffa00ad30b>]  [<ffffffffa00ad30b>] scsi_init_sgtable+0x51/0x9f [scsi_mod]
> RSP: 0000:ffffffff8163cb40  EFLAGS: 00010002
> RAX: 000000000000002d RBX: ffff88004b7a3590 RCX: 00000000ffffffff
> RDX: 000000000000000b RSI: ffff880034b4d0a0 RDI: 0000000000002000
> RBP: ffffffff8163cb50 R08: ffff880001017b50 R09: ffffffff8163cb90
> R10: ffff88007d281000 R11: 0000000000000000 R12: ffff88003b323c58
> R13: 00000000082b73a7 R14: ffffffff8163cc48 R15: 00000000000003f0
> FS:  00007fe47f3ea6f0(0000) GS:ffffffff81645080(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fe47a37f000 CR3: 00000000443a8000 CR4: 00000000000026e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process cc1 (pid: 16556, threadinfo ffff88001c332000, task ffff880008398000)
> Stack:
>  ffff88003b323c00 ffff88007d281000 ffffffff8163cb70 ffffffffa00ad550
>  ffff88004b7a3590 ffff88007d281000 ffffffff8163cba0 ffffffffa00ad6b1
>  ffff88007d1f1410 ffff88007d1f0000 ffff88004b7a3590 ffff88007d281000
> Call Trace:
>  <IRQ> <0> [<ffffffffa00ad550>] scsi_init_io+0x1f/0xc9 [scsi_mod]
>  [<ffffffffa00ad6b1>] scsi_setup_fs_cmnd+0xb7/0xbf [scsi_mod]
>  [<ffffffffa00d5175>] sd_prep_fn+0x65/0x81c [sd_mod]
>  [<ffffffffa00a768c>] ? scsi_done+0x0/0x12 [scsi_mod]
>  [<ffffffffa00a768c>] ? scsi_done+0x0/0x12 [scsi_mod]
>  [<ffffffff8114345e>] elv_next_request+0xd7/0x19e
>  [<ffffffffa00acc45>] scsi_request_fn+0x90/0x4cf [scsi_mod]
>  [<ffffffff81144e86>] blk_invoke_request_fn+0x75/0x14a
>  [<ffffffff8114547a>] __blk_run_queue+0x25/0x29
>  [<ffffffff8114549f>] blk_run_queue+0x21/0x35
>  [<ffffffffa00ac4f3>] scsi_run_queue+0x2e3/0x37d [scsi_mod]
>  [<ffffffffa00ad2aa>] scsi_next_command+0x36/0x46 [scsi_mod]
>  [<ffffffffa00addd7>] scsi_io_completion+0x3df/0x423 [scsi_mod]
>  [<ffffffffa00df3ad>] ? __ata_qc_complete+0xc0/0xc9 [libata]
>  [<ffffffffa00e1011>] ? ata_qc_complete+0x1ea/0x1f3 [libata]
>  [<ffffffffa00a7683>] scsi_finish_command+0xe9/0xf2 [scsi_mod]
>  [<ffffffffa00adf45>] scsi_softirq_done+0x11a/0x123 [scsi_mod]
>  [<ffffffff81149275>] blk_done_softirq+0x69/0x79
>  [<ffffffff810412b7>] __do_softirq+0x83/0x144
>  [<ffffffff8100d23c>] call_softirq+0x1c/0x28
>  [<ffffffff8100e17c>] do_softirq+0x34/0x76
>  [<ffffffff81040fd1>] irq_exit+0x3f/0x79
>  [<ffffffff8100e43f>] do_IRQ+0x130/0x155
>  [<ffffffff8100cb13>] ret_from_intr+0x0/0xa
>  <EOI> <0>Code: 80 00 00 00 4c 89 e7 e8 0a 0d 0b e1 85 c0 74 45 48 c7 c2 59 d3 0a a0 be 80 00 00 00 4c 89 e7 e8 78 09 0b e1 b8 02 00 00 00 eb 25 <0f> 0b eb fe 41 89 44 24 08 83 7b 4c 02 75 08 8b 83 18 01 00 00 
> RIP  [<ffffffffa00ad30b>] scsi_init_sgtable+0x51/0x9f [scsi_mod]
>  RSP <ffffffff8163cb40>
> ---[ end trace 1b0175d9b3d81293 ]---

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Thomas Gleixner]

On Tue, 3 Mar 2009, James Bottomley wrote:

> On Tue, 2009-03-03 at 23:07 +0100, Thomas Gleixner wrote:
> > On Tue, 3 Mar 2009, Thomas Gleixner wrote:
> > > My bad. I was playing with that to get rid of the aic7xxx wreckage on
> > > one of my test boxen and forgot to remove it.
> > 
> > While the one below is definitey not my fault. It's on Linus latest:
> > 
> >  commit 2450cf51a1bdba7037e91b1bcc494b01c58aaf66
> > 
> > While compiling a kernel I triggerred the BUG below. Not so nice as it
> > took a whole filesystem with it. fsck took more than 20 min to recover
> > the leftovers :(
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> > 
> > ------------[ cut here ]------------
> > kernel BUG at /home/tglx/work/kernel/git/linux-2.6/drivers/scsi/scsi_lib.c:1141!
> 
> This is BUG_ON(count > sdb->table.nents);
> 
> It looks like the sg list got split and grew in size ... I suspect this
> might be libata related, so cc'ing the ide list.  I suspect either the
> block layer initially parametrised this wrongly (tomo bug) or a sg list
> got split then requeued (something in libata?).

FYI, after I've lost a full day of work including the results of four
"iozone -a -g 4G" runs I tried to reproduce the problem on that
machine - the leftovers of the filesystem are pretty useless anyway.

It took about 2hrs to trigger the bug again. Same back trace.

Anything I can do what might help to decode the problem ?

Thanks,

	tglx

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[James Bottomley]

On Wed, 2009-03-04 at 03:01 +0100, Thomas Gleixner wrote: 
> On Tue, 3 Mar 2009, James Bottomley wrote:
> 
> > On Tue, 2009-03-03 at 23:07 +0100, Thomas Gleixner wrote:
> > > On Tue, 3 Mar 2009, Thomas Gleixner wrote:
> > > > My bad. I was playing with that to get rid of the aic7xxx wreckage on
> > > > one of my test boxen and forgot to remove it.
> > > 
> > > While the one below is definitey not my fault. It's on Linus latest:
> > > 
> > >  commit 2450cf51a1bdba7037e91b1bcc494b01c58aaf66
> > > 
> > > While compiling a kernel I triggerred the BUG below. Not so nice as it
> > > took a whole filesystem with it. fsck took more than 20 min to recover
> > > the leftovers :(
> > > 
> > > Thanks,
> > > 
> > > 	tglx
> > > 
> > > 
> > > ------------[ cut here ]------------
> > > kernel BUG at /home/tglx/work/kernel/git/linux-2.6/drivers/scsi/scsi_lib.c:1141!
> > 
> > This is BUG_ON(count > sdb->table.nents);
> > 
> > It looks like the sg list got split and grew in size ... I suspect this
> > might be libata related, so cc'ing the ide list.  I suspect either the
> > block layer initially parametrised this wrongly (tomo bug) or a sg list
> > got split then requeued (something in libata?).
> 
> FYI, after I've lost a full day of work including the results of four
> "iozone -a -g 4G" runs I tried to reproduce the problem on that
> machine - the leftovers of the filesystem are pretty useless anyway.
> 
> It took about 2hrs to trigger the bug again. Same back trace.
> 
> Anything I can do what might help to decode the problem ?

I discussed this with Fujita Tomonori ... we think it's probably in the
generic block merging code. 

Could you run with this debugging code added until the fault triggers so
we can get an exact view of what the layout of the request is and why
we're getting an extra segment on mapping?

Thanks,

James

P.S. I think if you take the BUG() statement out, as long as it's only
one segment over, the machine should stay up long enough for a clean
shutdown.

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 940dc32..5219153 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1139,7 +1139,33 @@ static int scsi_init_sgtable(struct request *req, struct scsi_data_buffer *sdb,
 	 * each segment.
 	 */
 	count = blk_rq_map_sg(req->q, req, sdb->table.sgl);
-	BUG_ON(count > sdb->table.nents);
+	if (unlikely(count > sdb->table.nents)) {
+		struct bio_vec *bvec;
+		struct req_iterator iter;
+		struct scatterlist *sg;
+		int i=0;
+
+		printk(KERN_ERR "MAPPING miscount %d phys maps to %d\n",
+		       sdb->table.nents, count);
+		blk_dump_rq_flags(req, "Request Flags");
+		
+		printk("DUMPING REQUEST LIST:\n");
+		rq_for_each_segment(bvec, req, iter) {
+			printk("[%d]: phys 0x%lx len 0x%x\n", i,
+			       (unsigned long)page_to_phys(bvec->bv_page) + bvec->bv_offset,
+			       bvec->bv_len);
+			i++;
+		}
+		printk("DUMPING MAPPED LIST:\n");
+		for_each_sg(sdb->table.sgl, sg, count, i) {
+			printk("[%d]: phys 0x%lx len 0x%x\n", i,
+			       (unsigned long)page_to_phys(sg_page(sg)) + sg->offset,
+			       sg->length);
+		}
+		BUG();
+	}
+			
+
 	sdb->table.nents = count;
 	if (blk_pc_request(req))
 		sdb->length = req->data_len;

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Thomas Gleixner]

On Wed, 4 Mar 2009, Thomas Gleixner wrote:

Instrumented the code and the result of the failing request is
below. Looks like the function which sets up the request gets
nr_phys_segments wrong by one. 

If you need further trace data feel free to ask.

Thanks,

	tglx

bvprv: bvec_to_phys(bvprv)
bvec:  bvec_to_phys(bvec)
max_segment_size is 65536

scsi_init_sgtable: ISG: nr_seg 9 nents 9

blk_rq_map_sg: MSG: nbytes 4096

blk_rq_map_sg: MSG: new segment 1: bvec 0x6a8cd000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8cd000 bvec 0x6a8ce000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ce000 bvec 0x6a8cf000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8cf000 bvec 0x6a8d0000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d0000 bvec 0x6a8d1000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d1000 bvec 0x6a8d2000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d2000 bvec 0x6a8d3000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d3000 bvec 0x6a8d4000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d4000 bvec 0x6a8d5000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d5000 bvec 0x6a8d6000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d6000 bvec 0x6a8d7000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d7000 bvec 0x6a8d8000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d8000 bvec 0x6a8d9000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8d9000 bvec 0x6a8da000 sg->length 53248 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8da000 bvec 0x6a8db000 sg->length 57344 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8db000 bvec 0x6a8dc000 sg->length 61440 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8dc000 bvec 0x6a8dd000 sg->length 65536 

blk_rq_map_sg: MSG: new segment 2: bvec 0x6a8dd000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8dd000 bvec 0x6a8de000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8de000 bvec 0x6a8df000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8df000 bvec 0x6a8e0000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e0000 bvec 0x6a8e1000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e1000 bvec 0x6a8e2000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e2000 bvec 0x6a8e3000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e3000 bvec 0x6a8e4000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e4000 bvec 0x6a8e5000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e5000 bvec 0x6a8e6000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e6000 bvec 0x6a8e7000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e7000 bvec 0x6a8e8000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e8000 bvec 0x6a8e9000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8e9000 bvec 0x6a8ea000 sg->length 53248 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ea000 bvec 0x6a8eb000 sg->length 57344 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8eb000 bvec 0x6a8ec000 sg->length 61440 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ec000 bvec 0x6a8ed000 sg->length 65536 

blk_rq_map_sg: MSG: new segment 3: bvec 0x6a8ed000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ed000 bvec 0x6a8ee000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ee000 bvec 0x6a8ef000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ef000 bvec 0x6a8f0000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f0000 bvec 0x6a8f1000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f1000 bvec 0x6a8f2000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f2000 bvec 0x6a8f3000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f3000 bvec 0x6a8f4000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f4000 bvec 0x6a8f5000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f5000 bvec 0x6a8f6000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f6000 bvec 0x6a8f7000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f7000 bvec 0x6a8f8000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f8000 bvec 0x6a8f9000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8f9000 bvec 0x6a8fa000 sg->length 53248 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8fa000 bvec 0x6a8fb000 sg->length 57344 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8fb000 bvec 0x6a8fc000 sg->length 61440 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8fc000 bvec 0x6a8fd000 sg->length 65536 

blk_rq_map_sg: MSG: new segment 4: bvec 0x6a8fd000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8fd000 bvec 0x6a8fe000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8fe000 bvec 0x6a8ff000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6a8ff000 bvec 0x6ac80000 sg->length 12288 

blk_rq_map_sg: MSG: new segment 5: bvec 0x6ac80000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac80000 bvec 0x6ac81000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac81000 bvec 0x6ac82000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac82000 bvec 0x6ac83000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac83000 bvec 0x6ac84000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac84000 bvec 0x6ac85000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac85000 bvec 0x6ac86000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac86000 bvec 0x6ac87000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac87000 bvec 0x6ac88000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac88000 bvec 0x6ac89000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac89000 bvec 0x6ac8a000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac8a000 bvec 0x6ac8b000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac8b000 bvec 0x6ac8c000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ac8c000 bvec 0x6accd000 sg->length 53248 

blk_rq_map_sg: MSG: new segment 6: bvec 0x6accd000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6accd000 bvec 0x6acce000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acce000 bvec 0x6accf000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6accf000 bvec 0x6acd0000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd0000 bvec 0x6acd1000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd1000 bvec 0x6acd2000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd2000 bvec 0x6acd3000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd3000 bvec 0x6acd4000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd4000 bvec 0x6acd5000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd5000 bvec 0x6acd6000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd6000 bvec 0x6acd7000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd7000 bvec 0x6acd8000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd8000 bvec 0x6acd9000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acd9000 bvec 0x6acda000 sg->length 53248 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acda000 bvec 0x6acdb000 sg->length 57344 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acdb000 bvec 0x6acdc000 sg->length 61440 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acdc000 bvec 0x6acdd000 sg->length 65536 

blk_rq_map_sg: MSG: new segment 7: bvec 0x6acdd000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acdd000 bvec 0x6acde000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acde000 bvec 0x6acdf000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acdf000 bvec 0x6ace0000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace0000 bvec 0x6ace1000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace1000 bvec 0x6ace2000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace2000 bvec 0x6ace3000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace3000 bvec 0x6ace4000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace4000 bvec 0x6ace5000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace5000 bvec 0x6ace6000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace6000 bvec 0x6ace7000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace7000 bvec 0x6ace8000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace8000 bvec 0x6ace9000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6ace9000 bvec 0x6acea000 sg->length 53248 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acea000 bvec 0x6aceb000 sg->length 57344 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6aceb000 bvec 0x6acec000 sg->length 61440 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acec000 bvec 0x6aced000 sg->length 65536 

blk_rq_map_sg: MSG: new segment 8: bvec 0x6aced000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6aced000 bvec 0x6acee000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acee000 bvec 0x6acef000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acef000 bvec 0x6acf0000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf0000 bvec 0x6acf1000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf1000 bvec 0x6acf2000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf2000 bvec 0x6acf3000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf3000 bvec 0x6acf4000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf4000 bvec 0x6acf5000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf5000 bvec 0x6acf6000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf6000 bvec 0x6acf7000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf7000 bvec 0x6acf8000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf8000 bvec 0x6acf9000 sg->length 49152 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acf9000 bvec 0x6acfa000 sg->length 53248 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acfa000 bvec 0x6acfb000 sg->length 57344 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acfb000 bvec 0x6acfc000 sg->length 61440 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acfc000 bvec 0x6acfd000 sg->length 65536 

blk_rq_map_sg: MSG: new segment 9: bvec 0x6acfd000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acfd000 bvec 0x6acfe000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acfe000 bvec 0x6acff000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6acff000 bvec 0x6b080000 sg->length 12288 

blk_rq_map_sg: MSG: new segment 10: bvec 0x6b080000

blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b080000 bvec 0x6b081000 sg->length 4096 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b081000 bvec 0x6b082000 sg->length 8192 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b082000 bvec 0x6b083000 sg->length 12288 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b083000 bvec 0x6b084000 sg->length 16384 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b084000 bvec 0x6b085000 sg->length 20480 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b085000 bvec 0x6b086000 sg->length 24576 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b086000 bvec 0x6b087000 sg->length 28672 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b087000 bvec 0x6b088000 sg->length 32768 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b088000 bvec 0x6b089000 sg->length 36864 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b089000 bvec 0x6b08a000 sg->length 40960 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b08a000 bvec 0x6b08b000 sg->length 45056 
blk_rq_map_sg: MSG: nbytes 4096
blk_rq_map_sg: MSG: bvprv 0x6b08b000 bvec 0x6b08c000 sg->length 49152 

scsi_init_sgtable: ISG: count 10

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[James Bottomley]

On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> 
> Instrumented the code and the result of the failing request is
> below. Looks like the function which sets up the request gets
> nr_phys_segments wrong by one. 
> 
> If you need further trace data feel free to ask.

OK, the mapping all checks out correctly ... there must be something
wrong with the way we count before mapping.

If you're tracing everything, could you add these static prints to the
trace ... they'll trigger a lot, but capturing how they applied to the
failing request might tell us why the count is wrong.

Thanks,

James

---

diff --git a/block/blk-merge.c b/block/blk-merge.c
index a104593..a529cba 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -127,21 +127,29 @@ static int blk_phys_contig_segment(struct request_queue *q, struct bio *bio,
 		return 0;
 
 	if (bio->bi_seg_back_size + nxt->bi_seg_front_size >
-	    q->max_segment_size)
+	    q->max_segment_size) {
+		printk("Refusing contig merge, over segment size\n");
 		return 0;
+	}
 
-	if (!bio_has_data(bio))
+	if (!bio_has_data(bio)) {
+		printk("Allowing contig merge, bio has no data\n");
 		return 1;
+	}
 
-	if (!BIOVEC_PHYS_MERGEABLE(__BVEC_END(bio), __BVEC_START(nxt)))
+	if (!BIOVEC_PHYS_MERGEABLE(__BVEC_END(bio), __BVEC_START(nxt))) {
+		printk("Refusing contig merge, bio not phys mergeable\n");
 		return 0;
+	}
 
 	/*
 	 * bio and nxt are contiguous in memory; check if the queue allows
 	 * these two to be merged into one
 	 */
-	if (BIO_SEG_BOUNDARY(q, bio, nxt))
+	if (BIO_SEG_BOUNDARY(q, bio, nxt)) {
+		printk("Allowing contig merge, not across segment boundary\n");
 		return 1;
+	}
 
 	return 0;
 }
@@ -325,6 +333,12 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
 	if ((req->nr_sectors + next->nr_sectors) > q->max_sectors)
 		return 0;
 
+	printk("Merging end %lx (segs %d) with beginning %lx (segs %d)\n",
+	       bvec_to_phys(__BVEC_END(req->biotail)), req->nr_phys_segments,
+	       bvec_to_phys(__BVEC_START(next->bio)), next->nr_phys_segments);
+	printk("Front size is %d, back size is %d\n",
+	       next->bio->bi_seg_front_size, req->biotail->bi_seg_back_size);
+
 	total_phys_segments = req->nr_phys_segments + next->nr_phys_segments;
 	if (blk_phys_contig_segment(q, req->biotail, next->bio)) {
 		if (req->nr_phys_segments == 1)

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[James Bottomley]

On Wed, 2009-03-04 at 22:56 +0000, James Bottomley wrote:
> On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > 
> > Instrumented the code and the result of the failing request is
> > below. Looks like the function which sets up the request gets
> > nr_phys_segments wrong by one. 
> > 
> > If you need further trace data feel free to ask.
> 
> OK, the mapping all checks out correctly ... there must be something
> wrong with the way we count before mapping.
> 
> If you're tracing everything, could you add these static prints to the
> trace ... they'll trigger a lot, but capturing how they applied to the
> failing request might tell us why the count is wrong.

Debugging this on IRC, this is the point we reached:

ftrace debugging patch:

http://tglx.de/~tglx/dbg.patch

We're tracing both blk_recalc_rq_segments() and
blk_phys_contig_segment()

The results are here:

http://tglx.de/~tglx/t.txt.bz2

Although what they show is that we're missing the point where the
counting goes wrong (blk_recalc_rq_segments only goes up to 5 max).

James

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[FUJITA Tomonori]

CC'ed Jens,

On Wed, 04 Mar 2009 22:56:29 +0000
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > 
> > Instrumented the code and the result of the failing request is
> > below. Looks like the function which sets up the request gets
> > nr_phys_segments wrong by one. 
> > 
> > If you need further trace data feel free to ask.
> 
> OK, the mapping all checks out correctly ... there must be something
> wrong with the way we count before mapping.

Yeah, looks we miscalculate nr_phys_segments in the merging path.

blk_recount_segments() needs to set bi_seg_front_size and
bi_seg_back_size for ll_merge_requests_fn()?

=
diff --git a/block/blk-merge.c b/block/blk-merge.c
index a104593..efb65b6 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
 
 void blk_recount_segments(struct request_queue *q, struct bio *bio)
 {
+	unsigned int seg_size;
 	struct bio *nxt = bio->bi_next;
 
 	bio->bi_next = NULL;
-	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
+	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
 	bio->bi_next = nxt;
 	bio->bi_flags |= (1 << BIO_SEG_VALID);
+
+	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
+		bio->bi_seg_front_size = seg_size;
+	if (bio->bi_phys_segments > bio->bi_seg_back_size)
+		bio->bi_seg_back_size = seg_size;
+
 }
 EXPORT_SYMBOL(blk_recount_segments);
 

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[FUJITA Tomonori]

On Thu, 5 Mar 2009 17:36:13 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> CC'ed Jens,
> 
> On Wed, 04 Mar 2009 22:56:29 +0000
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > 
> > > Instrumented the code and the result of the failing request is
> > > below. Looks like the function which sets up the request gets
> > > nr_phys_segments wrong by one. 
> > > 
> > > If you need further trace data feel free to ask.
> > 
> > OK, the mapping all checks out correctly ... there must be something
> > wrong with the way we count before mapping.
> 
> Yeah, looks we miscalculate nr_phys_segments in the merging path.
> 
> blk_recount_segments() needs to set bi_seg_front_size and
> bi_seg_back_size for ll_merge_requests_fn()?
> 
> =
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index a104593..efb65b6 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
>  
>  void blk_recount_segments(struct request_queue *q, struct bio *bio)
>  {
> +	unsigned int seg_size;
>  	struct bio *nxt = bio->bi_next;
>  
>  	bio->bi_next = NULL;
> -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
>  	bio->bi_next = nxt;
>  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> +
> +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> +		bio->bi_seg_front_size = seg_size;
> +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> +		bio->bi_seg_back_size = seg_size;
> +
>  }
>  EXPORT_SYMBOL(blk_recount_segments);

Duh, here's the proper patch.

diff --git a/block/blk-merge.c b/block/blk-merge.c
index a104593..06e0db4 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
 
 void blk_recount_segments(struct request_queue *q, struct bio *bio)
 {
+	unsigned int seg_size;
 	struct bio *nxt = bio->bi_next;
 
 	bio->bi_next = NULL;
-	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
+	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
 	bio->bi_next = nxt;
 	bio->bi_flags |= (1 << BIO_SEG_VALID);
+
+	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
+		bio->bi_seg_front_size = seg_size;
+	if (seg_size > bio->bi_seg_back_size)
+		bio->bi_seg_back_size = seg_size;
+
 }
 EXPORT_SYMBOL(blk_recount_segments);
 

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[FUJITA Tomonori]

Oops, somehow I forgot to CC Jens...

On Thu, 5 Mar 2009 17:39:17 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Thu, 5 Mar 2009 17:36:13 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > CC'ed Jens,
> > 
> > On Wed, 04 Mar 2009 22:56:29 +0000
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > > 
> > > > Instrumented the code and the result of the failing request is
> > > > below. Looks like the function which sets up the request gets
> > > > nr_phys_segments wrong by one. 
> > > > 
> > > > If you need further trace data feel free to ask.
> > > 
> > > OK, the mapping all checks out correctly ... there must be something
> > > wrong with the way we count before mapping.
> > 
> > Yeah, looks we miscalculate nr_phys_segments in the merging path.
> > 
> > blk_recount_segments() needs to set bi_seg_front_size and
> > bi_seg_back_size for ll_merge_requests_fn()?
> > 
> > =
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index a104593..efb65b6 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> >  
> >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> >  {
> > +	unsigned int seg_size;
> >  	struct bio *nxt = bio->bi_next;
> >  
> >  	bio->bi_next = NULL;
> > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> >  	bio->bi_next = nxt;
> >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > +
> > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > +		bio->bi_seg_front_size = seg_size;
> > +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> > +		bio->bi_seg_back_size = seg_size;
> > +
> >  }
> >  EXPORT_SYMBOL(blk_recount_segments);
> 
> Duh, here's the proper patch.
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index a104593..06e0db4 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
>  
>  void blk_recount_segments(struct request_queue *q, struct bio *bio)
>  {
> +	unsigned int seg_size;
>  	struct bio *nxt = bio->bi_next;
>  
>  	bio->bi_next = NULL;
> -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
>  	bio->bi_next = nxt;
>  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> +
> +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> +		bio->bi_seg_front_size = seg_size;
> +	if (seg_size > bio->bi_seg_back_size)
> +		bio->bi_seg_back_size = seg_size;
> +
>  }
>  EXPORT_SYMBOL(blk_recount_segments);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Jens Axboe]

On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> Oops, somehow I forgot to CC Jens...
> 
> On Thu, 5 Mar 2009 17:39:17 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > On Thu, 5 Mar 2009 17:36:13 +0900
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > 
> > > CC'ed Jens,
> > > 
> > > On Wed, 04 Mar 2009 22:56:29 +0000
> > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > 
> > > > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > > > 
> > > > > Instrumented the code and the result of the failing request is
> > > > > below. Looks like the function which sets up the request gets
> > > > > nr_phys_segments wrong by one. 
> > > > > 
> > > > > If you need further trace data feel free to ask.
> > > > 
> > > > OK, the mapping all checks out correctly ... there must be something
> > > > wrong with the way we count before mapping.
> > > 
> > > Yeah, looks we miscalculate nr_phys_segments in the merging path.
> > > 
> > > blk_recount_segments() needs to set bi_seg_front_size and
> > > bi_seg_back_size for ll_merge_requests_fn()?
> > > 
> > > =
> > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > index a104593..efb65b6 100644
> > > --- a/block/blk-merge.c
> > > +++ b/block/blk-merge.c
> > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > >  
> > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > >  {
> > > +	unsigned int seg_size;
> > >  	struct bio *nxt = bio->bi_next;
> > >  
> > >  	bio->bi_next = NULL;
> > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > >  	bio->bi_next = nxt;
> > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > +
> > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > +		bio->bi_seg_front_size = seg_size;
> > > +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> > > +		bio->bi_seg_back_size = seg_size;
> > > +
> > >  }
> > >  EXPORT_SYMBOL(blk_recount_segments);
> > 
> > Duh, here's the proper patch.
> > 
> > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > index a104593..06e0db4 100644
> > --- a/block/blk-merge.c
> > +++ b/block/blk-merge.c
> > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> >  
> >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> >  {
> > +	unsigned int seg_size;
> >  	struct bio *nxt = bio->bi_next;
> >  
> >  	bio->bi_next = NULL;
> > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> >  	bio->bi_next = nxt;
> >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > +
> > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > +		bio->bi_seg_front_size = seg_size;
> > +	if (seg_size > bio->bi_seg_back_size)
> > +		bio->bi_seg_back_size = seg_size;
> > +
> >  }
> >  EXPORT_SYMBOL(blk_recount_segments);

Good catch, I merged it with a slight change of layout and clearing
seg_size initially, to avoid gcc silly errors.

-- 
Jens Axboe

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Jens Axboe]

On Thu, Mar 05 2009, Jens Axboe wrote:
> On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> > Oops, somehow I forgot to CC Jens...
> > 
> > On Thu, 5 Mar 2009 17:39:17 +0900
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > 
> > > On Thu, 5 Mar 2009 17:36:13 +0900
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > CC'ed Jens,
> > > > 
> > > > On Wed, 04 Mar 2009 22:56:29 +0000
> > > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > > 
> > > > > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > > > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > > > > 
> > > > > > Instrumented the code and the result of the failing request is
> > > > > > below. Looks like the function which sets up the request gets
> > > > > > nr_phys_segments wrong by one. 
> > > > > > 
> > > > > > If you need further trace data feel free to ask.
> > > > > 
> > > > > OK, the mapping all checks out correctly ... there must be something
> > > > > wrong with the way we count before mapping.
> > > > 
> > > > Yeah, looks we miscalculate nr_phys_segments in the merging path.
> > > > 
> > > > blk_recount_segments() needs to set bi_seg_front_size and
> > > > bi_seg_back_size for ll_merge_requests_fn()?
> > > > 
> > > > =
> > > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > > index a104593..efb65b6 100644
> > > > --- a/block/blk-merge.c
> > > > +++ b/block/blk-merge.c
> > > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > > >  
> > > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > > >  {
> > > > +	unsigned int seg_size;
> > > >  	struct bio *nxt = bio->bi_next;
> > > >  
> > > >  	bio->bi_next = NULL;
> > > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > > >  	bio->bi_next = nxt;
> > > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > > +
> > > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > > +		bio->bi_seg_front_size = seg_size;
> > > > +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> > > > +		bio->bi_seg_back_size = seg_size;
> > > > +
> > > >  }
> > > >  EXPORT_SYMBOL(blk_recount_segments);
> > > 
> > > Duh, here's the proper patch.
> > > 
> > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > index a104593..06e0db4 100644
> > > --- a/block/blk-merge.c
> > > +++ b/block/blk-merge.c
> > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > >  
> > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > >  {
> > > +	unsigned int seg_size;
> > >  	struct bio *nxt = bio->bi_next;
> > >  
> > >  	bio->bi_next = NULL;
> > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > >  	bio->bi_next = nxt;
> > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > +
> > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > +		bio->bi_seg_front_size = seg_size;
> > > +	if (seg_size > bio->bi_seg_back_size)
> > > +		bio->bi_seg_back_size = seg_size;
> > > +
> > >  }
> > >  EXPORT_SYMBOL(blk_recount_segments);
> 
> Good catch, I merged it with a slight change of layout and clearing
> seg_size initially, to avoid gcc silly errors.

While merging that, I think we can do better than this. Essentially we
just need to have __blk_recalc_rq_segments() track the back bio as well,
then we don't have to pass in a pointer for segment sizes.

Totally untested, comments welcome...

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 6be3797..5a244f0 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -39,14 +39,13 @@ void blk_recalc_rq_sectors(struct request *rq, int nsect)
 }
 
 static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
-					     struct bio *bio,
-					     unsigned int *seg_size_ptr)
+					     struct bio *bio)
 {
 	unsigned int phys_size;
 	struct bio_vec *bv, *bvprv = NULL;
 	int cluster, i, high, highprv = 1;
 	unsigned int seg_size, nr_phys_segs;
-	struct bio *fbio;
+	struct bio *fbio, *bbio;
 
 	if (!bio)
 		return 0;
@@ -87,41 +86,28 @@ new_segment:
 			seg_size = bv->bv_len;
 			highprv = high;
 		}
+		bbio = bio;
 	}
 
-	if (seg_size_ptr)
-		*seg_size_ptr = seg_size;
+	if (nr_phys_segs == 1 && seg_size > fbio->bi_seg_front_size)
+		fbio->bi_seg_front_size = seg_size;
+	if (seg_size > bbio->bi_seg_back_size)
+		bbio->bi_seg_back_size = seg_size;
 
 	return nr_phys_segs;
 }
 
 void blk_recalc_rq_segments(struct request *rq)
 {
-	unsigned int seg_size = 0, phys_segs;
-
-	phys_segs = __blk_recalc_rq_segments(rq->q, rq->bio, &seg_size);
-
-	if (phys_segs == 1 && seg_size > rq->bio->bi_seg_front_size)
-		rq->bio->bi_seg_front_size = seg_size;
-	if (seg_size > rq->biotail->bi_seg_back_size)
-		rq->biotail->bi_seg_back_size = seg_size;
-
-	rq->nr_phys_segments = phys_segs;
+	rq->nr_phys_segments = __blk_recalc_rq_segments(rq->q, rq->bio);
 }
 
 void blk_recount_segments(struct request_queue *q, struct bio *bio)
 {
 	struct bio *nxt = bio->bi_next;
-	unsigned int seg_size = 0;
 
 	bio->bi_next = NULL;
-	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
-
-	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
-		bio->bi_seg_front_size = seg_size;
-	if (seg_size > bio->bi_seg_back_size)
-		bio->bi_seg_back_size = seg_size;
-
+	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio);
 	bio->bi_next = nxt;
 	bio->bi_flags |= (1 << BIO_SEG_VALID);
 }

-- 
Jens Axboe

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[FUJITA Tomonori]

On Thu, 5 Mar 2009 11:14:36 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Mar 05 2009, Jens Axboe wrote:
> > On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> > > Oops, somehow I forgot to CC Jens...
> > > 
> > > On Thu, 5 Mar 2009 17:39:17 +0900
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > On Thu, 5 Mar 2009 17:36:13 +0900
> > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > 
> > > > > CC'ed Jens,
> > > > > 
> > > > > On Wed, 04 Mar 2009 22:56:29 +0000
> > > > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > > > 
> > > > > > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > > > > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > > > > > 
> > > > > > > Instrumented the code and the result of the failing request is
> > > > > > > below. Looks like the function which sets up the request gets
> > > > > > > nr_phys_segments wrong by one. 
> > > > > > > 
> > > > > > > If you need further trace data feel free to ask.
> > > > > > 
> > > > > > OK, the mapping all checks out correctly ... there must be something
> > > > > > wrong with the way we count before mapping.
> > > > > 
> > > > > Yeah, looks we miscalculate nr_phys_segments in the merging path.
> > > > > 
> > > > > blk_recount_segments() needs to set bi_seg_front_size and
> > > > > bi_seg_back_size for ll_merge_requests_fn()?
> > > > > 
> > > > > =
> > > > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > > > index a104593..efb65b6 100644
> > > > > --- a/block/blk-merge.c
> > > > > +++ b/block/blk-merge.c
> > > > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > > > >  
> > > > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > > > >  {
> > > > > +	unsigned int seg_size;
> > > > >  	struct bio *nxt = bio->bi_next;
> > > > >  
> > > > >  	bio->bi_next = NULL;
> > > > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > > > >  	bio->bi_next = nxt;
> > > > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > > > +
> > > > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > > > +		bio->bi_seg_front_size = seg_size;
> > > > > +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> > > > > +		bio->bi_seg_back_size = seg_size;
> > > > > +
> > > > >  }
> > > > >  EXPORT_SYMBOL(blk_recount_segments);
> > > > 
> > > > Duh, here's the proper patch.
> > > > 
> > > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > > index a104593..06e0db4 100644
> > > > --- a/block/blk-merge.c
> > > > +++ b/block/blk-merge.c
> > > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > > >  
> > > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > > >  {
> > > > +	unsigned int seg_size;
> > > >  	struct bio *nxt = bio->bi_next;
> > > >  
> > > >  	bio->bi_next = NULL;
> > > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > > >  	bio->bi_next = nxt;
> > > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > > +
> > > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > > +		bio->bi_seg_front_size = seg_size;
> > > > +	if (seg_size > bio->bi_seg_back_size)
> > > > +		bio->bi_seg_back_size = seg_size;
> > > > +
> > > >  }
> > > >  EXPORT_SYMBOL(blk_recount_segments);
> > 
> > Good catch, I merged it with a slight change of layout and clearing
> > seg_size initially, to avoid gcc silly errors.
> 
> While merging that, I think we can do better than this. Essentially we
> just need to have __blk_recalc_rq_segments() track the back bio as well,
> then we don't have to pass in a pointer for segment sizes.
> 
> Totally untested, comments welcome...

Yeah, I think that updating bi_seg_front_size and bi_seg_back_size at
one place, __blk_recalc_rq_segments, is better. I thought about the
same way. But we are already in -rc7 and this must go into mainline
now. So I chose a less-intrusive way (similar to what we have done in
the past).

As you know, the merging code is really complicated and we could
overlook stuff easily. ;) It might be better to simplify the merging
code a bit.

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Jens Axboe]

On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> On Thu, 5 Mar 2009 11:14:36 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Thu, Mar 05 2009, Jens Axboe wrote:
> > > On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> > > > Oops, somehow I forgot to CC Jens...
> > > > 
> > > > On Thu, 5 Mar 2009 17:39:17 +0900
> > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > 
> > > > > On Thu, 5 Mar 2009 17:36:13 +0900
> > > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > > 
> > > > > > CC'ed Jens,
> > > > > > 
> > > > > > On Wed, 04 Mar 2009 22:56:29 +0000
> > > > > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > > > > 
> > > > > > > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > > > > > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > > > > > > 
> > > > > > > > Instrumented the code and the result of the failing request is
> > > > > > > > below. Looks like the function which sets up the request gets
> > > > > > > > nr_phys_segments wrong by one. 
> > > > > > > > 
> > > > > > > > If you need further trace data feel free to ask.
> > > > > > > 
> > > > > > > OK, the mapping all checks out correctly ... there must be something
> > > > > > > wrong with the way we count before mapping.
> > > > > > 
> > > > > > Yeah, looks we miscalculate nr_phys_segments in the merging path.
> > > > > > 
> > > > > > blk_recount_segments() needs to set bi_seg_front_size and
> > > > > > bi_seg_back_size for ll_merge_requests_fn()?
> > > > > > 
> > > > > > =
> > > > > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > > > > index a104593..efb65b6 100644
> > > > > > --- a/block/blk-merge.c
> > > > > > +++ b/block/blk-merge.c
> > > > > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > > > > >  
> > > > > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > > > > >  {
> > > > > > +	unsigned int seg_size;
> > > > > >  	struct bio *nxt = bio->bi_next;
> > > > > >  
> > > > > >  	bio->bi_next = NULL;
> > > > > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > > > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > > > > >  	bio->bi_next = nxt;
> > > > > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > > > > +
> > > > > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > > > > +		bio->bi_seg_front_size = seg_size;
> > > > > > +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> > > > > > +		bio->bi_seg_back_size = seg_size;
> > > > > > +
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(blk_recount_segments);
> > > > > 
> > > > > Duh, here's the proper patch.
> > > > > 
> > > > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > > > index a104593..06e0db4 100644
> > > > > --- a/block/blk-merge.c
> > > > > +++ b/block/blk-merge.c
> > > > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > > > >  
> > > > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > > > >  {
> > > > > +	unsigned int seg_size;
> > > > >  	struct bio *nxt = bio->bi_next;
> > > > >  
> > > > >  	bio->bi_next = NULL;
> > > > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > > > >  	bio->bi_next = nxt;
> > > > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > > > +
> > > > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > > > +		bio->bi_seg_front_size = seg_size;
> > > > > +	if (seg_size > bio->bi_seg_back_size)
> > > > > +		bio->bi_seg_back_size = seg_size;
> > > > > +
> > > > >  }
> > > > >  EXPORT_SYMBOL(blk_recount_segments);
> > > 
> > > Good catch, I merged it with a slight change of layout and clearing
> > > seg_size initially, to avoid gcc silly errors.
> > 
> > While merging that, I think we can do better than this. Essentially we
> > just need to have __blk_recalc_rq_segments() track the back bio as well,
> > then we don't have to pass in a pointer for segment sizes.
> > 
> > Totally untested, comments welcome...
> 
> Yeah, I think that updating bi_seg_front_size and bi_seg_back_size at
> one place, __blk_recalc_rq_segments, is better. I thought about the
> same way. But we are already in -rc7 and this must go into mainline
> now. So I chose a less-intrusive way (similar to what we have done in
> the past).
> 
> As you know, the merging code is really complicated and we could
> overlook stuff easily. ;) It might be better to simplify the merging
> code a bit.

If someone (Ingo?) is willing to test the last variant, I'd much rather
add that. It does simplify it (imho), and it kills 23 lines while only
adding 9. But a quick response would be nice, then I can ask Linus to
pull it later today.

-- 
Jens Axboe

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[FUJITA Tomonori]

On Thu, 5 Mar 2009 11:30:24 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> > > While merging that, I think we can do better than this. Essentially we
> > > just need to have __blk_recalc_rq_segments() track the back bio as well,
> > > then we don't have to pass in a pointer for segment sizes.
> > > 
> > > Totally untested, comments welcome...
> > 
> > Yeah, I think that updating bi_seg_front_size and bi_seg_back_size at
> > one place, __blk_recalc_rq_segments, is better. I thought about the
> > same way. But we are already in -rc7 and this must go into mainline
> > now. So I chose a less-intrusive way (similar to what we have done in
> > the past).
> > 
> > As you know, the merging code is really complicated and we could
> > overlook stuff easily. ;) It might be better to simplify the merging
> > code a bit.
> 
> If someone (Ingo?) is willing to test the last variant, I'd much rather
> add that. It does simplify it (imho), and it kills 23 lines while only
> adding 9. But a quick response would be nice, then I can ask Linus to
> pull it later today.

I prefer to keep your change for 2.6.30 but if you want to push it
now, it's fine by me.

Ingo, you can quickly hit this bug without the patch?

I've not hit this bug while I've been performing intensive I/Os for
the last three hours. And I thought that Thomas took two hours to hit
this. So maybe it's too early to give 'Tested-by'. With 
max_segment_size decreased, we might hit this easier.

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Jens Axboe]

On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> On Thu, 5 Mar 2009 11:30:24 +0100
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > > > While merging that, I think we can do better than this. Essentially we
> > > > just need to have __blk_recalc_rq_segments() track the back bio as well,
> > > > then we don't have to pass in a pointer for segment sizes.
> > > > 
> > > > Totally untested, comments welcome...
> > > 
> > > Yeah, I think that updating bi_seg_front_size and bi_seg_back_size at
> > > one place, __blk_recalc_rq_segments, is better. I thought about the
> > > same way. But we are already in -rc7 and this must go into mainline
> > > now. So I chose a less-intrusive way (similar to what we have done in
> > > the past).
> > > 
> > > As you know, the merging code is really complicated and we could
> > > overlook stuff easily. ;) It might be better to simplify the merging
> > > code a bit.
> > 
> > If someone (Ingo?) is willing to test the last variant, I'd much rather
> > add that. It does simplify it (imho), and it kills 23 lines while only
> > adding 9. But a quick response would be nice, then I can ask Linus to
> > pull it later today.
> 
> I prefer to keep your change for 2.6.30 but if you want to push it
> now, it's fine by me.

I honestly can't see much of a difference in change complexity, so I
don't see much point in putting one fix in 2.6.29 and then doing another
for 2.6.30...

> Ingo, you can quickly hit this bug without the patch?
> 
> I've not hit this bug while I've been performing intensive I/Os for
> the last three hours. And I thought that Thomas took two hours to hit
> this. So maybe it's too early to give 'Tested-by'. With 
> max_segment_size decreased, we might hit this easier.

Yep, that may help. I haven't seen this thread until I was cc'ed on it,
so I haven't even read up on the generic problem yet...

-- 
Jens Axboe

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[FUJITA Tomonori]

On Thu, 5 Mar 2009 12:10:40 +0100
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> > On Thu, 5 Mar 2009 11:30:24 +0100
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > > > While merging that, I think we can do better than this. Essentially we
> > > > > just need to have __blk_recalc_rq_segments() track the back bio as well,
> > > > > then we don't have to pass in a pointer for segment sizes.
> > > > > 
> > > > > Totally untested, comments welcome...
> > > > 
> > > > Yeah, I think that updating bi_seg_front_size and bi_seg_back_size at
> > > > one place, __blk_recalc_rq_segments, is better. I thought about the
> > > > same way. But we are already in -rc7 and this must go into mainline
> > > > now. So I chose a less-intrusive way (similar to what we have done in
> > > > the past).
> > > > 
> > > > As you know, the merging code is really complicated and we could
> > > > overlook stuff easily. ;) It might be better to simplify the merging
> > > > code a bit.
> > > 
> > > If someone (Ingo?) is willing to test the last variant, I'd much rather
> > > add that. It does simplify it (imho), and it kills 23 lines while only
> > > adding 9. But a quick response would be nice, then I can ask Linus to
> > > pull it later today.
> > 
> > I prefer to keep your change for 2.6.30 but if you want to push it
> > now, it's fine by me.
> 
> I honestly can't see much of a difference in change complexity, so I
> don't see much point in putting one fix in 2.6.29 and then doing another
> for 2.6.30...

My preference are:

1) simply reverting commit 1e42807918d17e8c93bf14fbb74be84b141334c1
(and blaming ext4 for now).

2) applying my patch, affecting only blk_recount_segments().

3) applying your patch, affecting blk_recount_segments() and
blk_recalc_rq_segments().


But as I said, the third options is fine by me. Your patch looks ok to
me.


> > Ingo, you can quickly hit this bug without the patch?
> > 
> > I've not hit this bug while I've been performing intensive I/Os for
> > the last three hours. And I thought that Thomas took two hours to hit
> > this. So maybe it's too early to give 'Tested-by'. With 
> > max_segment_size decreased, we might hit this easier.
> 
> Yep, that may help. I haven't seen this thread until I was cc'ed on it,
> so I haven't even read up on the generic problem yet...

I think that last night James and Thomas finally found that the block
layer miscalculates nr_phys_segments. And then I figured out exactly
where the bug is, I guess hopefully.

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Ingo Molnar]

* Jens Axboe <jens.axboe@oracle.com> wrote:

> > > Totally untested, comments welcome...
> > 
> > Yeah, I think that updating bi_seg_front_size and 
> > bi_seg_back_size at one place, __blk_recalc_rq_segments, is 
> > better. I thought about the same way. But we are already in 
> > -rc7 and this must go into mainline now. So I chose a 
> > less-intrusive way (similar to what we have done in the 
> > past).
> > 
> > As you know, the merging code is really complicated and we 
> > could overlook stuff easily. ;) It might be better to 
> > simplify the merging code a bit.
> 
> If someone (Ingo?) is willing to test the last variant, I'd 
> much rather add that. It does simplify it (imho), and it kills 
> 23 lines while only adding 9. But a quick response would be 
> nice, then I can ask Linus to pull it later today.

sure, can give it a whirl.

Note that your patch in this thread does no apply cleanly. Could 
you please send a pull request of your latest fixes that i could 
pull into tip:out-of-tree for testing purposes?

	Ingo

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Jens Axboe]

On Thu, Mar 05 2009, Ingo Molnar wrote:
> 
> * Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > > > Totally untested, comments welcome...
> > > 
> > > Yeah, I think that updating bi_seg_front_size and 
> > > bi_seg_back_size at one place, __blk_recalc_rq_segments, is 
> > > better. I thought about the same way. But we are already in 
> > > -rc7 and this must go into mainline now. So I chose a 
> > > less-intrusive way (similar to what we have done in the 
> > > past).
> > > 
> > > As you know, the merging code is really complicated and we 
> > > could overlook stuff easily. ;) It might be better to 
> > > simplify the merging code a bit.
> > 
> > If someone (Ingo?) is willing to test the last variant, I'd 
> > much rather add that. It does simplify it (imho), and it kills 
> > 23 lines while only adding 9. But a quick response would be 
> > nice, then I can ask Linus to pull it later today.
> 
> sure, can give it a whirl.
> 
> Note that your patch in this thread does no apply cleanly. Could 
> you please send a pull request of your latest fixes that i could 
> pull into tip:out-of-tree for testing purposes?

Hmm that's odd, I have no changes in blk-merge.c in my tree against
Linus'. But you can pull:

  git://git.kernel.dk/linux-2.6-block.git for-linus

Jens Axboe (2):
      cciss: remove 30 second initial timeout on controller reset
      block: fix missing bio back/front segment size setting in blk_recount_segments()

Kris Shannon (1):
      Fix kernel NULL pointer dereference in xen-blkfront

Roel Kluin (1):
      loop: don't increment p->offset with (size_t) -EINVAL

 block/blk-merge.c            |   25 +++++++++----------------
 drivers/block/cciss.c        |    8 +++-----
 drivers/block/loop.c         |    3 +--
 drivers/block/xen-blkfront.c |    2 ++
 4 files changed, 15 insertions(+), 23 deletions(-)

-- 
Jens Axboe

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Ingo Molnar]

* Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Mar 05 2009, Ingo Molnar wrote:
> > 
> > * Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > > > Totally untested, comments welcome...
> > > > 
> > > > Yeah, I think that updating bi_seg_front_size and 
> > > > bi_seg_back_size at one place, __blk_recalc_rq_segments, is 
> > > > better. I thought about the same way. But we are already in 
> > > > -rc7 and this must go into mainline now. So I chose a 
> > > > less-intrusive way (similar to what we have done in the 
> > > > past).
> > > > 
> > > > As you know, the merging code is really complicated and we 
> > > > could overlook stuff easily. ;) It might be better to 
> > > > simplify the merging code a bit.
> > > 
> > > If someone (Ingo?) is willing to test the last variant, I'd 
> > > much rather add that. It does simplify it (imho), and it kills 
> > > 23 lines while only adding 9. But a quick response would be 
> > > nice, then I can ask Linus to pull it later today.
> > 
> > sure, can give it a whirl.
> > 
> > Note that your patch in this thread does no apply cleanly. Could 
> > you please send a pull request of your latest fixes that i could 
> > pull into tip:out-of-tree for testing purposes?
> 
> Hmm that's odd, I have no changes in blk-merge.c in my tree 
> against Linus'. But you can pull:
> 
>   git://git.kernel.dk/linux-2.6-block.git for-linus
> 
> Jens Axboe (2):
>       cciss: remove 30 second initial timeout on controller reset
>       block: fix missing bio back/front segment size setting in blk_recount_segments()
> 
> Kris Shannon (1):
>       Fix kernel NULL pointer dereference in xen-blkfront
> 
> Roel Kluin (1):
>       loop: don't increment p->offset with (size_t) -EINVAL
> 
>  block/blk-merge.c            |   25 +++++++++----------------
>  drivers/block/cciss.c        |    8 +++-----
>  drivers/block/loop.c         |    3 +--
>  drivers/block/xen-blkfront.c |    2 ++
>  4 files changed, 15 insertions(+), 23 deletions(-)

It pulled without conflicts so we are good. Started testing it. 
Once Thomas is around i suspect he'll be able to test it too 
with his reproducer.

	Ingo

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Thomas Gleixner]

Jens,

On Thu, 5 Mar 2009, Ingo Molnar wrote:
> * Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Thu, Mar 05 2009, Ingo Molnar wrote:
> It pulled without conflicts so we are good. Started testing it. 
> Once Thomas is around i suspect he'll be able to test it too 
> with his reproducer.

I started the tests on the reproducer machine and the first result
looks good so far. I let the tests run in a loop to make sure that it
fixes the problem for real. Will report later today what the long run
outcome is.

Thanks,

	tglx

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Thomas Gleixner]

On Thu, 5 Mar 2009, Thomas Gleixner wrote:
> Jens,
> 
> On Thu, 5 Mar 2009, Ingo Molnar wrote:
> > * Jens Axboe <jens.axboe@oracle.com> wrote:
> > > On Thu, Mar 05 2009, Ingo Molnar wrote:
> > It pulled without conflicts so we are good. Started testing it. 
> > Once Thomas is around i suspect he'll be able to test it too 
> > with his reproducer.
> 
> I started the tests on the reproducer machine and the first result
> looks good so far. I let the tests run in a loop to make sure that it
> fixes the problem for real. Will report later today what the long run
> outcome is.

Still up and running.

Tested-by: Thomas Gleixner <tglx@linutronix.de>

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Thu, Mar 05 2009, Ingo Molnar wrote:
> > > 
> > > * Jens Axboe <jens.axboe@oracle.com> wrote:
> > > 
> > > > > > Totally untested, comments welcome...
> > > > > 
> > > > > Yeah, I think that updating bi_seg_front_size and 
> > > > > bi_seg_back_size at one place, __blk_recalc_rq_segments, is 
> > > > > better. I thought about the same way. But we are already in 
> > > > > -rc7 and this must go into mainline now. So I chose a 
> > > > > less-intrusive way (similar to what we have done in the 
> > > > > past).
> > > > > 
> > > > > As you know, the merging code is really complicated and we 
> > > > > could overlook stuff easily. ;) It might be better to 
> > > > > simplify the merging code a bit.
> > > > 
> > > > If someone (Ingo?) is willing to test the last variant, I'd 
> > > > much rather add that. It does simplify it (imho), and it kills 
> > > > 23 lines while only adding 9. But a quick response would be 
> > > > nice, then I can ask Linus to pull it later today.
> > > 
> > > sure, can give it a whirl.
> > > 
> > > Note that your patch in this thread does no apply cleanly. Could 
> > > you please send a pull request of your latest fixes that i could 
> > > pull into tip:out-of-tree for testing purposes?
> > 
> > Hmm that's odd, I have no changes in blk-merge.c in my tree 
> > against Linus'. But you can pull:
> > 
> >   git://git.kernel.dk/linux-2.6-block.git for-linus
> > 
> > Jens Axboe (2):
> >       cciss: remove 30 second initial timeout on controller reset
> >       block: fix missing bio back/front segment size setting in blk_recount_segments()
> > 
> > Kris Shannon (1):
> >       Fix kernel NULL pointer dereference in xen-blkfront
> > 
> > Roel Kluin (1):
> >       loop: don't increment p->offset with (size_t) -EINVAL
> > 
> >  block/blk-merge.c            |   25 +++++++++----------------
> >  drivers/block/cciss.c        |    8 +++-----
> >  drivers/block/loop.c         |    3 +--
> >  drivers/block/xen-blkfront.c |    2 ++
> >  4 files changed, 15 insertions(+), 23 deletions(-)
> 
> It pulled without conflicts so we are good. Started testing 
> it. Once Thomas is around i suspect he'll be able to test it 
> too with his reproducer.

FYI, here the patches didnt cause any problems.

Tested-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [BUG] 2.6.29-rc6-2450cf in scsi_lib.c (was: Large amount of scsi-sgpool)objects

[Ingo Molnar]

* Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Mar 05 2009, FUJITA Tomonori wrote:
> > Oops, somehow I forgot to CC Jens...
> > 
> > On Thu, 5 Mar 2009 17:39:17 +0900
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > 
> > > On Thu, 5 Mar 2009 17:36:13 +0900
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > CC'ed Jens,
> > > > 
> > > > On Wed, 04 Mar 2009 22:56:29 +0000
> > > > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > > > 
> > > > > On Wed, 2009-03-04 at 22:45 +0100, Thomas Gleixner wrote:
> > > > > > On Wed, 4 Mar 2009, Thomas Gleixner wrote:
> > > > > > 
> > > > > > Instrumented the code and the result of the failing request is
> > > > > > below. Looks like the function which sets up the request gets
> > > > > > nr_phys_segments wrong by one. 
> > > > > > 
> > > > > > If you need further trace data feel free to ask.
> > > > > 
> > > > > OK, the mapping all checks out correctly ... there must be something
> > > > > wrong with the way we count before mapping.
> > > > 
> > > > Yeah, looks we miscalculate nr_phys_segments in the merging path.
> > > > 
> > > > blk_recount_segments() needs to set bi_seg_front_size and
> > > > bi_seg_back_size for ll_merge_requests_fn()?
> > > > 
> > > > =
> > > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > > index a104593..efb65b6 100644
> > > > --- a/block/blk-merge.c
> > > > +++ b/block/blk-merge.c
> > > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > > >  
> > > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > > >  {
> > > > +	unsigned int seg_size;
> > > >  	struct bio *nxt = bio->bi_next;
> > > >  
> > > >  	bio->bi_next = NULL;
> > > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > > >  	bio->bi_next = nxt;
> > > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > > +
> > > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > > +		bio->bi_seg_front_size = seg_size;
> > > > +	if (bio->bi_phys_segments > bio->bi_seg_back_size)
> > > > +		bio->bi_seg_back_size = seg_size;
> > > > +
> > > >  }
> > > >  EXPORT_SYMBOL(blk_recount_segments);
> > > 
> > > Duh, here's the proper patch.
> > > 
> > > diff --git a/block/blk-merge.c b/block/blk-merge.c
> > > index a104593..06e0db4 100644
> > > --- a/block/blk-merge.c
> > > +++ b/block/blk-merge.c
> > > @@ -111,12 +111,19 @@ void blk_recalc_rq_segments(struct request *rq)
> > >  
> > >  void blk_recount_segments(struct request_queue *q, struct bio *bio)
> > >  {
> > > +	unsigned int seg_size;
> > >  	struct bio *nxt = bio->bi_next;
> > >  
> > >  	bio->bi_next = NULL;
> > > -	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, NULL);
> > > +	bio->bi_phys_segments = __blk_recalc_rq_segments(q, bio, &seg_size);
> > >  	bio->bi_next = nxt;
> > >  	bio->bi_flags |= (1 << BIO_SEG_VALID);
> > > +
> > > +	if (bio->bi_phys_segments == 1 && seg_size > bio->bi_seg_front_size)
> > > +		bio->bi_seg_front_size = seg_size;
> > > +	if (seg_size > bio->bi_seg_back_size)
> > > +		bio->bi_seg_back_size = seg_size;
> > > +
> > >  }
> > >  EXPORT_SYMBOL(blk_recount_segments);
> 
> Good catch, I merged it with a slight change of layout and 
> clearing seg_size initially, to avoid gcc silly errors.

thanks Jens for the super-quick action! I have tried the v2 
patch from Fujita and it's OK in -tip testing so far, on various 
pieces of x86 hardware with different storage systems - ata, 
sata, aic7xxx, etc.

Tested-by: Ingo Molnar <mingo@elte.hu>

	Ingo