Re: [Qemu-devel] [Qemu-block] [PATCH] block: check BlockDriverState object before dereference

[John Snow]

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:
> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg02305.html
> 

>> 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
level...

> 
> The full explanation of the long-term plans and what "isolated section"
> means is at
> https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg02016.html.
> (Unfortunately, since then we've reintroduced the "double aio_poll" hack
> in BDRV_POLL_WHILE...).
> 
> Paolo
> 

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
hint.

John