Re: [libvirt] [PATCH 3/4] qemuDomainAgentJob: Introduce query and modify jobs
On 06/14/2018 12:11 AM, John Ferlan wrote:
On 06/08/2018 09:45 AM, Michal Privoznik wrote:
> These jobs can be used to mark job type over qemu agent.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/qemu/qemu_domain.c | 4 +++-
> src/qemu/qemu_domain.h | 2 ++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 09404f6569..e5e11f0cb7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -94,7 +94,9 @@ VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST,
> );
>
> VIR_ENUM_IMPL(qemuDomainAgentJob, QEMU_AGENT_JOB_LAST,
> - "none"
> + "none",
> + "query",
> + "modify",
> );
>
> VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST,
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 6ada26ca99..f2759951e5 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -84,6 +84,8 @@ VIR_ENUM_DECL(qemuDomainJob)
>
> typedef enum {
> QEMU_AGENT_JOB_NONE = 0, /* No agent job. */
> + QEMU_AGENT_JOB_QUERY, /* Does not change state of domain */
> + QEMU_AGENT_JOB_MODIFY, /* May change state of domain */
Seems there are 2 classes of MODIFY - one is just agent modify and one
is agent and normal modify. Maybe a QEMU_AGENT_JOB_MODIFY_BLOCK would
provide some clarity or easier to read code. A MODIFY_BLOCK ensures a
normal job cannot occur at the same time because something being done
(reboot, shutdown, and settime currently).
Then the black magic for starting both a normal and an agent job is left
to qemuDomainObjBeginAgentJob obviating qemuDomainObjBeginJobWithAgent.
Simmilarly, the specific logic in EndJobWithAgent for is used when
agentActive == MODIFY_BLOCK
No. We have to avoid any dependencies between these two jobs as much as
we can. Here, I can offer another view on jobs. Imagine you have a
struct (say virDomainDef) and you need to protect it from multiple
accesses. So you introduce big lock and anything that touches the struct
will serialize there. This is what we currently have as jobs. But there
is a bottle neck, because in the struct you have two, completely
independent members but you still have one global lock. This is
suboptimal so you invent two locks: one to guard one member, the other
to guard the other member. Cool, so anybody who wishes to access the
first member grabs the first lock, and anybody wishing to access the
second member grabs the second lock. Expect you have couple of places
where you need to grab both. Well, so you do.
And our jobs are like locks (one thread can't set a job until the other
ends it).
Therefore, we have to avoid situation where we could deadlock: that is
one thread has monitor job, the other has agent job and both try to
acquire job of each other (classical deadlock scenario). Sure, you can't
really prevent that with pure syntax, but if we have the third function
(which grabs both jobs) then it's at least more visible what is going
on, I guess.
Michal