Re: [PATCH] block: fix mis-synchronisation in blkdev_issue_zeroout()

[Jeff Moyer <jmoyer@redhat.com>]

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

That's a pretty tight window.  The complete is immediately following the
increment.  I'm surprised thread 2 has time to finish up and exit the
function before the completion is done.

> We can fix this easily by simplifying bio_batch and completion counting.
> We can count completion locally in blkdev_issue_zeroout() without need of
> locking or atomic operation because we are the only one handling issued
> variable holding the number of submitted bios. So remove atomic_t done
> from struct bio_batch.

It seems to me like it might be better to just not complete anything
until the count is zero.  Why issue a wakeup for every bio?
fs/direct-io does something similar, maybe take a look at the
dio_bio_end* routines and see if that would fit well here.  With your
scheme, I worry about missing a completion, maybe because the first bio
completes before you are done submitting bios.  Is that possible?

> Also remove bio_end_io_t *end_io since it is not used.

Yeah, no idea why that was in there.

Cheers,
Jeff