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

[Kevin Wolf <kwolf@redhat.com>]

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 <berto@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.

This is somewhat similar to attaching a BlockBackend to a drained BDS.
We already take care to immediately quiesce the BB in this case (even
though this isn't very effective because the BB doesn't propagate it
correctly to its users yet...)

Thoughts?
(Paolo, I'm looking at you.)

Kevin