[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH] block: check BlockDriverState obje
Re: [Qemu-devel] [Qemu-block] [PATCH] block: check BlockDriverState object before dereference
Mon, 31 Jul 2017 20:14:30 -0400
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1
On 07/23/2017 10:36 AM, Paolo Bonzini wrote:
> On 21/07/2017 17:47, Stefan Hajnoczi wrote:
>> Hmm...BlockDriverState still has bdrv_is_inserted() even though
>> BlockBackend->root can be NULL? CCing Markus in case he has thoughts on
>> the BB/BDS split.
>> I find it weird that block-backend.c calls bdrv_inc_in_flight() and then
>> bdrv_co_flush() will call it again. Perhaps we need to do this so that
>> blk_drain() works correctly but it's odd that they share the same
>> counter variable.
> See here:
>> I need to count requests at the BB level because the blk_aio_*
>> operations have a separate bottom half that is invoked if either 1) they
>> never reach BDS (because of an error); or 2) the bdrv_co_* call
>> completes without yielding. The count must be >0 when blk_aio_*
>> returns, or bdrv_drain (and thus blk_drain) won't loop. Because
>> bdrv_drain and blk_drain are conflated, the counter must be the BDS one.
>> In turn, the BDS counter is needed because of the lack of isolated
>> sections. The right design would be for blk_isolate_begin to call
>> blk_drain on *other* BlockBackends reachable in a child-to-parent visit.
>> Instead, until that is implemented, we have bdrv_drained_begin that
>> emulates that through the same-named callback, followed by a
>> parent-to-child bdrv_drain that is almost always unnecessary.
OK, so you have some justifications for why it needs to be at the BB
> The full explanation of the long-term plans and what "isolated section"
> means is at
> (Unfortunately, since then we've reintroduced the "double aio_poll" hack
> in BDRV_POLL_WHILE...).
I may need some nudging towards understanding what the right solution
here is, though. Should the blk_aio_flush assume that there always is a
root BDS? should it not assume that?
It's difficult for me to understand right now if the bug is in the
expectation for the blk_ functions and the caller should be amended, or
if you need changes to the way the blk_ functions are trying to
increment a counter that doesn't exist.
I can handle the former fairly easily; if it's the latter, I'm afraid
it's stuck in the middle of some of your changes and I'd need a stronger