[libvirt] [v2 RESEND 0/3] qemu: Allow concurrent access to monitor and guest agent
Obsoletes: https://www.redhat.com/archives/libvir-list/2018-June/msg01320.html This is the same version as the one linked above, but since I've merged my other patchset that touches the same area in the code reviewers will get merge conflicts when applying these. Rebase & resend. Michal Privoznik (3): qemu: Introduce qemuDomainAgentJob qemu: Introduce APIs for manipulating qemuDomainAgentJob qemu: Switch code to use new agent job APIs src/qemu/THREADS.txt | 112 +++++++++++++++++++++++++--- src/qemu/qemu_domain.c | 193 ++++++++++++++++++++++++++++++++++++++++--------- src/qemu/qemu_domain.h | 30 ++++++++ src/qemu/qemu_driver.c | 91 ++++++++++++++--------- 4 files changed, 346 insertions(+), 80 deletions(-) -- 2.16.4
Introduce guest agent specific job categories to allow threads to run agent monitor specific jobs while normal monitor jobs can also be running. Alter _qemuDomainJobObj in order to duplicate certain fields that will be used for guest agent specific tasks to increase concurrency and throughput and reduce serialization. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++++ src/qemu/qemu_domain.h | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8a2d4750a5..91f3c6d236 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -93,6 +93,12 @@ VIR_ENUM_IMPL(qemuDomainJob, QEMU_JOB_LAST, "async nested", ); +VIR_ENUM_IMPL(qemuDomainAgentJob, QEMU_AGENT_JOB_LAST, + "none", + "query", + "modify", +); + VIR_ENUM_IMPL(qemuDomainAsyncJob, QEMU_ASYNC_JOB_LAST, "none", "migration out", diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9e2da0a37c..12573e5f86 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -82,6 +82,15 @@ typedef enum { } qemuDomainJob; 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 */ + + QEMU_AGENT_JOB_LAST +} qemuDomainAgentJob; +VIR_ENUM_DECL(qemuDomainAgentJob) + /* Async job consists of a series of jobs that may change state. Independent * jobs that do not change state (and possibly others if explicitly allowed by * current async job) are allowed to be run even if async job is active. @@ -158,11 +167,20 @@ typedef struct _qemuDomainJobObj qemuDomainJobObj; typedef qemuDomainJobObj *qemuDomainJobObjPtr; struct _qemuDomainJobObj { virCond cond; /* Use to coordinate jobs */ + + /* The following members are for QEMU_JOB_* */ qemuDomainJob active; /* Currently running job */ unsigned long long owner; /* Thread id which set current job */ const char *ownerAPI; /* The API which owns the job */ unsigned long long started; /* When the current job started */ + /* The following members are for QEMU_AGENT_JOB_* */ + qemuDomainAgentJob agentActive; /* Currently running agent job */ + unsigned long long agentOwner; /* Thread id which set current agent job */ + const char *agentOwnerAPI; /* The API which owns the agent job */ + unsigned long long agentStarted; /* When the current agent job started */ + + /* The following members are for QEMU_ASYNC_JOB_* */ virCond asyncCond; /* Use to coordinate with async jobs */ qemuDomainAsyncJob asyncJob; /* Currently active async job */ unsigned long long asyncOwner; /* Thread which set current async job */ -- 2.16.4
On Tue, Jun 19, 2018 at 08:38:00 +0200, Michal Privoznik wrote:
Introduce guest agent specific job categories to allow threads to run agent monitor specific jobs while normal monitor jobs can also be running.
Alter _qemuDomainJobObj in order to duplicate certain fields that will be used for guest agent specific tasks to increase concurrency and throughput and reduce serialization.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 6 ++++++ src/qemu/qemu_domain.h | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
The point is to break QEMU_JOB_* into smaller pieces which enables us to achieve higher throughput. For instance, if there are two threads, one is trying to query something on qemu monitor while the other is trying to query something on agent monitor these two threads would serialize. There is not much reason for that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/THREADS.txt | 112 ++++++++++++++++++++++++++--- src/qemu/qemu_domain.c | 187 +++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_domain.h | 12 ++++ 3 files changed, 264 insertions(+), 47 deletions(-) diff --git a/src/qemu/THREADS.txt b/src/qemu/THREADS.txt index 7243161fe3..d17f3f4e0c 100644 --- a/src/qemu/THREADS.txt +++ b/src/qemu/THREADS.txt @@ -51,8 +51,8 @@ There are a number of locks on various objects Since virDomainObjPtr lock must not be held during sleeps, the job conditions provide additional protection for code making updates. - QEMU driver uses two kinds of job conditions: asynchronous and - normal. + QEMU driver uses three kinds of job conditions: asynchronous, agent + and normal. Asynchronous job condition is used for long running jobs (such as migration) that consist of several monitor commands and it is @@ -69,16 +69,23 @@ There are a number of locks on various objects it needs to wait until the asynchronous job ends and try to acquire the job again. + Agent job condition is then used when thread wishes to talk to qemu + agent monitor. It is possible to acquire just agent job + (qemuDomainObjBeginAgentJob), or only normal job + (qemuDomainObjBeginJob) or both at the same time + (qemuDomainObjBeginJobWithAgent). Which type of job to grab depends + whether caller wishes to communicate only with agent socket, or only + with qemu monitor socket or both, respectively. + Immediately after acquiring the virDomainObjPtr lock, any method - which intends to update state must acquire either asynchronous or - normal job condition. The virDomainObjPtr lock is released while - blocking on these condition variables. Once the job condition is - acquired, a method can safely release the virDomainObjPtr lock - whenever it hits a piece of code which may sleep/wait, and - re-acquire it after the sleep/wait. Whenever an asynchronous job - wants to talk to the monitor, it needs to acquire nested job (a - special kind of normal job) to obtain exclusive access to the - monitor. + which intends to update state must acquire asynchronous, normal or + agent job . The virDomainObjPtr lock is released while blocking on + these condition variables. Once the job condition is acquired, a + method can safely release the virDomainObjPtr lock whenever it hits + a piece of code which may sleep/wait, and re-acquire it after the + sleep/wait. Whenever an asynchronous job wants to talk to the + monitor, it needs to acquire nested job (a special kind of normal + job) to obtain exclusive access to the monitor. Since the virDomainObjPtr lock was dropped while waiting for the job condition, it is possible that the domain is no longer active @@ -132,6 +139,30 @@ To acquire the normal job condition +To acquire the agent job condition + + qemuDomainObjBeginAgentJob() + - Waits until there is no other agent job set + - Sets job.agentActive tp the job type + + qemuDomainObjEndAgentJob() + - Sets job.agentActive to 0 + - Signals on job.cond condition + + + +To acquire both normal and agent job condition + + qemuDomainObjBeginJobWithAgent() + - Waits until there is no normal and no agent job set + - Sets both job.active and job.agentActive with required job types + + qemuDomainObjEndJobWithAgent() + - Sets both job.active and job.agentActive to 0 + - Signals on job.cond condition + + + To acquire the asynchronous job condition qemuDomainObjBeginAsyncJob() @@ -247,6 +278,65 @@ Design patterns virDomainObjEndAPI(&obj); + * Invoking an agent command on a virDomainObjPtr + + virDomainObjPtr obj; + qemuAgentPtr agent; + + obj = qemuDomObjFromDomain(dom); + + qemuDomainObjBeginAgentJob(obj, QEMU_AGENT_JOB_TYPE); + + ...do prep work... + + if (!qemuDomainAgentAvailable(obj, true)) + goto cleanup; + + agent = qemuDomainObjEnterAgent(obj); + qemuAgentXXXX(agent, ..); + qemuDomainObjExitAgent(obj, agent); + + ...do final work... + + qemuDomainObjEndAgentJob(obj); + virDomainObjEndAPI(&obj); + + + * Invoking both monitor and agent commands on a virDomainObjPtr + + virDomainObjPtr obj; + qemuAgentPtr agent; + + obj = qemuDomObjFromDomain(dom); + + qemuDomainObjBeginJobWithAgent(obj, QEMU_JOB_TYPE, QEMU_AGENT_JOB_TYPE); + + if (!virDomainObjIsActive(dom)) + goto cleanup; + + ...do prep work... + + if (!qemuDomainAgentAvailable(obj, true)) + goto cleanup; + + agent = qemuDomainObjEnterAgent(obj); + qemuAgentXXXX(agent, ..); + qemuDomainObjExitAgent(obj, agent); + + ... + + qemuDomainObjEnterMonitor(obj); + qemuMonitorXXXX(priv->mon); + qemuDomainObjExitMonitor(obj); + + /* Alternatively, talk to the monitor first and then talk to the agent. */ + + ...do final work... + + qemuDomainObjEndJobWithAgent(obj); + virDomainObjEndAPI(&obj); + + * Running asynchronous job virDomainObjPtr obj; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 91f3c6d236..30a06a1575 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -313,6 +313,19 @@ qemuDomainObjResetJob(qemuDomainObjPrivatePtr priv) job->started = 0; } + +static void +qemuDomainObjResetAgentJob(qemuDomainObjPrivatePtr priv) +{ + qemuDomainJobObjPtr job = &priv->job; + + job->agentActive = QEMU_AGENT_JOB_NONE; + job->agentOwner = 0; + job->agentOwnerAPI = NULL; + job->agentStarted = 0; +} + + static void qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv) { @@ -6356,6 +6369,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) return !priv->job.active && qemuDomainNestedJobAllowed(priv, job); } +static bool +qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv, + qemuDomainJob job, + qemuDomainAgentJob agentJob) +{ + return (job == QEMU_JOB_NONE || !priv->job.active) && + (agentJob == QEMU_AGENT_JOB_NONE || !priv->job.agentActive); +} + /* Give up waiting for mutex after 30 seconds */ #define QEMU_JOB_WAIT_TIME (1000ull * 30) @@ -6385,6 +6407,7 @@ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainJob job, + qemuDomainAgentJob agentJob, qemuDomainAsyncJob asyncJob, bool nowait) { @@ -6398,16 +6421,15 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, int ret = -1; unsigned long long duration = 0; unsigned long long asyncDuration = 0; - const char *jobStr; - if (async) - jobStr = qemuDomainAsyncJobTypeToString(asyncJob); - else - jobStr = qemuDomainJobTypeToString(job); - - VIR_DEBUG("Starting %s: %s (vm=%p name=%s, current job=%s async=%s)", - async ? "async job" : "job", jobStr, obj, obj->def->name, + VIR_DEBUG("Starting job: job=%s agentJob=%s asyncJob=%s " + "(vm=%p name=%s, current job=%s agentJob=%s async=%s)", + qemuDomainJobTypeToString(job), + qemuDomainAgentJobTypeToString(agentJob), + qemuDomainAsyncJobTypeToString(asyncJob), + obj, obj->def->name, qemuDomainJobTypeToString(priv->job.active), + qemuDomainAgentJobTypeToString(priv->job.agentActive), qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); if (virTimeMillisNow(&now) < 0) { @@ -6434,10 +6456,9 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, goto error; } - while (priv->job.active) { + while (!qemuDomainObjCanSetJob(priv, job, agentJob)) { if (nowait) goto cleanup; - VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) goto error; @@ -6448,32 +6469,48 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, if (!nested && !qemuDomainNestedJobAllowed(priv, job)) goto retry; - qemuDomainObjResetJob(priv); - ignore_value(virTimeMillisNow(&now)); - if (job != QEMU_JOB_ASYNC) { - VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)", - qemuDomainJobTypeToString(job), - qemuDomainAsyncJobTypeToString(priv->job.asyncJob), - obj, obj->def->name); - priv->job.active = job; - priv->job.owner = virThreadSelfID(); - priv->job.ownerAPI = virThreadJobGet(); + if (job) { + qemuDomainObjResetJob(priv); + + if (job != QEMU_JOB_ASYNC) { + VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)", + qemuDomainJobTypeToString(job), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + obj, obj->def->name); + priv->job.active = job; + priv->job.owner = virThreadSelfID(); + priv->job.ownerAPI = virThreadJobGet(); + priv->job.started = now; + } else { + VIR_DEBUG("Started async job: %s (vm=%p name=%s)", + qemuDomainAsyncJobTypeToString(asyncJob), + obj, obj->def->name); + qemuDomainObjResetAsyncJob(priv); + if (VIR_ALLOC(priv->job.current) < 0) + goto cleanup; + priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE; + priv->job.asyncJob = asyncJob; + priv->job.asyncOwner = virThreadSelfID(); + priv->job.asyncOwnerAPI = virThreadJobGet(); + priv->job.asyncStarted = now; + priv->job.current->started = now; + } + } + + if (agentJob) { + qemuDomainObjResetAgentJob(priv); + + VIR_DEBUG("Started agent job: %s (vm=%p name=%s job=%s async=%s)", + qemuDomainAgentJobTypeToString(agentJob), + obj, obj->def->name, + qemuDomainJobTypeToString(priv->job.active), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + priv->job.agentActive = agentJob; + priv->job.agentOwner = virThreadSelfID(); + priv->job.agentOwnerAPI = virThreadJobGet(); priv->job.started = now; - } else { - VIR_DEBUG("Started async job: %s (vm=%p name=%s)", - qemuDomainAsyncJobTypeToString(asyncJob), - obj, obj->def->name); - qemuDomainObjResetAsyncJob(priv); - if (VIR_ALLOC(priv->job.current) < 0) - goto cleanup; - priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE; - priv->job.asyncJob = asyncJob; - priv->job.asyncOwner = virThreadSelfID(); - priv->job.asyncOwnerAPI = virThreadJobGet(); - priv->job.asyncStarted = now; - priv->job.current->started = now; } if (qemuDomainTrackJob(job)) @@ -6554,12 +6591,46 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, qemuDomainJob job) { if (qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_AGENT_JOB_NONE, QEMU_ASYNC_JOB_NONE, false) < 0) return -1; else return 0; } +/** + * qemuDomainObjBeginAgentJob: + * + * Grabs agent type of job. Use if caller talks to guest agent only. + * + * To end job call qemuDomainObjEndAgentJob. + */ +int qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainAgentJob agentJob) +{ + return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_NONE, + agentJob, + QEMU_ASYNC_JOB_NONE, false); +} + +/** + * qemuDomainObjBeginJobWithAgent: + * + * Grabs both monitor and agent types of job. Use if caller talks to both + * monitor and guest agent. + * + * To end job call qemuDomainObjEndJobWithAgent. + */ +int qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job, + qemuDomainAgentJob agentJob) +{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + agentJob, QEMU_ASYNC_JOB_NONE, false); +} + int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainAsyncJob asyncJob, @@ -6569,6 +6640,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv; if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC, + QEMU_AGENT_JOB_NONE, asyncJob, false) < 0) return -1; @@ -6598,6 +6670,7 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver, return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC_NESTED, + QEMU_AGENT_JOB_NONE, QEMU_ASYNC_JOB_NONE, false); } @@ -6621,6 +6694,7 @@ qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver, qemuDomainJob job) { return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_AGENT_JOB_NONE, QEMU_ASYNC_JOB_NONE, true); } @@ -6646,7 +6720,47 @@ qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj) qemuDomainObjResetJob(priv); if (qemuDomainTrackJob(job)) qemuDomainObjSaveJob(driver, obj); - virCondSignal(&priv->job.cond); + virCondBroadcast(&priv->job.cond); +} + +void +qemuDomainObjEndAgentJob(virDomainObjPtr obj) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + qemuDomainAgentJob agentJob = priv->job.agentActive; + + priv->jobs_queued--; + + VIR_DEBUG("Stopping agent job: %s (async=%s vm=%p name=%s)", + qemuDomainAgentJobTypeToString(agentJob), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + obj, obj->def->name); + + qemuDomainObjResetAgentJob(priv); + virCondBroadcast(&priv->job.cond); +} + +void +qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver, + virDomainObjPtr obj) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + qemuDomainJob job = priv->job.active; + qemuDomainAgentJob agentJob = priv->job.agentActive; + + priv->jobs_queued--; + + VIR_DEBUG("Stopping both jobs: %s %s (async=%s vm=%p name=%s)", + qemuDomainJobTypeToString(job), + qemuDomainAgentJobTypeToString(agentJob), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + obj, obj->def->name); + + qemuDomainObjResetJob(priv); + qemuDomainObjResetAgentJob(priv); + if (qemuDomainTrackJob(job)) + qemuDomainObjSaveJob(driver, obj); + virCondBroadcast(&priv->job.cond); } void @@ -6800,8 +6914,9 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver, * obj must be locked before calling * * To be called immediately before any QEMU agent API call. - * Must have already called qemuDomainObjBeginJob() and checked - * that the VM is still active. + * Must have already called qemuDomainObjBeginAgentJob() or + * qemuDomainObjBeginJobWithAgent() and checked that the VM is + * still active. * * To be followed with qemuDomainObjExitAgent() once complete */ diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 12573e5f86..0a9af756b8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -518,6 +518,15 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainJob job) ATTRIBUTE_RETURN_CHECK; +int qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainAgentJob agentJob) + ATTRIBUTE_RETURN_CHECK; +int qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job, + qemuDomainAgentJob agentJob) + ATTRIBUTE_RETURN_CHECK; int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainAsyncJob asyncJob, @@ -535,6 +544,9 @@ int qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver, void qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj); +void qemuDomainObjEndAgentJob(virDomainObjPtr obj); +void qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver, + virDomainObjPtr obj); void qemuDomainObjEndAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj); void qemuDomainObjAbortAsyncJob(virDomainObjPtr obj); -- 2.16.4
On Tue, Jun 19, 2018 at 08:38:01 +0200, Michal Privoznik wrote:
The point is to break QEMU_JOB_* into smaller pieces which enables us to achieve higher throughput. For instance, if there are two threads, one is trying to query something on qemu monitor while the other is trying to query something on agent monitor these two threads would serialize. There is not much reason for that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/THREADS.txt | 112 ++++++++++++++++++++++++++--- src/qemu/qemu_domain.c | 187 +++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_domain.h | 12 ++++ 3 files changed, 264 insertions(+), 47 deletions(-) ... diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 91f3c6d236..30a06a1575 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c ... @@ -6356,6 +6369,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) return !priv->job.active && qemuDomainNestedJobAllowed(priv, job); }
+static bool +qemuDomainObjCanSetJob(qemuDomainObjPrivatePtr priv, + qemuDomainJob job, + qemuDomainAgentJob agentJob) +{ + return (job == QEMU_JOB_NONE || !priv->job.active) && + (agentJob == QEMU_AGENT_JOB_NONE || !priv->job.agentActive);
This is pretty strange, everything you compare here is an enum, either qemuDomainJob or qemuDomainAgentJob and mixing ! with an explicit comparison with *_NONE is confusing. It's not incorrect, but I think (job == QEMU_JOB_NONE || priv->job.active == QEMU_JOB_NONE) && (agentJob == QEMU_AGENT_JOB_NONE || priv->job.agentActive == QEMU_AGENT_JOB_NONE) would be easier to read. Or alternatively you could use ! everywhere.
+} + /* Give up waiting for mutex after 30 seconds */ #define QEMU_JOB_WAIT_TIME (1000ull * 30)
...
@@ -6434,10 +6456,9 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, goto error; }
- while (priv->job.active) { + while (!qemuDomainObjCanSetJob(priv, job, agentJob)) {
You're now checking more variables for a single condition which means waking up a single thread with virCondSignal could easily wake the one which cannot currently run. We need to use virCondBroadcast instead and I see you did the change, which is nice. However, it might be useful to add a comment to the code explaining why we use virCondBroadcast to avoid any future confusion or even blind attempts to replace it with virCondSignal.
if (nowait) goto cleanup; -
Drop this line removal, it's most likely a rebase artifact anyway.
VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) goto error; @@ -6448,32 +6469,48 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, if (!nested && !qemuDomainNestedJobAllowed(priv, job)) goto retry;
- qemuDomainObjResetJob(priv); - ignore_value(virTimeMillisNow(&now));
- if (job != QEMU_JOB_ASYNC) { - VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)", - qemuDomainJobTypeToString(job), - qemuDomainAsyncJobTypeToString(priv->job.asyncJob), - obj, obj->def->name); - priv->job.active = job; - priv->job.owner = virThreadSelfID(); - priv->job.ownerAPI = virThreadJobGet(); + if (job) { + qemuDomainObjResetJob(priv); + + if (job != QEMU_JOB_ASYNC) { + VIR_DEBUG("Started job: %s (async=%s vm=%p name=%s)", + qemuDomainJobTypeToString(job), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + obj, obj->def->name); + priv->job.active = job; + priv->job.owner = virThreadSelfID(); + priv->job.ownerAPI = virThreadJobGet(); + priv->job.started = now; + } else { + VIR_DEBUG("Started async job: %s (vm=%p name=%s)", + qemuDomainAsyncJobTypeToString(asyncJob), + obj, obj->def->name); + qemuDomainObjResetAsyncJob(priv); + if (VIR_ALLOC(priv->job.current) < 0) + goto cleanup; + priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE; + priv->job.asyncJob = asyncJob; + priv->job.asyncOwner = virThreadSelfID(); + priv->job.asyncOwnerAPI = virThreadJobGet(); + priv->job.asyncStarted = now; + priv->job.current->started = now; + } + } + + if (agentJob) { + qemuDomainObjResetAgentJob(priv); + + VIR_DEBUG("Started agent job: %s (vm=%p name=%s job=%s async=%s)", + qemuDomainAgentJobTypeToString(agentJob), + obj, obj->def->name, + qemuDomainJobTypeToString(priv->job.active), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + priv->job.agentActive = agentJob; + priv->job.agentOwner = virThreadSelfID(); + priv->job.agentOwnerAPI = virThreadJobGet(); priv->job.started = now;
This should be priv->job.agentStarted = now
- } else { - VIR_DEBUG("Started async job: %s (vm=%p name=%s)", - qemuDomainAsyncJobTypeToString(asyncJob), - obj, obj->def->name); - qemuDomainObjResetAsyncJob(priv); - if (VIR_ALLOC(priv->job.current) < 0) - goto cleanup; - priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_ACTIVE; - priv->job.asyncJob = asyncJob; - priv->job.asyncOwner = virThreadSelfID(); - priv->job.asyncOwnerAPI = virThreadJobGet(); - priv->job.asyncStarted = now; - priv->job.current->started = now; }
if (qemuDomainTrackJob(job)) @@ -6554,12 +6591,46 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, qemuDomainJob job) { if (qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_AGENT_JOB_NONE, QEMU_ASYNC_JOB_NONE, false) < 0) return -1; else return 0; }
+/** + * qemuDomainObjBeginAgentJob: + * + * Grabs agent type of job. Use if caller talks to guest agent only. + * + * To end job call qemuDomainObjEndAgentJob. + */ +int qemuDomainObjBeginAgentJob(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainAgentJob agentJob)
I know this file is pretty inconsistent and you're not making it a lot worse, but s/int /int / and reindent the rest.
+{ + return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_NONE, + agentJob, + QEMU_ASYNC_JOB_NONE, false); +} + +/** + * qemuDomainObjBeginJobWithAgent: + * + * Grabs both monitor and agent types of job. Use if caller talks to both + * monitor and guest agent. + * + * To end job call qemuDomainObjEndJobWithAgent. + */ +int qemuDomainObjBeginJobWithAgent(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job, + qemuDomainAgentJob agentJob)
s/int /int / again
+{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + agentJob, QEMU_ASYNC_JOB_NONE, false);
Strange indentation. And since you're going to touch the code here, the following formatting would look a bit better: return qemuDomainObjBeginJobInternal(driver, obj, job, agentJob, QEMU_ASYNC_JOB_NONE, false);
+} + int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainAsyncJob asyncJob, @@ -6569,6 +6640,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv;
if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC, + QEMU_AGENT_JOB_NONE, asyncJob, false) < 0) return -1;
@@ -6598,6 +6670,7 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC_NESTED, + QEMU_AGENT_JOB_NONE, QEMU_ASYNC_JOB_NONE, false); } @@ -6621,6 +6694,7 @@ qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver, qemuDomainJob job) { return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_AGENT_JOB_NONE, QEMU_ASYNC_JOB_NONE, true); }
@@ -6646,7 +6720,47 @@ qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj) qemuDomainObjResetJob(priv); if (qemuDomainTrackJob(job)) qemuDomainObjSaveJob(driver, obj); - virCondSignal(&priv->job.cond); + virCondBroadcast(&priv->job.cond); +} + +void +qemuDomainObjEndAgentJob(virDomainObjPtr obj) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + qemuDomainAgentJob agentJob = priv->job.agentActive; + + priv->jobs_queued--; + + VIR_DEBUG("Stopping agent job: %s (async=%s vm=%p name=%s)", + qemuDomainAgentJobTypeToString(agentJob), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + obj, obj->def->name); + + qemuDomainObjResetAgentJob(priv); + virCondBroadcast(&priv->job.cond); +} + +void +qemuDomainObjEndJobWithAgent(virQEMUDriverPtr driver, + virDomainObjPtr obj) +{ + qemuDomainObjPrivatePtr priv = obj->privateData; + qemuDomainJob job = priv->job.active; + qemuDomainAgentJob agentJob = priv->job.agentActive; + + priv->jobs_queued--; + + VIR_DEBUG("Stopping both jobs: %s %s (async=%s vm=%p name=%s)", + qemuDomainJobTypeToString(job), + qemuDomainAgentJobTypeToString(agentJob), + qemuDomainAsyncJobTypeToString(priv->job.asyncJob), + obj, obj->def->name); + + qemuDomainObjResetJob(priv); + qemuDomainObjResetAgentJob(priv); + if (qemuDomainTrackJob(job)) + qemuDomainObjSaveJob(driver, obj); + virCondBroadcast(&priv->job.cond); }
void @@ -6800,8 +6914,9 @@ qemuDomainObjEnterMonitorAsync(virQEMUDriverPtr driver, * obj must be locked before calling * * To be called immediately before any QEMU agent API call. - * Must have already called qemuDomainObjBeginJob() and checked - * that the VM is still active. + * Must have already called qemuDomainObjBeginAgentJob() or + * qemuDomainObjBeginJobWithAgent() and checked that the VM is + * still active.
The documentation of qemuDomainObjEnterMonitorInternal would benefit from similar treatment.
* * To be followed with qemuDomainObjExitAgent() once complete */ ...
With the small issues fixed Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
There are two sets of functions here: 1) some functions talk on both monitor and agent monitor, 2) some functions only talk on agent monitor. For functions from set 1) we need to use qemuDomainObjBeginJobWithAgent() and for functions from set 2) we need to use qemuDomainObjBeginAgentJob() only. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 91 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3abbe41895..cffd4c928a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1954,6 +1954,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) bool useAgent = false, agentRequested, acpiRequested; bool isReboot = false; bool agentForced; + bool agentJob = false; int agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN; virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN | @@ -1980,9 +1981,14 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0)) goto cleanup; + agentJob = useAgent; + if (virDomainObjGetState(vm, NULL) != VIR_DOMAIN_RUNNING) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); @@ -2026,7 +2032,10 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) } endjob: - qemuDomainObjEndJob(driver, vm); + if (agentJob) + qemuDomainObjEndJobWithAgent(driver, vm); + else + qemuDomainObjEndJob(driver, vm); cleanup: virDomainObjEndAPI(&vm); @@ -2049,6 +2058,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) bool useAgent = false, agentRequested, acpiRequested; bool isReboot = true; bool agentForced; + bool agentJob = false; int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT; virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN | @@ -2075,9 +2085,14 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0)) goto cleanup; + agentJob = useAgent; + agentForced = agentRequested && !acpiRequested; if (!qemuDomainAgentAvailable(vm, agentForced)) { if (agentForced) @@ -2115,7 +2130,10 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) } endjob: - qemuDomainObjEndJob(driver, vm); + if (agentJob) + qemuDomainObjEndJobWithAgent(driver, vm); + else + qemuDomainObjEndJob(driver, vm); cleanup: virDomainObjEndAPI(&vm); @@ -4949,6 +4967,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, virDomainDefPtr def; virDomainDefPtr persistentDef; bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE); + bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST); int ret = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -4963,13 +4982,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)) goto cleanup; if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob; - if (flags & VIR_DOMAIN_VCPU_GUEST) + if (useAgent) ret = qemuDomainSetVcpusAgent(vm, nvcpus); else if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = qemuDomainSetVcpusMax(driver, def, persistentDef, nvcpus); @@ -4978,7 +4998,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, nvcpus, hotpluggable); endjob: - qemuDomainObjEndJob(driver, vm); + if (useAgent) + qemuDomainObjEndAgentJob(vm); + else + qemuDomainObjEndJob(driver, vm); cleanup: virDomainObjEndAPI(&vm); @@ -5429,7 +5452,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) goto cleanup; if (flags & VIR_DOMAIN_VCPU_GUEST) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup; if (!virDomainObjIsActive(vm)) { @@ -5447,7 +5470,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) qemuDomainObjExitAgent(vm, agent); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); if (ncpuinfo < 0) goto cleanup; @@ -18954,7 +18977,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -18985,7 +19008,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, qemuDomainObjExitAgent(vm, agent); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -19070,7 +19093,7 @@ qemuDomainQemuAgentCommand(virDomainPtr domain, if (virDomainQemuAgentCommandEnsureACL(domain->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -19088,7 +19111,7 @@ qemuDomainQemuAgentCommand(virDomainPtr domain, VIR_FREE(result); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -19170,7 +19193,7 @@ qemuDomainFSTrim(virDomainPtr dom, if (virDomainFSTrimEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (!qemuDomainAgentAvailable(vm, true)) @@ -19184,7 +19207,7 @@ qemuDomainFSTrim(virDomainPtr dom, qemuDomainObjExitAgent(vm, agent); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -19352,7 +19375,7 @@ qemuDomainGetTime(virDomainPtr dom, if (virDomainGetTimeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -19371,7 +19394,7 @@ qemuDomainGetTime(virDomainPtr dom, ret = 0; endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -19402,7 +19425,9 @@ qemuDomainSetTime(virDomainPtr dom, priv = vm->privateData; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -19447,7 +19472,7 @@ qemuDomainSetTime(virDomainPtr dom, ret = 0; endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndJobWithAgent(driver, vm); cleanup: virDomainObjEndAPI(&vm); @@ -19473,7 +19498,7 @@ qemuDomainFSFreeze(virDomainPtr dom, if (virDomainFSFreezeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -19482,7 +19507,7 @@ qemuDomainFSFreeze(virDomainPtr dom, ret = qemuDomainSnapshotFSFreeze(driver, vm, mountpoints, nmountpoints); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -19514,7 +19539,7 @@ qemuDomainFSThaw(virDomainPtr dom, if (virDomainFSThawEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -19523,7 +19548,7 @@ qemuDomainFSThaw(virDomainPtr dom, ret = qemuDomainSnapshotFSThaw(driver, vm, true); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -20547,7 +20572,7 @@ qemuDomainGetFSInfo(virDomainPtr dom, if (virDomainGetFSInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -20567,7 +20592,7 @@ qemuDomainGetFSInfo(virDomainPtr dom, qemuDomainObjExitAgent(vm, agent); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -20604,7 +20629,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, break; case VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT: - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup; if (!qemuDomainAgentAvailable(vm, true)) @@ -20615,7 +20640,7 @@ qemuDomainInterfaceAddresses(virDomainPtr dom, qemuDomainObjExitAgent(vm, agent); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); break; @@ -20816,7 +20841,7 @@ qemuDomainSetUserPassword(virDomainPtr dom, if (virDomainSetUserPasswordEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (virDomainObjCheckActive(vm) < 0) @@ -20836,7 +20861,7 @@ qemuDomainSetUserPassword(virDomainPtr dom, ret = 0; endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: virDomainObjEndAPI(&vm); @@ -21077,7 +21102,7 @@ qemuDomainGetGuestVcpus(virDomainPtr dom, if (virDomainGetGuestVcpusEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup; if (!qemuDomainAgentAvailable(vm, true)) @@ -21096,7 +21121,7 @@ qemuDomainGetGuestVcpus(virDomainPtr dom, ret = 0; endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: VIR_FREE(info); @@ -21136,7 +21161,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, if (virDomainSetGuestVcpusEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup; if (!qemuDomainAgentAvailable(vm, true)) @@ -21182,7 +21207,7 @@ qemuDomainSetGuestVcpus(virDomainPtr dom, qemuDomainObjExitAgent(vm, agent); endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm); cleanup: VIR_FREE(info); -- 2.16.4
On Tue, Jun 19, 2018 at 08:38:02 +0200, Michal Privoznik wrote:
There are two sets of functions here: 1) some functions talk on both monitor and agent monitor, 2) some functions only talk on agent monitor.
For functions from set 1) we need to use qemuDomainObjBeginJobWithAgent() and for functions from set 2) we need to use qemuDomainObjBeginAgentJob() only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 91 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3abbe41895..cffd4c928a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1954,6 +1954,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) bool useAgent = false, agentRequested, acpiRequested; bool isReboot = false; bool agentForced; + bool agentJob = false; int agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN;
virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN | @@ -1980,9 +1981,14 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0))
Looks a bit hard to parse, it would be easier to just call qemuDomainObjBeginJobWithAgent: qemuDomainAgentJob agentJob = QEMU_AGENT_JOB_NONE; if (useAgent) agentJob = QEMU_AGENT_JOB_MODIFY; if (qemuDomainObjBeginJobWithAgent(driver, vm, QEMU_JOB_MODIFY, agentJob) < 0) goto cleanup; Perhaps it would even make sense to document this usage if the caller does not always need to talk to the agent.
@@ -2026,7 +2032,10 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) }
endjob: - qemuDomainObjEndJob(driver, vm); + if (agentJob) + qemuDomainObjEndJobWithAgent(driver, vm); + else + qemuDomainObjEndJob(driver, vm);
And this would still work even with the suggested changes.
cleanup: virDomainObjEndAPI(&vm); @@ -2049,6 +2058,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) bool useAgent = false, agentRequested, acpiRequested; bool isReboot = true; bool agentForced; + bool agentJob = false; int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT;
virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN | @@ -2075,9 +2085,14 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0))
The same pattern could be used here.
goto cleanup;
+ agentJob = useAgent; + agentForced = agentRequested && !acpiRequested; if (!qemuDomainAgentAvailable(vm, agentForced)) { if (agentForced) @@ -2115,7 +2130,10 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) }
endjob: - qemuDomainObjEndJob(driver, vm); + if (agentJob) + qemuDomainObjEndJobWithAgent(driver, vm); + else + qemuDomainObjEndJob(driver, vm);
cleanup: virDomainObjEndAPI(&vm); @@ -4949,6 +4967,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, virDomainDefPtr def; virDomainDefPtr persistentDef; bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE); + bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST); int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -4963,13 +4982,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)) goto cleanup;
And here.
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob;
- if (flags & VIR_DOMAIN_VCPU_GUEST) + if (useAgent) ret = qemuDomainSetVcpusAgent(vm, nvcpus); else if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = qemuDomainSetVcpusMax(driver, def, persistentDef, nvcpus); @@ -4978,7 +4998,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, nvcpus, hotpluggable);
endjob: - qemuDomainObjEndJob(driver, vm); + if (useAgent) + qemuDomainObjEndAgentJob(vm); + else + qemuDomainObjEndJob(driver, vm);
cleanup: virDomainObjEndAPI(&vm); @@ -5429,7 +5452,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) goto cleanup;
if (flags & VIR_DOMAIN_VCPU_GUEST) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup;
if (!virDomainObjIsActive(vm)) { @@ -5447,7 +5470,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) qemuDomainObjExitAgent(vm, agent);
endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm);
if (ncpuinfo < 0) goto cleanup;
I'd expect changes to qemuDomainSnapshotCreateActiveExternal here since it calls qemuDomainSnapshotFSFreeze and qemuDomainSnapshotFSThaw from an async job without acquiring a nested job, which looks wrong. And this patch should change it to acquire an agent job before talking to the agent.
@@ -18954,7 +18977,7 @@ qemuDomainPMSuspendForDuration(virDomainPtr dom, if (virDomainPMSuspendForDurationEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0) goto cleanup;
if (virDomainObjCheckActive(vm) < 0) ...
The rest looks OK. Jirka
On 06/19/2018 05:05 PM, Jiri Denemark wrote:
On Tue, Jun 19, 2018 at 08:38:02 +0200, Michal Privoznik wrote:
There are two sets of functions here: 1) some functions talk on both monitor and agent monitor, 2) some functions only talk on agent monitor.
For functions from set 1) we need to use qemuDomainObjBeginJobWithAgent() and for functions from set 2) we need to use qemuDomainObjBeginAgentJob() only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 91 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3abbe41895..cffd4c928a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1954,6 +1954,7 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) bool useAgent = false, agentRequested, acpiRequested; bool isReboot = false; bool agentForced; + bool agentJob = false; int agentFlag = QEMU_AGENT_SHUTDOWN_POWERDOWN;
virCheckFlags(VIR_DOMAIN_SHUTDOWN_ACPI_POWER_BTN | @@ -1980,9 +1981,14 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) if (virDomainShutdownFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0))
Looks a bit hard to parse, it would be easier to just call qemuDomainObjBeginJobWithAgent:
qemuDomainAgentJob agentJob = QEMU_AGENT_JOB_NONE;
if (useAgent) agentJob = QEMU_AGENT_JOB_MODIFY;
if (qemuDomainObjBeginJobWithAgent(driver, vm, QEMU_JOB_MODIFY, agentJob) < 0) goto cleanup;
Perhaps it would even make sense to document this usage if the caller does not always need to talk to the agent.
@@ -2026,7 +2032,10 @@ static int qemuDomainShutdownFlags(virDomainPtr dom, unsigned int flags) }
endjob: - qemuDomainObjEndJob(driver, vm); + if (agentJob) + qemuDomainObjEndJobWithAgent(driver, vm); + else + qemuDomainObjEndJob(driver, vm);
And this would still work even with the suggested changes.
Okay.
cleanup: virDomainObjEndAPI(&vm); @@ -2049,6 +2058,7 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) bool useAgent = false, agentRequested, acpiRequested; bool isReboot = true; bool agentForced; + bool agentJob = false; int agentFlag = QEMU_AGENT_SHUTDOWN_REBOOT;
virCheckFlags(VIR_DOMAIN_REBOOT_ACPI_POWER_BTN | @@ -2075,9 +2085,14 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) if (virDomainRebootEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginJobWithAgent(driver, vm, + QEMU_JOB_MODIFY, + QEMU_AGENT_JOB_MODIFY) < 0))
The same pattern could be used here.
goto cleanup;
+ agentJob = useAgent; + agentForced = agentRequested && !acpiRequested; if (!qemuDomainAgentAvailable(vm, agentForced)) { if (agentForced) @@ -2115,7 +2130,10 @@ qemuDomainReboot(virDomainPtr dom, unsigned int flags) }
endjob: - qemuDomainObjEndJob(driver, vm); + if (agentJob) + qemuDomainObjEndJobWithAgent(driver, vm); + else + qemuDomainObjEndJob(driver, vm);
cleanup: virDomainObjEndAPI(&vm); @@ -4949,6 +4967,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, virDomainDefPtr def; virDomainDefPtr persistentDef; bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE); + bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST); int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -4963,13 +4982,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)) goto cleanup;
And here.
Actually no. This one is different to the previous two places. This one is either grab domain job OR agent job but not both at the same time (which is what previous places do). I could do: if (flags & VIR_DOMAIN_VCPU_GUEST) agentJob = QEMU_AGENT_JOB_MODIFY else job = QEMU_JOB_MODIFY; and then if (qemuDomainObjBeginJobWithAgent(job, agentJob) < 0) ... It's rather stretching of the API but whatever.
if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto endjob;
- if (flags & VIR_DOMAIN_VCPU_GUEST) + if (useAgent) ret = qemuDomainSetVcpusAgent(vm, nvcpus); else if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = qemuDomainSetVcpusMax(driver, def, persistentDef, nvcpus); @@ -4978,7 +4998,10 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, nvcpus, hotpluggable);
endjob: - qemuDomainObjEndJob(driver, vm); + if (useAgent) + qemuDomainObjEndAgentJob(vm); + else + qemuDomainObjEndJob(driver, vm);
cleanup: virDomainObjEndAPI(&vm); @@ -5429,7 +5452,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) goto cleanup;
if (flags & VIR_DOMAIN_VCPU_GUEST) { - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + if (qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_QUERY) < 0) goto cleanup;
if (!virDomainObjIsActive(vm)) { @@ -5447,7 +5470,7 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) qemuDomainObjExitAgent(vm, agent);
endjob: - qemuDomainObjEndJob(driver, vm); + qemuDomainObjEndAgentJob(vm);
if (ncpuinfo < 0) goto cleanup;
I'd expect changes to qemuDomainSnapshotCreateActiveExternal here since it calls qemuDomainSnapshotFSFreeze and qemuDomainSnapshotFSThaw from an async job without acquiring a nested job, which looks wrong. And this patch should change it to acquire an agent job before talking to the agent.
Okay, I'll fix it and post v3. Michal
On Tue, Jun 19, 2018 at 19:03:24 +0200, Michal Prívozník wrote:
On 06/19/2018 05:05 PM, Jiri Denemark wrote:
On Tue, Jun 19, 2018 at 08:38:02 +0200, Michal Privoznik wrote:
There are two sets of functions here: 1) some functions talk on both monitor and agent monitor, 2) some functions only talk on agent monitor.
For functions from set 1) we need to use qemuDomainObjBeginJobWithAgent() and for functions from set 2) we need to use qemuDomainObjBeginAgentJob() only.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 91 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 58 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3abbe41895..cffd4c928a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c ... @@ -4949,6 +4967,7 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, virDomainDefPtr def; virDomainDefPtr persistentDef; bool hotpluggable = !!(flags & VIR_DOMAIN_VCPU_HOTPLUGGABLE); + bool useAgent = !!(flags & VIR_DOMAIN_VCPU_GUEST); int ret = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -4963,13 +4982,14 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if ((!useAgent && qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) || + (useAgent && qemuDomainObjBeginAgentJob(driver, vm, QEMU_AGENT_JOB_MODIFY) < 0)) goto cleanup;
And here.
Actually no. This one is different to the previous two places. This one is either grab domain job OR agent job but not both at the same time (which is what previous places do).
Ah right, I misread qemuDomainObjBeginAgentJob as qemuDomainObjBeginJobWithAgent. In this case, I'd just modify the code a bit to make it clearer the two cases are mutually exclusive, e.g.: int rc; if (useAgent) rc = qemuDomainObjBeginAgentJob(); else rc = qemuDomainObjBeginJob(); if (rc < 0) goto cleanup; Jirka
participants (3)
-
Jiri Denemark -
Michal Privoznik -
Michal Prívozník