Re: [Qemu-block] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Paolo Bonzini]

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 <address@hidden>
> 
> 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