qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [Qemu-devel] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()
Date: Fri, 21 Oct 2016 15:34:14 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0



On 10/21/2016 03:03 PM, Alberto Garcia wrote:
On Fri 21 Oct 2016 08:55:51 PM CEST, John Snow <address@hidden> wrote:

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.

The problem is: do you want to be able to create a new block job and
let it run? Because then you can end up having the same problem that
this patch is trying to prevent if the new job attempts to reopen a
BlockDriverState.


The plan was to create jobs in a pre-started mode and only to engage
them after the drained section. Do any jobs re-open a BDS prior to the
creation of their coroutine?

Yeah, block-commit for example (see commit_start()), and block-stream
after this series.

Berto


Ah, that is a problem for that use case then, but no matter.

I think I've worked out with Kevin the other day (Kevin hit me with a rather large trout) that a drained_all shouldn't really be necessary for qmp_transactions so long as each action is diligent in using bdrv_drained_begin/end for any given BDS that is relevant to it.

I was worried at one point about this flow:

1) bdrv_drained_begin(0x01), do_stuff()
2) bdrv_drained_begin(0x02), do_stuff()
[...]

And thought I might need to rework it as:

bdrv_drained_all_begin()
1) do_stuff()
2) do_stuff()
bdrv_drained_all_end()

but Kevin has pointed out that even though actions and drains are interspersed, the point-in-time simply becomes the time at last drain and it still should be coherent, so I won't need the drain-all after all.

--js





reply via email to

[Prev in Thread] Current Thread [Next in Thread]