Re: [bug] ata subsystem related crash with latest -git

Jens Axboe
2007-10-17T19:35:58Z

On Wed, Oct 17 2007, Linus Torvalds wrote:
>
>
> On Wed, 17 Oct 2007, Ingo Molnar wrote:
> >
> > nope, this did not help. First bootup went fine, second bootup crashed
> > again (see below), without hitting the BUG_ON().
>
> I think you'll always hit it if you have a scatter-gather list that is
> exactly filled up.
>
> Why? Because those things do "sg_next()" on the last entry, and as
> mentioned, that ends up actually accessing one past the end - even if the
> end result is not actually ever *used* (because we just effectively
> incremented it to past the last entry when the code was done with the SG
> list).
>
> So I think the sg_next() interface is fundamentally mis-designed. It
> should do the scatter-gather link following on *starting* to use the SG
> entry, not after finishing with it.
>
> Put another way: I suspect pretty much every single sg_next() out there is
> likely to hit this issue. The way that blk_rq_map_sg() fixed its problem
> was exactly to move the "sg_next()" to *before* the use of the SG (and
> even that one is somewhat bogus, in that it just blindly assumes that the
> first entry is not a link entry).
>
> I suspect the "the next entry is a link" bit should be in the *previous*
> entry, and then sg_next() could look like
>
> if (next_is_link(sg))
> sg = sg_chain_ptr(sg+1);
> else
> sg++;
> return sg;
>
> and that would work.
>
> The alternative is to always make sure to allocate one more SG entry than
> required, so that the last entry is always either the link, or an unused
> entry!

OK, I think you have a very good point here. Ingo, can you verify it
goes away if apply the below? I have to tend to Other Life stuff but
will take this up again tomorrow morning and fix the sg_next() usage.

Linus, please still pull the branch I asked you to earlier. Thanks!

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index aac8a02..58ede7e 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -39,7 +39,7 @@
* (unless chaining is used). Should ideally fit inside a single page, to
* avoid a higher order allocation.
*/
-#define SCSI_MAX_SG_SEGMENTS 128
+#define SCSI_MAX_SG_SEGMENTS 129

struct scsi_host_sg_pool {
size_t size;

--
Jens Axboe