lkml.org 
[lkml]   [2011]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2] block: fix mis-synchronisation in blkdev_issue_zeroout()
    On 2011-03-11 15:31, Lukas Czerner wrote:
    > On Fri, 11 Mar 2011, Jeff Moyer wrote:
    >
    >> Lukas Czerner <lczerner@redhat.com> 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 <lczerner@redhat.com>
    >>> Reported-by: Eric Whitney <eric.whitney@hp.com>
    >>> Tested-by: Eric Whitney <eric.whitney@hp.com>
    >>> CC: Jens Axboe <axboe@kernel.dk>
    >>> CC: Dmitry Monakhov <dmonakhov@openvz.org>
    >>> CC: Jeff Moyer <jmoyer@redhat.com>
    >>> ---
    >>> 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 <jmoyer@redhat.com>
    >>
    >> 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



    \
     
     \ /
      Last update: 2011-03-11 15:39    [W:0.027 / U:1.320 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site