Lukas Czerner <lczerner@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
