Messages in this thread | | | Date | Wed, 17 Oct 2007 10:52:13 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [bug] block subsystem related crash with latest -git |
| |
On Wed, 17 Oct 2007, Jens Axboe wrote: > > OK, the below should actually be safe, I don't know why I talked myself > into the next_sg stuff in the beginning. It's always safe to zero sg, > since it's a valid entry - nothing to save in ->page. Ingo, does this > work for you?
I really don't think this should work.
Doing "sg_next()" on a valid sg is *always* ok. So if the old code didn't work, then "sg" wasn't valid to start with (and the code *after* the sg_next() would have oopsed even if you try to avoid using sg_next.
So avoiding the "sg_next()" on the last entry is pointless.
Also, your patch makes the code almost totally unreadable, with that subtle issue of the "if (bvprv && cluster)" case not triggering on the first case, so the NULL initial sg is "safe".
So at a guess, I think the *real* problem is simply that the passed-in sglist was just too small. What guarantees that the sg list allocation (apparently done by scsi_alloc_sgtable()) is big enough?
If I read things right, scsi_alloc_sgtable() will allocate "cmd->use_sg" SG enties, no? But I also notice that it does not seem to initialize the SG allocation, so those SG entries contain random crap - including, perhaps, a random - and bogus - chain pointer in sg->page..
Yes, we set sh->page *if* we create a chain, but if we don't chain, we leave the old random contents around which in turn may include old and stale chain pointers. Or am I missing something?
So when you added that "memset(sg, 0, sizeof(*sg))" into blk_rq_map_sg(), you did it way too late - it needs to be done when the sg chain is allocated, and for every entry (and then the "link" entry needs to be linked in separately)
I think.
Linus
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |