On 19/10/2016 19:11, Kevin Wolf wrote:
> Am 14.10.2016 um 15:08 hat Alberto Garcia geschrieben:
>> bdrv_drain_all() doesn't allow the caller to do anything after all
>> pending requests have been completed but before block jobs are
>> resumed.
>>
>> This patch splits bdrv_drain_all() into _begin() and _end() for that
>> purpose. It also adds aio_{disable,enable}_external() calls to disable
>> external clients in the meantime.
>>
>> Signed-off-by: Alberto Garcia <be...@igalia.com>
>
> This looks okay as a first step, possibly enough for this series (we'll
> have to review this carefully), but it leaves us with a rather limited
> version of bdrv_drain_all_begin/end that excludes many useful cases. One
> of them is that John wants to use it around QMP transactions.
>
> Specifically, you can't add a new BDS or a new block job in a drain_all
> section because then bdrv_drain_all_end() would try to unpause the new
> thing even though it has never been paused. Depending on what else we
> did with it, this will either corrupt the pause counters or just
> directly fail an assertion.
>
> My first thoughts were about how to let an unpause succeed without a
> previous pause for these objects, but actually I think this isn't what
> we should do. We rather want to actually do the pause instead because
> even new BDSes and block jobs should probably start in a quiesced state
> when created inside a drain_all section.Yes, I agree with this. It shouldn't be too hard to implement it. It would require a BlockDriverState to look at the global "inside bdrv_drain_all_begin" state, and ask its BlockBackend to disable itself upon bdrv_replace_child. Basically, "foo->quiesce_counter" should become "foo->quiesce_counter || all_quiesce_counter", I think. It may well be done as a separate patch if there is a TODO comment in bdrv_replace_child; as Kevin said, there are assertions to protect against misuse. Paolo
signature.asc
Description: OpenPGP digital signature
