On 2011-03-11 15:31, Lukas Czerner wrote: > On Fri, 11 Mar 2011, Jeff Moyer wrote: > >> Lukas Czerner <[email protected]> writes: >> >>> BZ29402 >>> https://bugzilla.kernel.org/show_bug.cgi?id=29402 >>> >>> We can hit serious mis-synchronization in bio completion path of >>> blkdev_issue_zeroout() leading to a panic. >>> >>> The problem is that when we are going to wait_for_completion() in >>> blkdev_issue_zeroout() we check if the bb.done equals issued (number of >>> submitted bios). If it does, we can skip the wait_for_completition() >>> and just out of the function since there is nothing to wait for. >>> However, there is a ordering problem because bio_batch_end_io() is >>> calling atomic_inc(&bb->done) before complete(), hence it might seem to >>> blkdev_issue_zeroout() that all bios has been completed and exit. At >>> this point when bio_batch_end_io() is going to call complete(bb->wait), >>> bb and wait does not longer exist since it was allocated on stack in >>> blkdev_issue_zeroout() ==> panic! >>> >>> (thread 1) (thread 2) >>> bio_batch_end_io() blkdev_issue_zeroout() >>> if(bb) { ... >>> if (bb->end_io) ... >>> bb->end_io(bio, err); ... >>> atomic_inc(&bb->done); ... >>> ... while (issued != atomic_read(&bb.done)) >>> ... (let issued == bb.done) >>> ... (do the rest of the function) >>> ... return ret; >>> complete(bb->wait); >>> ^^^^^^^^ >>> panic >>> >>> We can fix this easily by simplifying bio_batch and completion counting. >>> >>> Also remove bio_end_io_t *end_io since it is not used. >>> >>> Signed-off-by: Lukas Czerner <[email protected]> >>> Reported-by: Eric Whitney <[email protected]> >>> Tested-by: Eric Whitney <[email protected]> >>> CC: Jens Axboe <[email protected]> >>> CC: Dmitry Monakhov <[email protected]> >>> CC: Jeff Moyer <[email protected]> >>> --- >>> block/blk-lib.c | 19 +++++++------------ >>> 1 files changed, 7 insertions(+), 12 deletions(-) >>> >>> diff --git a/block/blk-lib.c b/block/blk-lib.c >>> index eec78be..bd3e8df 100644 >>> --- a/block/blk-lib.c >>> +++ b/block/blk-lib.c >>> @@ -109,7 +109,6 @@ struct bio_batch >>> atomic_t done; >>> unsigned long flags; >>> struct completion *wait; >>> - bio_end_io_t *end_io; >>> }; >>> >>> static void bio_batch_end_io(struct bio *bio, int err) >>> @@ -122,12 +121,9 @@ static void bio_batch_end_io(struct bio *bio, int err) >>> else >>> clear_bit(BIO_UPTODATE, &bb->flags); >>> } >>> - if (bb) { >>> - if (bb->end_io) >>> - bb->end_io(bio, err); >>> - atomic_inc(&bb->done); >>> - complete(bb->wait); >>> - } >>> + if (bb) >>> + if (atomic_dec_and_test(&bb->done)) >>> + complete(bb->wait); >> >> I think bb will always be set here, no real need to check. >> >> Anyway, I though I already added my: >> >> Reviewed-by: Jeff Moyer <[email protected]> >> >> to this. No? >> >> Cheers, >> Jeff > > Yes, you did and I forgot to add it into the patch. Sorry about that.
No worries, I added it now. -- Jens Axboe _______________________________________________ stable mailing list [email protected] http://linux.kernel.org/mailman/listinfo/stable
