On Tue, 2011-01-18 at 16:53 -0800, Andrew Morton wrote: > On Mon, 17 Jan 2011 22:00:27 -0500 > David Dillow <dillowda@xxxxxxxx> wrote: > > > When using devices that support max_segments > BIO_MAX_PAGES (256), > > direct IO tries to allocate a bio with more pages than allowed, which > > leads to an oops in dio_bio_alloc(). Clamp the request to the supported > > maximum, and change dio_bio_alloc() to reflect that bio_alloc() will > > always return a bio when called with __GFP_WAIT and a valid number of > > vectors. > > Which device driver triggers this condition? I found it with the changes I've been making to the SRP driver to support much larger IOs; this is not upstream yet. > > Also added cc to stable, as this has been a longstanding item. > > Well. -stable only needs the patch if the driver which triggers the > problem is also there. But we don't know what driver that is, yet??? Good point. Any driver that supports a sg_tablesize of more than 256 has the potential for problems. A quick search of the tree indicates that the following set their initial sg_tablesize to SCSI_MAX_SG_CHAIN_SEGMENTS (2048), though they may be saved by a separate bug I found -- and sent a patch to the list -- that incorrectly causes that define to take the value 128. ./drivers/usb/storage/scsiglue.c ./drivers/scsi/arm/powertec.c ./drivers/scsi/arm/eesox.c ./drivers/scsi/arm/cumana_2.c ./drivers/ata/pata_icside.c CCISS looks to pull the config from the hardware and has a case for more than 512 entries, so maybe on that one. AACRAID has cases above 256, and also seems to calculate the setting in some instances as well. FNIC uses 1024. > > + /* > > + * bio_alloc() is guaranteed to return a bio when called with > > + * __GFP_WAIT and we request a valid number of vectors. > > + */ > > bio = bio_alloc(GFP_KERNEL, nr_vecs); > > + BUG_ON(!bio); > > This BUG_ON() is pretty pointless, > > > bio->bi_bdev = bdev; > > because the next statement will reliably oops, providing us with the > same information. Fair enough, though I'd argue -- weakly -- that BUG_ON gives a pointer to the source. I had to dig through the assembly and figure out that this function was inlined, but I'd agree that it wasn't too hard and it'd be nice to avoid the conditional on a hot path. I'll reroll it, or you can drop the BUG_ON, either way is fine by me. -- Dave Dillow National Center for Computational Science Oak Ridge National Laboratory (865) 241-6602 office -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
