Provide a small comment on the function and its parameters. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c5237e4d41..97149613a2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6334,8 +6334,22 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) /* Give up waiting for mutex after 30 seconds */ #define QEMU_JOB_WAIT_TIME (1000ull * 30) -/* - * obj must be locked before calling +/** + * qemuDomainObjBeginJobInternal: + * @driver: qemu driver + * @obj: domain object + * @job: qemuDomainJob to start + * @asyncJob: qemuDomainAsyncJob to start + * + * Acquires job over domain object which must be locked before + * calling. If there's already a job running waits up to + * QEMU_JOB_WAIT_TIME after which the functions fails reporting + * an error. + * + * Returns: 0 on success, + * -2 if unable to start job because of timeout or + * maxQueuedJobs limit, + * -1 otherwise. */ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, -- 2.16.4

The variable is initialized to -1 already. There's no way it can be overwritten by the time control gets to the line I'm removing. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 97149613a2..5273ab56ac 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6471,7 +6471,6 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, else blocker = priv->job.asyncOwnerAPI; - ret = -1; if (errno == ETIMEDOUT) { if (blocker) { virReportError(VIR_ERR_OPERATION_TIMEOUT, -- 2.16.4

The aim of this API is to allow caller do best effort. Some functions of ours can work even when acquiring job fails (e.g. qemuConnectGetAllDomainStats()). But what they can't bear is delay if they have to wait up to 30 seconds for each domain. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5273ab56ac..9657573342 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) * @obj: domain object * @job: qemuDomainJob to start * @asyncJob: qemuDomainAsyncJob to start + * @instant: don't wait trying to acquire @job * * Acquires job over domain object which must be locked before * calling. If there's already a job running waits up to * QEMU_JOB_WAIT_TIME after which the functions fails reporting - * an error. + * an error unless @instant is set. + * If @instant is true this function tries to acquire job and if + * it fails returns immediately without waiting. No error is + * reported in this case. * * Returns: 0 on success, * -2 if unable to start job because of timeout or @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainJob job, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, + bool instant) { qemuDomainObjPrivatePtr priv = obj->privateData; unsigned long long now; @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, } while (!nested && !qemuDomainNestedJobAllowed(priv, job)) { + if (instant) + goto cleanup; + VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0) goto error; } while (priv->job.active) { + if (instant) + 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; @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, qemuDomainJob job) { if (qemuDomainObjBeginJobInternal(driver, obj, job, - QEMU_ASYNC_JOB_NONE) < 0) + QEMU_ASYNC_JOB_NONE, false) < 0) return -1; else return 0; @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv; if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC, - asyncJob) < 0) + asyncJob, false) < 0) return -1; priv = obj->privateData; @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver, return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC_NESTED, - QEMU_ASYNC_JOB_NONE); + QEMU_ASYNC_JOB_NONE, + false); } +int +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) +{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_ASYNC_JOB_NONE, true); +} /* * obj must be locked and have a reference before calling diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f17157b951..4b63c00dff 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -512,6 +512,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainAsyncJob asyncJob) ATTRIBUTE_RETURN_CHECK; +int qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) + ATTRIBUTE_RETURN_CHECK; void qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj); -- 2.16.4

On 06/13/2018 05:28 PM, John Ferlan wrote: Okay. Naming is hard :-P Of course. For instance, if there's an async job running and another thread is trying to start a sync, non-nested job that is not allowed for the async job. Say, thread A is doing a snapshot which sets job mask (=allowed sync jobs) to: query, destroy, abort, suspend, migration-op. Then there's a thread B which is executing qemuDomainSetUserPassword(). This threads tries to grab modify job. However, since grabbing modify is not allowed (due to job mask) thread B sits and waits until thread A releases the async job. At that point, thread B can safely grab modify job because there's no other (nor async) job running. Long story short, not only synchronous jobs serialize, also async (and any combination of those two) do. I'm not quite sure what do you mean. You mean whether grabbing a sync job while there's async job already running? This is supported. The fact that asyncJob is QEMU_ASYNC_JOB_NONE does not mean "set sync job to @job and async job to QEMU_ASYNC_JOB_NONE". In fact, qemuDomainObjBeginJobInternal() is capable of grabbing either sync or async job but not both at the same time. When grabbing a sync job, @asynJob == QEMU_ASYNC_JOB_NONE and when grabbing an async job, @job == QEMU_JOB_ASYNC. Anyway, the point of BeginJobInstant() is to be drop-in replacement for plain BeginJob(), so whatever the latter passes to BeginJobInternal() the former should mimic it. But then again, I'm not quite sure I understand what you mean. Michal

https://bugzilla.redhat.com/show_bug.cgi?id=1552092 If there's a long running job it might cause us to wait 30 seconds before we give up acquiring job. This may cause trouble to interactive applications that fetch stats repeatedly every few seconds. Solution is to introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT flag which tries to acquire job but does not wait if acquiring failed. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 10 ++++++++++ src/qemu/qemu_driver.c | 15 ++++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index da773b76cb..1a1d34620d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -2055,6 +2055,7 @@ typedef enum { VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = VIR_CONNECT_LIST_DOMAINS_SHUTOFF, VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER, + VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT = 1 << 29, /* ignore stalled domains */ VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING = 1 << 30, /* include backing chain for block stats */ VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1U << 31, /* enforce requested stats */ } virConnectGetAllDomainStatsFlags; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index d44b553c74..906b097adb 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -11502,6 +11502,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn, * fields for offline domains if the statistics are meaningful only for a * running domain. * + * Passing VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT in + * @flags means when libvirt is unable to fetch stats for any of + * the domains (for whatever reason) such domain is silently + * ignored. + * * Similarly to virConnectListAllDomains, @flags can contain various flags to * filter the list of domains to provide stats for. * @@ -11586,6 +11591,11 @@ virConnectGetAllDomainStats(virConnectPtr conn, * fields for offline domains if the statistics are meaningful only for a * running domain. * + * Passing VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT in + * @flags means when libvirt is unable to fetch stats for any of + * the domains (for whatever reason) such domain is silently + * ignored. + * * Note that any of the domain list filtering flags in @flags may be rejected * by this function. * diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 38ea865ce3..28338de05f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20446,6 +20446,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE | VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT | VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE | + VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT | VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING | VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1); @@ -20480,9 +20481,17 @@ qemuConnectGetAllDomainStats(virConnectPtr conn, virObjectLock(vm); - if (HAVE_JOB(privflags) && - qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) == 0) - domflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + if (HAVE_JOB(privflags)) { + int rv; + + if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT) + rv = qemuDomainObjBeginJobInstant(driver, vm, QEMU_JOB_QUERY); + else + rv = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY); + + if (rv == 0) + domflags |= QEMU_DOMAIN_STATS_HAVE_JOB; + } /* else: without a job it's still possible to gather some data */ if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING) -- 2.16.4

On 06/13/2018 05:34 PM, John Ferlan wrote: Well, my hidden idea was also that we can "misuse" this flag to not wait on other places too. For instance, if we find out (somehow) that a domain is in D state, we would consider it stale even without any job running on it. Okay, we have no way of detecting if qemu is in D state right now, but you get my point. If we don't lock this flag down to just domain jobs (that not all drivers have btw), we can use it more widely. Not really, this is jut git generating misleading diff (for human). In fact this flag is added to virConnectGetAllDomainStats() and virDomainListGetStats(). OKay. Sure. Because what virDomainListGetStats does is it calls driver->connectGetAllDomainStats() just like virConnectGetAllDomainStats() does. So at the driver side there's only one function serving two public APIs. Not sure what do you mean. Any StatsWorker callback (from qemuDomainGetStats()) that needs to talk to monitor checks if grabbing job succeeded or not. If it didn't the callback returns immediately. You can see this live - just put sleep(60) right at the beginning of qemuAgentGetTime(), and then from one console run 'virsh domtime $dom' and from another one 'virsh domstats'. unfinished sen... :-) Probably yeah. But that can be done outside of this patch set. Michal

On 06/14/2018 04:10 PM, John Ferlan wrote: I thought about this and I don't think so. So current behaviour is: with enforce you still might miss some stats if acquiring job fails. ENFORCE merely tells libvirt to check if requested stats are available (=driver knows how to fetch requested stats). Therefore I don't think these two flags are mutually exclusive. Yup. But again, this is pre-existing and deserves a separate patch. I can send it and (to motivate others to merge these) - I will do that once these are in as a follow up ;-) :-D Michal

On 06/14/2018 05:35 PM, Daniel P. Berrangé wrote: Okay, works for me. I'll post v2 shortly. This is clever idea. But qemu is not the only source of stats we gather. We also fetch some data from CGroups, /proc, ovs bridge, etc. So libvirt would need to add its own stats for client to see. This means there will be a function that updates the shared mem every so often (as client tells us via some new API?). The same goes for qemu impl. Now imagine two clients wanting two different refresh rates with GCD 1 :-) Michal

This new switch can be used to set VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT flag for stats fetching API. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- tools/virsh-domain-monitor.c | 7 +++++++ tools/virsh.pod | 16 +++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8cbb3db37c..96a65a8ac7 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1992,6 +1992,10 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("add backing chain information to block stats"), }, + {.name = "best-effort", + .type = VSH_OT_BOOL, + .help = N_("ignore stalled domains"), + }, VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(N_("list of domains to get stats for"), 0), {.name = NULL} }; @@ -2087,6 +2091,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "backing")) flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING; + if (vshCommandOptBool(cmd, "best-effort")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT; + if (vshCommandOptBool(cmd, "domain")) { if (VIR_ALLOC_N(domlist, 1) < 0) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 3f3314a87e..8f3bc67405 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -968,11 +968,11 @@ that require a block device name (such as I<domblkinfo> or I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command. -=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--state>] -[I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] [I<--block>] -[I<--perf>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] -[I<--list-transient>] [I<--list-running>] [I<--list-paused>] -[I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] +=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--best-effort>] +[I<--state>] [I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] +[I<--block>] [I<--perf>] [[I<--list-active>] [I<--list-inactive>] +[I<--list-persistent>] [I<--list-transient>] [I<--list-running>] +[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] Get statistics for multiple or all domains. Without any argument this command prints all available statistics for all domains. @@ -1123,6 +1123,12 @@ daemon supports the selected group of stats. Flag I<--enforce> forces the command to fail if the daemon doesn't support the selected group. +When collecting stats libvirtd may wait for some time if there's +already another job running on given domain for it to finish. +This may cause unnecessary delay in delivering stats. Using +I<--best-effort> suppresses this behaviour. On the other hand +some statistics might be missing for such domain. + =item B<domiflist> I<domain> [I<--inactive>] Print a table showing the brief information of all virtual interfaces -- 2.16.4