Messages in this thread | | | From | Neil Brown <> | Date | Tue, 24 Jun 2008 16:08:15 +1000 | Subject | Re: [PATCH 1/2] Avoid bio_endio recursion |
| |
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.
> + > + 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
| |