[libvirt] [PATCH 3/4] qemuDomainAgentJob: Introduce query and modify jobs

[John Ferlan]

On 06/14/2018 10:22 AM, Michal Privoznik wrote:
> 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 at 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 anyone that has chased locks that have any type of dependency knows
about deadlocks and lock ordering. The changes appear to avoid deadlock
scenarios - at least I hope so!

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

Fair enough, but if I'm going to complain about something I should offer
suggestions. I think you've presented enough of a defense of the two
method solution to have me drop thinking about my suggestion.

John