lkml.org 
[lkml]   [2008]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] Avoid bio_endio recursion


    On Tue, 24 Jun 2008, Neil Brown wrote:

    > On Tuesday June 24, mpatocka@redhat.com 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).
    >>
    >> This patch removes the recursion.
    >
    > This is very cool, thanks! A close mirror of the recursion
    > avoidance in generic_make_request.
    >
    > You use a per-cpu queue were generic_make_request uses a per-task
    > queue. This is fitting as bi_end_io doesn't have a process context,
    > but is supposed to be fast and able to run with interrupts disabled,
    > so tying to a cpu is no problem.

    Yes. I think "current" variable can't be used in irq context, it would
    blow with irq-stacks (or access some weird unknown memory).

    I had another version of the patch that doesn't disable interrupts and
    only disables preempt and uses local_t atomic cpu-local variables. It is
    somehow more tricky, because interrupt can be triggered any time while
    processing the queue and it can add anything to the queue. Then I realized
    that bio_endio runs most time with disabled interrupts anyway, so it'd be
    better to just disable interrupts and don't do that local_cmpxchg
    trickery.

    Mikulas

    >> +
    >> + bio_queue = NULL;
    >> +queue_empty_next_bio:
    >> + *bio_end_queue_ptr = &bio_queue;
    >> +next_bio:
    >> +
    >> if (error)
    >> clear_bit(BIO_UPTODATE, &bio->bi_flags);
    >> else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
    >> @@ -1175,6 +1196,17 @@
    >>
    >> if (bio->bi_end_io)
    >> bio->bi_end_io(bio, error);
    >> +
    >> + if (bio_queue) {
    >> + bio = bio_queue;
    >> + bio_queue = bio->bi_next;
    >> + if (!bio_queue) goto queue_empty_next_bio;
    >> + goto next_bio;
    >
    > checkpatch.pl doesn't like that:
    > ERROR: trailing statements should be on next line
    >
    > and I don't either. I would not bother with the mini-optimisation at
    > all.
    > Discard the queue_empty_next_bio label and replace the "if () goto"
    > with
    > if (!bio_queue)
    > *bio_end_queue_ptr = &bio_queue;
    >
    > and leave gcc to optimise the assignment if it wants to.
    >
    > Reviewed-by: NeilBrown <neilb@suse.de>
    >
    > Thanks,
    > NeilBrown
    >


    \
     
     \ /
      Last update: 2008-06-24 16:41    [W:0.026 / U:0.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site