Re: [PATCH] fixes for linked list bugs in block I/O code

[Dave Peterson <dsp@llnl.gov>]

On Wednesday 07 May 2003 11:29 pm, Jens Axboe wrote:
> This is convoluted nonsense, bhtail->b_reqnext is NULL by definition. So
> a simple
>
> 	bh->b_reqnext = NULL;
>
> is much clearer.

Ok, that's fine with me.  Setting it explicitly to NULL is also correct
and perhaps a bit more clear.

> >                         req->bhtail->b_reqnext = bh;
> >                         req->bhtail = bh;
> >                         req->nr_sectors = req->hard_nr_sectors += count;
> > @@ -1061,6 +1062,7 @@
> >         req->waiting = NULL;
> >         req->bh = bh;
> >         req->bhtail = bh;
> > +       bh->b_reqnext = NULL;
> >         req->rq_dev = bh->b_rdev;
> >         req->start_time = jiffies;
> >         req_new_io(req, 0, count);
>
> Bart already covered why 2.5 definitely does not need it. I dunno what
> to say for 2.4, to me it looks like a BUG if you pass in a buffer_head
> with uninitialized b_reqnext. Why should that member be any different?

Ok, agreed that 2.5 does not need the patch.

> In fact, from where did you see this buffer_head coming from? Who is
> submitting IO on a not properly inited bh? To me, that sounds like not a
> block layer bug but an fs bug.

I would argue that __make_request() is where the insertion of the buffer_head
into the request structure is performed, and setting the b_reqnext field of
the buffer_head to its proper value is an integral part of performing the
insertion.  Why not keep all of the insertion logic in one place rather than
distribuing it throughout the code and requiring all callers of
__make_request() to remember to initialize b_reqnext?  Localizing the
insertion logic makes the code clearer, more compact, and easier to maintain.

-Dave