Re: [PATCH 1/2] Avoid bio_endio recursion

[Jens Axboe <jens.axboe@oracle.com>]

On Tue, Jun 24 2008, Mikulas Patocka wrote:
> 
> 
> On Tue, 24 Jun 2008, Jens Axboe wrote:
> 
> >On Tue, Jun 24 2008, Mikulas Patocka wrote:
> >>Hi
> >>
> >>bio_endio calls bi_end_io callback. In case of stacked devices (raid, dm),
> >>bio_end_io may call bio_endio again, up to an unspecified length.
> >>
> >>The crash because of stack overflow was really observed on sparc64. And
> >>this recursion was one of the contributing factors (using 9 stack frames
> >>--- that is 1728 bytes).
> >
> >Looks good, I like the concept. Can you please make it a little less
> >goto driven, though? The next_bio and goto next_bio could just be a
> >while().
> >
> >-- 
> >Jens Axboe
> >
> 
> Hi.
> 
> This is the patch, slightly de-goto-ized. (it still contains one, I think 
> that while (1) { ... break ... } is no better readable than goto).

Sure, that looks better.

> I found another problem in my previous patch, I forgot about the "error" 
> variable (it would cause misbehavior for example if disk fails, submits an 
> error and raid driver turns this failure into success). We need to save 
> the error variable somewhere in the bio, there is no other place where it 
> could be placed. I temporarily saved it to bi_idx, because it's unused at 
> this place.

I don't think bi_idx is a fantastic idea, I could easily imagine the
bi_end_io function wanting to do a segment loop on the bio. Use
bi_phys_segments instead (or bi_hw_segemnts, no difference), they should
only be used when queuing and building IO, not for completion purposes.
And put a big fat comment there explaining the overload. Plus they are
just a cache, so if you use either of those and at the same time clear
BIO_SEG_VALID in bi_flags, then it's guarenteed to be safe.

Also please put the per-cpu definition outside of bio_endio(). And I
don't think you need to disable interrupts, a plain preempt_disable() /
preempt_enable() should be enough.


-- 
Jens Axboe