3:18 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2018-June/msg00546.html
diff to v1:
- Rename to STATS_NOWAIT
- John's review worked in
- added comment to qemuDomainObjBeginJobNowait
Michal Privoznik (5):
qemu_domain: Document qemuDomainObjBeginJob
qemuDomainObjBeginJobInternal: Remove spurious @ret assignment
qemu_domain: Introduce qemuDomainObjBeginJobNowait
Introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT
virsh: Introduce --nowait to domstats
include/libvirt/libvirt-domain.h | 2 ++
src/libvirt-domain.c | 12 ++++++++
src/qemu/qemu_domain.c | 60 +++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_domain.h | 4 +++
src/qemu/qemu_driver.c | 15 ++++++++--
tools/virsh-domain-monitor.c | 7 +++++
tools/virsh.pod | 16 +++++++----
7 files changed, 101 insertions(+), 15 deletions(-)
--
2.16.4
3:18 a.m.
New subject: [libvirt] [PATCH v2 1/5] qemu_domain: Document qemuDomainObjBeginJob
Provide a small comment on the function and its parameters.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
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 2119233907..be36a9e3e4 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6353,8 +6353,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 for a 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
3:18 a.m.
New subject: [libvirt] [PATCH v2 2/5] qemuDomainObjBeginJobInternal: Remove spurious @ret assignment
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>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
---
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 be36a9e3e4..21d54938b6 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6490,7 +6490,6 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
else
blocker = priv->job.asyncOwnerAPI;
- ret = -1;
if (errno == ETIMEDOUT) {
if (blocker) {
virReportError(VIR_ERR_OPERATION_TIMEOUT,
--
2.16.4
3:18 a.m.
New subject: [libvirt] [PATCH v2 3/5] qemu_domain: Introduce qemuDomainObjBeginJobNowait
The aim of this API is to allow the caller do best effort. Some
functions can work even when acquiring the 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 that
is processing some other job.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_domain.h | 4 ++++
2 files changed, 42 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 21d54938b6..a01067049e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6359,11 +6359,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob
job)
* @obj: domain object
* @job: qemuDomainJob to start
* @asyncJob: qemuDomainAsyncJob to start
+ * @nowait: don't wait trying to acquire @job
*
* Acquires job for a 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 @nowait is set.
+ * If @nowait is true this function tries to acquire job and if
+ * it fails, then it 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
@@ -6374,7 +6378,8 @@ static int ATTRIBUTE_NONNULL(1)
qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainJob job,
- qemuDomainAsyncJob asyncJob)
+ qemuDomainAsyncJob asyncJob,
+ bool nowait)
{
qemuDomainObjPrivatePtr priv = obj->privateData;
unsigned long long now;
@@ -6414,12 +6419,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
}
while (!nested && !qemuDomainNestedJobAllowed(priv, job)) {
+ if (nowait)
+ 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 (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;
@@ -6536,7 +6547,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;
@@ -6551,7 +6562,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv;
if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC,
- asyncJob) < 0)
+ asyncJob, false) < 0)
return -1;
priv = obj->privateData;
@@ -6580,9 +6591,31 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
return qemuDomainObjBeginJobInternal(driver, obj,
QEMU_JOB_ASYNC_NESTED,
- QEMU_ASYNC_JOB_NONE);
+ QEMU_ASYNC_JOB_NONE,
+ false);
}
+/**
+ * qemuDomainObjBeginJobNowait:
+ *
+ * @driver: qemu driver
+ * @obj: domain object
+ * @job: qemuDomainJob to start
+ *
+ * Acquires job for a domain object which must be locked before
+ * calling. If there's already a job running it returns
+ * immediately without any error reported.
+ *
+ * Returns: see qemuDomainObjBeginJobInternal
+ */
+int
+qemuDomainObjBeginJobNowait(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 fd8d9b5305..9e2da0a37c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -510,6 +510,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
virDomainObjPtr obj,
qemuDomainAsyncJob asyncJob)
ATTRIBUTE_RETURN_CHECK;
+int qemuDomainObjBeginJobNowait(virQEMUDriverPtr driver,
+ virDomainObjPtr obj,
+ qemuDomainJob job)
+ ATTRIBUTE_RETURN_CHECK;
void qemuDomainObjEndJob(virQEMUDriverPtr driver,
virDomainObjPtr obj);
--
2.16.4
6:25 p.m.
New subject: [libvirt] [PATCH v2 3/5] qemu_domain: Introduce qemuDomainObjBeginJobNowait
On 06/15/2018 04:18 AM, Michal Privoznik wrote:
The aim of this API is to allow the caller do best effort. Some
s/do/to do/
functions can work even when acquiring the 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 that
is processing some other job.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_domain.c | 43 ++++++++++++++++++++++++++++++++++++++-----
src/qemu/qemu_domain.h | 4 ++++
2 files changed, 42 insertions(+), 5 deletions(-)
Couple of nits noted, one above, one below...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 21d54938b6..a01067049e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -6359,11 +6359,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob
job)
* @obj: domain object
* @job: qemuDomainJob to start
* @asyncJob: qemuDomainAsyncJob to start
+ * @nowait: don't wait trying to acquire @job
*
* Acquires job for a 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 @nowait is set.
Paragraph break have a empty line for readability.
+ * If @nowait is true this function tries to acquire job and if
+ * it fails, then it 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
[...]
3:18 a.m.
New subject: [libvirt] [PATCH v2 4/5] Introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT
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 the job. This is problematic
to interactive applications that fetch stats repeatedly every few
seconds.
The solution is to introduce
VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag which tries to
acquire job but does not wait if acquiring failed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
include/libvirt/libvirt-domain.h | 2 ++
src/libvirt-domain.c | 12 ++++++++++++
src/qemu/qemu_driver.c | 15 ++++++++++++---
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 3ef7c24528..796f2e1408 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -2055,6 +2055,8 @@ 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_NOWAIT = 1 << 29, /* report statistics that
can be obtained
+ immediately without any
blocking */
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 4a899f31c8..c71f2e6877 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11502,6 +11502,12 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* fields for offline domains if the statistics are meaningful only for a
* running domain.
*
+ * Passing VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT in
+ * @flags means when libvirt is unable to fetch stats for any of
+ * the domains (for whatever reason) only a subset of statistics
+ * is returned for the domain. That subset being statistics that
+ * don't involve querying the underlying hypervisor.
+ *
* Similarly to virConnectListAllDomains, @flags can contain various flags to
* filter the list of domains to provide stats for.
*
@@ -11586,6 +11592,12 @@ virConnectGetAllDomainStats(virConnectPtr conn,
* fields for offline domains if the statistics are meaningful only for a
* running domain.
*
+ * Passing VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT in
+ * @flags means when libvirt is unable to fetch stats for any of
+ * the domains (for whatever reason) only a subset of statistics
+ * is returned for the domain. That subset being statistics that
+ * don't involve querying the underlying hypervisor.
+ *
* 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 42069ee617..35038889a3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -20428,6 +20428,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_NOWAIT |
VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING |
VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
@@ -20462,9 +20463,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_NOWAIT)
+ rv = qemuDomainObjBeginJobNowait(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
6:25 p.m.
New subject: [libvirt] [PATCH v2 4/5] Introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT
On 06/15/2018 04:18 AM, Michal Privoznik wrote:
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 the job. This is problematic
to interactive applications that fetch stats repeatedly every few
seconds.
The solution is to introduce
VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag which tries to
acquire job but does not wait if acquiring failed.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
include/libvirt/libvirt-domain.h | 2 ++
src/libvirt-domain.c | 12 ++++++++++++
src/qemu/qemu_driver.c | 15 ++++++++++++---
3 files changed, 26 insertions(+), 3 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Is the 30 seconds qemu specific? Could drop it from the commit message
- if you feel so compelled.
1:08 a.m.
New subject: [libvirt] [PATCH v2 4/5] Introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT
On 06/19/2018 01:25 AM, John Ferlan wrote:
On 06/15/2018 04:18 AM, Michal Privoznik wrote:
> 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 the job. This is problematic
> to interactive applications that fetch stats repeatedly every few
> seconds.
>
> The solution is to introduce
> VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag which tries to
> acquire job but does not wait if acquiring failed.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> include/libvirt/libvirt-domain.h | 2 ++
> src/libvirt-domain.c | 12 ++++++++++++
> src/qemu/qemu_driver.c | 15 ++++++++++++---
> 3 files changed, 26 insertions(+), 3 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Is the 30 seconds qemu specific? Could drop it from the commit message
- if you feel so compelled.
Yes & no. Each driver has its own timeout but all set it to 30 seconds:
#define LXC_JOB_WAIT_TIME (1000ull * 30)
#define LIBXL_JOB_WAIT_TIME (1000ull * 30)
#define VZ_JOB_WAIT_TIME (1000 * 30)
#define QEMU_JOB_WAIT_TIME (1000ull * 30)
Therefore I rather keep 30seconds in the commit message.
Michal
3:18 a.m.
New subject: [libvirt] [PATCH v2 5/5] virsh: Introduce --nowait to domstats
This new switch can be used to set
VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag for stats
fetching API.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
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..886f7f16b5 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 = "nowait",
+ .type = VSH_OT_BOOL,
+ .help = N_("report only stats that are accessible instantly"),
+ },
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, "nowait"))
+ flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT;
+
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..7cb8c8a6e4 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<--nowait>]
+[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<--nowait> 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
6:25 p.m.
New subject: [libvirt] [PATCH v2 5/5] virsh: Introduce --nowait to domstats
On 06/15/2018 04:18 AM, Michal Privoznik wrote:
This new switch can be used to set
VIR_CONNECT_GET_ALL_DOMAINS_STATS_NOWAIT flag for stats
fetching API.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
tools/virsh-domain-monitor.c | 7 +++++++
tools/virsh.pod | 16 +++++++++++-----
2 files changed, 18 insertions(+), 5 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2348
days inactive
2352
days old
9 comments
3 participants
participants (3)
-
John Ferlan -
Michal Privoznik -
Michal Prívozník