2:14 a.m.
This is in response to my earlier v2 patch attempt at exposing
qcow2 allocation watermarks during a block commit (although that
series attempted to expand virDomainGetXMLDesc, while this one
expands virDomainListGetStats instead):
https://www.redhat.com/archives/libvir-list/2014-November/msg00589.html
It is also a followup to my preliminiaries (where I had review
comments to split patch 2/2, now 3 separate patches in this series):
https://www.redhat.com/archives/libvir-list/2014-November/msg00932.html
There's enough new content here that I didn't know whether to call
this a v3 of the original approach, but as the original approach was
rejected on design without much review, it feels like this is a new
series, even if a couple of patches from the v2 series ended up in
this series in a slightly different form.
See patch 12 for a sample of the planned output; I still have a few
more patches to polish to properly get qemu JSON output wired into
the command for correct values, but this should at least be sufficient
to write up client code that interacts with the interface.
The patches can also be grabbed from:
git fetch git://repo.or.cz/libvirt/ericb.git getstats
or browsed at:
http://repo.or.cz/w/libvirt/ericb.git/tree/refs/heads/getstats
Eric Blake (12):
getstats: avoid memory leak on OOM
getstats: improve documentation
qemu: refactor blockinfo job handling
qemu: let blockinfo reuse virStorageSource
qemu: refactor blockinfo data gathering
getstats: start giving offline block stats
getstats: report block sizes for offline domains
getstats: add block.n.path stat
getstats: prepare for dynamic block.count stat
getstats: add new flag for block backing chain
getstats: split block stats reporting for easier recursion
getstats: start crawling backing chain for qemu
include/libvirt/libvirt-domain.h | 1 +
src/libvirt-domain.c | 24 ++-
src/qemu/qemu_driver.c | 415 ++++++++++++++++++++++++++-------------
src/util/virstoragefile.h | 3 +-
tools/virsh-domain-monitor.c | 7 +
tools/virsh.pod | 10 +-
6 files changed, 315 insertions(+), 145 deletions(-)
--
1.9.3
2:14 a.m.
New subject: [libvirt] [PATCH 01/12] getstats: avoid memory leak on OOM
qemuDomainGetStatsBlock() could leak a stats hash table if it
encountered OOM while populating the virTypedParameters.
Oddly, the fix doesn't even touch qemuDomainGetStatsBlock :)
* src/qemu/qemu_driver.c (QEMU_ADD_COUNT_PARAM)
(QEMU_ADD_NAME_PARAM): Don't return early.
(qemuDomainGetStatsInterface): Adjust caller.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9152cf5..a7b208f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18371,7 +18371,7 @@ do { \
maxparams, \
param_name, \
count) < 0) \
- return -1; \
+ goto cleanup; \
} while (0)
#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
@@ -18384,7 +18384,7 @@ do { \
maxparams, \
param_name, \
name) < 0) \
- return -1; \
+ goto cleanup; \
} while (0)
#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
@@ -18448,6 +18448,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
"tx.drop", tmp.tx_drop);
}
+ cleanup:
return 0;
}
--
1.9.3
4:50 a.m.
New subject: [libvirt] [PATCH 01/12] getstats: avoid memory leak on OOM
On 12/06/14 09:14, Eric Blake wrote:
qemuDomainGetStatsBlock() could leak a stats hash table if it
encountered OOM while populating the virTypedParameters.
Oddly, the fix doesn't even touch qemuDomainGetStatsBlock :)
* src/qemu/qemu_driver.c (QEMU_ADD_COUNT_PARAM)
(QEMU_ADD_NAME_PARAM): Don't return early.
(qemuDomainGetStatsInterface): Adjust caller.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9152cf5..a7b208f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18371,7 +18371,7 @@ do { \
maxparams, \
param_name, \
count) < 0) \
- return -1; \
+ goto cleanup; \
} while (0)
#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
@@ -18384,7 +18384,7 @@ do { \
maxparams, \
param_name, \
name) < 0) \
- return -1; \
+ goto cleanup; \
} while (0)
#define QEMU_ADD_NET_PARAM(record, maxparams, num, name, value) \
@@ -18448,6 +18448,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
"tx.drop", tmp.tx_drop);
}
+ cleanup:
With this change qemuDomainGetStatsInterface will return success even in
case when the OOM occurred. You need to add a 'ret' variable set to -1
and set it to 0 just before cleanup, so that the error gets propagated.
return 0;
}
ACK if you keep reporting the error in OOM case in
qemuDomainGetStatsInterface()
Peter
10:44 a.m.
New subject: [libvirt] [PATCH 01/12] getstats: avoid memory leak on OOM
On 12/08/2014 03:50 AM, Peter Krempa wrote:
On 12/06/14 09:14, Eric Blake wrote:
> qemuDomainGetStatsBlock() could leak a stats hash table if it
> encountered OOM while populating the virTypedParameters.
> Oddly, the fix doesn't even touch qemuDomainGetStatsBlock :)
> + cleanup:
With this change qemuDomainGetStatsInterface will return success even in
case when the OOM occurred. You need to add a 'ret' variable set to -1
and set it to 0 just before cleanup, so that the error gets propagated.
Oh right :)
ACK if you keep reporting the error in OOM case in
qemuDomainGetStatsInterface()
Done and pushed:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index a7b208f..ed8e140 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -18409,6 +18409,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr
driver ATTRIBUTE_UNUSED,
{
size_t i;
struct _virDomainInterfaceStats tmp;
+ int ret = -1;
if (!virDomainObjIsActive(dom))
return 0;
@@ -18448,8 +18449,9 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr
driver ATTRIBUTE_UNUSED,
"tx.drop", tmp.tx_drop);
}
+ ret = 0;
cleanup:
- return 0;
+ return ret;
}
#undef QEMU_ADD_NET_PARAM
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:14 a.m.
New subject: [libvirt] [PATCH 02/12] getstats: improve documentation
At least with 'virsh domstats --block' on an offline domain, we
currently output no stats even though we recognize the stat
category. Although a later patch will improve this situation,
it is better to document that this is expected behavior.
Also, while the current implementation rejects filtering flags
for virDomainListGetStats, this limitation may be lifted in the
future and we do not enforce it at the API level.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
that recognized stats might not be reported.
(virDomainListGetStats): Likewise, and tweak filtering documentation.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/libvirt-domain.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 8333183..87123d1 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10940,7 +10940,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
*
* Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
* the function return error in case some of the stat types in @stats were
- * not recognized by the daemon.
+ * not recognized by the daemon. However, even with this flag, a hypervisor
+ * may omit individual fields within a known group if the information is not
+ * available; as an extreme example, a supported group may produce zero
+ * fields for offline domains if the statistics are meaningful only for a
+ * running domain.
*
* Similarly to virConnectListAllDomains, @flags can contain various flags to
* filter the list of domains to provide stats for.
@@ -11020,9 +11024,13 @@ virConnectGetAllDomainStats(virConnectPtr conn,
*
* Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
* the function return error in case some of the stat types in @stats were
- * not recognized by the daemon.
+ * not recognized by the daemon. However, even with this flag, a hypervisor
+ * may omit individual fields within a known group if the information is not
+ * available; as an extreme example, a supported group may produce zero
+ * fields for offline domains if the statistics are meaningful only for a
+ * running domain.
*
- * Note that any of the domain list filtering flags in @flags will be rejected
+ * Note that any of the domain list filtering flags in @flags may be rejected
* by this function.
*
* Returns the count of returned statistics structures on success, -1 on error.
--
1.9.3
4:57 a.m.
New subject: [libvirt] [PATCH 02/12] getstats: improve documentation
On 12/06/14 09:14, Eric Blake wrote:
At least with 'virsh domstats --block' on an offline domain,
we
currently output no stats even though we recognize the stat
category. Although a later patch will improve this situation,
it is better to document that this is expected behavior.
Also, while the current implementation rejects filtering flags
for virDomainListGetStats, this limitation may be lifted in the
future and we do not enforce it at the API level.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
that recognized stats might not be reported.
(virDomainListGetStats): Likewise, and tweak filtering documentation.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/libvirt-domain.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
ACK,
Peter
2:14 a.m.
New subject: [libvirt] [PATCH 03/12] qemu: refactor blockinfo job handling
In order for a future patch to virDomainListGetStats to reuse
some code for determining disk usage of offline domains, we
need to make it easier to pull out part of the guts of grabbing
blockinfo. The current implementation grabs a job fairly late
in the game, while getstats will already own a job; reordering
things so that the job is always grabbed up front in both
functions will make it easier to pull out the common code.
This patch results in grabbing a job in cases where one was not
previously needed, but as it is a query job, it should not be
noticeably slower.
This patch touches the same code as the fix for CVE-2014-6458
(commit b799259); in that patch, we avoided hotplug changing
a disk reference during the time of obtaining a monitor lock
by copying all data we needed and no longer referencing disk;
this patch goes the other way and ensures that by holding the
job, the disk cannot be changed so we no longer need to worry
about the disk being invalidated across the monitor lock.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Rearrange job
control to be outside of disk information.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 62 +++++++++++++++++++++++---------------------------
1 file changed, 29 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a7b208f..ae4485a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10999,7 +10999,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
int format;
bool activeFail = false;
virQEMUDriverConfigPtr cfg = NULL;
- char *alias = NULL;
char *buf = NULL;
ssize_t len;
@@ -11018,11 +11017,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto cleanup;
}
+ /* Technically, we only need a job if we are going to query the
+ * monitor, which is only for active domains that are using
+ * non-raw block devices. But it is easier to share code if we
+ * always grab a job; furthermore, grabbing the job ensures that
+ * hot-plug won't change disk behind our backs. */
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+ goto cleanup;
+
/* Check the path belongs to this domain. */
if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
_("invalid path %s not assigned to domain"), path);
- goto cleanup;
+ goto endjob;
}
disk = vm->def->disks[idx];
@@ -11032,36 +11039,36 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source
assigned"),
path);
- goto cleanup;
+ goto endjob;
}
if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY,
NULL, NULL)) == -1)
- goto cleanup;
+ goto endjob;
if (fstat(fd, &sb) < 0) {
virReportSystemError(errno,
_("cannot stat file '%s'"),
disk->src->path);
- goto cleanup;
+ goto endjob;
}
if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
virReportSystemError(errno, _("cannot read header '%s'"),
disk->src->path);
- goto cleanup;
+ goto endjob;
}
} else {
if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
- goto cleanup;
+ goto endjob;
if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
&buf)) < 0)
- goto cleanup;
+ goto endjob;
if (virStorageFileStat(disk->src, &sb) < 0) {
virReportSystemError(errno, _("failed to stat remote file
'%s'"),
NULLSTR(disk->src->path));
- goto cleanup;
+ goto endjob;
}
}
@@ -11073,17 +11080,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("no disk format for %s and probing is disabled"),
path);
- goto cleanup;
+ goto endjob;
}
if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
buf, len)) < 0)
- goto cleanup;
+ goto endjob;
}
if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
format, NULL)))
- goto cleanup;
+ goto endjob;
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
@@ -11105,7 +11112,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (end == (off_t)-1) {
virReportSystemError(errno,
_("failed to seek to end of %s"), path);
- goto cleanup;
+ goto endjob;
}
info->physical = end;
info->capacity = end;
@@ -11133,35 +11140,24 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (!virDomainObjIsActive(vm)) {
activeFail = true;
ret = 0;
- goto cleanup;
+ goto endjob;
}
- if (VIR_STRDUP(alias, disk->info.alias) < 0)
- goto cleanup;
+ qemuDomainObjEnterMonitor(driver, vm);
+ ret = qemuMonitorGetBlockExtent(priv->mon,
+ disk->info.alias,
+ &info->allocation);
+ qemuDomainObjExitMonitor(driver, vm);
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
- goto cleanup;
-
- if (virDomainObjIsActive(vm)) {
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetBlockExtent(priv->mon,
- alias,
- &info->allocation);
- qemuDomainObjExitMonitor(driver, vm);
- } else {
- activeFail = true;
- ret = 0;
- }
-
- if (!qemuDomainObjEndJob(driver, vm))
- vm = NULL;
} else {
ret = 0;
}
+ endjob:
+ if (!qemuDomainObjEndJob(driver, vm))
+ vm = NULL;
cleanup:
VIR_FREE(buf);
- VIR_FREE(alias);
virStorageSourceFree(meta);
VIR_FORCE_CLOSE(fd);
if (disk)
--
1.9.3
5:41 a.m.
New subject: [libvirt] [PATCH 03/12] qemu: refactor blockinfo job handling
On 12/06/2014 03:14 AM, Eric Blake wrote:
In order for a future patch to virDomainListGetStats to reuse
some code for determining disk usage of offline domains, we
need to make it easier to pull out part of the guts of grabbing
blockinfo. The current implementation grabs a job fairly late
in the game, while getstats will already own a job; reordering
things so that the job is always grabbed up front in both
functions will make it easier to pull out the common code.
This patch results in grabbing a job in cases where one was not
previously needed, but as it is a query job, it should not be
noticeably slower.
This patch touches the same code as the fix for CVE-2014-6458
(commit b799259); in that patch, we avoided hotplug changing
a disk reference during the time of obtaining a monitor lock
by copying all data we needed and no longer referencing disk;
this patch goes the other way and ensures that by holding the
job, the disk cannot be changed so we no longer need to worry
about the disk being invalidated across the monitor lock.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Rearrange job
control to be outside of disk information.
Ran thru my Coverity checker...
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 62 +++++++++++++++++++++++---------------------------
1 file changed, 29 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a7b208f..ae4485a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10999,7 +10999,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
int format;
bool activeFail = false;
virQEMUDriverConfigPtr cfg = NULL;
- char *alias = NULL;
char *buf = NULL;
ssize_t len;
@@ -11018,11 +11017,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto cleanup;
}
+ /* Technically, we only need a job if we are going to query the
+ * monitor, which is only for active domains that are using
+ * non-raw block devices. But it is easier to share code if we
+ * always grab a job; furthermore, grabbing the job ensures that
+ * hot-plug won't change disk behind our backs. */
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+ goto cleanup;
+
/* Check the path belongs to this domain. */
if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
virReportError(VIR_ERR_INVALID_ARG,
_("invalid path %s not assigned to domain"), path);
- goto cleanup;
+ goto endjob;
}
disk = vm->def->disks[idx];
@@ -11032,36 +11039,36 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source
assigned"),
path);
- goto cleanup;
+ goto endjob;
}
if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY,
NULL, NULL)) == -1)
- goto cleanup;
+ goto endjob;
if (fstat(fd, &sb) < 0) {
virReportSystemError(errno,
_("cannot stat file '%s'"),
disk->src->path);
- goto cleanup;
+ goto endjob;
}
if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
virReportSystemError(errno, _("cannot read header '%s'"),
disk->src->path);
- goto cleanup;
+ goto endjob;
}
} else {
if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
- goto cleanup;
+ goto endjob;
if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
&buf)) < 0)
- goto cleanup;
+ goto endjob;
if (virStorageFileStat(disk->src, &sb) < 0) {
virReportSystemError(errno, _("failed to stat remote file
'%s'"),
NULLSTR(disk->src->path));
- goto cleanup;
+ goto endjob;
}
}
@@ -11073,17 +11080,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("no disk format for %s and probing is
disabled"),
path);
- goto cleanup;
+ goto endjob;
}
if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
buf, len)) < 0)
- goto cleanup;
+ goto endjob;
}
if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
format, NULL)))
- goto cleanup;
+ goto endjob;
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
@@ -11105,7 +11112,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (end == (off_t)-1) {
virReportSystemError(errno,
_("failed to seek to end of %s"), path);
- goto cleanup;
+ goto endjob;
}
info->physical = end;
info->capacity = end;
@@ -11133,35 +11140,24 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (!virDomainObjIsActive(vm)) {
activeFail = true;
ret = 0;
- goto cleanup;
+ goto endjob;
}
- if (VIR_STRDUP(alias, disk->info.alias) < 0)
- goto cleanup;
+ qemuDomainObjEnterMonitor(driver, vm);
+ ret = qemuMonitorGetBlockExtent(priv->mon,
+ disk->info.alias,
+ &info->allocation);
+ qemuDomainObjExitMonitor(driver, vm);
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
- goto cleanup;
-
- if (virDomainObjIsActive(vm)) {
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetBlockExtent(priv->mon,
- alias,
- &info->allocation);
- qemuDomainObjExitMonitor(driver, vm);
- } else {
- activeFail = true;
- ret = 0;
- }
-
- if (!qemuDomainObjEndJob(driver, vm))
- vm = NULL;
} else {
ret = 0;
}
+ endjob:
+ if (!qemuDomainObjEndJob(driver, vm))
+ vm = NULL;
This makes a virObjectUnlock(vm); in the following cleanup section
unhappy...
11215 if (!qemuDomainObjEndJob(driver, vm))
(18) Event assign_zero: Assigning: "vm" = "NULL".
Also see events: [var_deref_model]
11216 vm = NULL;
cleanup:
VIR_FREE(buf);
- VIR_FREE(alias);
virStorageSourceFree(meta);
VIR_FORCE_CLOSE(fd);
if (disk)
11225 }
(21) Event var_deref_model: Passing null pointer "vm" to
"virObjectUnlock", which dereferences it. (The dereference is assumed on
the basis of the 'nonnull' parameter attribute.)
Also see events: [assign_zero]
11226 virObjectUnlock(vm);
2:14 a.m.
New subject: [libvirt] [PATCH 04/12] qemu: let blockinfo reuse virStorageSource
Right now, grabbing blockinfo always calls stat on the disk, then
opens the image to determine the capacity, using a throw-away
virStorageSourcePtr. This has a couple of drawbacks:
1. We are calling stat and opening a file on every invocation of
the API. However, there are cases where the stats should NOT be
changing between successive calls (if a domain is running, no
one should be changing the physical size of a block device or raw
image behind our backs; capacity of read-only files should not
be changing; and we are the gateway to the block-resize command
to know when the capacity of read-write files should be changing).
True, we still have to use stat in some cases (a sparse raw file
changes allocation if it is read-write and the amount of holes is
changing, and a read-write qcow2 image stored in a file changes
physical size if it was not fully pre-allocated). But for
read-only images, even this should be something we can remember
from the previous time, rather than repeating every call.
2. We want to enhance the power of virDomainListGetStats, by
sharing code. But we already have a virStorageSourcePtr for
each disk, and it would be easier to reuse the common structure
than to have to worry about the one-off virDomainBlockInfoPtr.
While this patch does not optimize reuse of information in point
1, it does get us closer to being able to do so; by updating a
structure that survives between consecutive calls.
* src/util/virstoragefile.h (_virStorageSource): Add physical, to
mirror virDomainBlockInfo.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into
storage source, then copy to block info.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++--------
src/util/virstoragefile.h | 3 ++-
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae4485a..e873362 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11034,6 +11034,26 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
disk = vm->def->disks[idx];
+ /* FIXME: For an offline domain, we always want to check current
+ * on-disk statistics (as users have been known to change offline
+ * images behind our backs). For a running domain, however, it
+ * would be nice to avoid opening a file (particularly since
+ * reading a file while qemu is writing it risks the reader seeing
+ * bogus data), or even avoid a stat, if the information
+ * remembered from the prevoius run is still viable.
+ *
+ * For read-only disks, nothing should be changing unless the user
+ * has requested a block-commit action. For read-write disks, we
+ * know some special cases: capacity should not change without a
+ * block-resize (where capacity is the only stat that requires
+ * opening a file, and even then, only for non-raw files); and
+ * physical size of a raw image or of a block device should
+ * likewise not be changing without block-resize. On the other
+ * hand, allocation of a raw file can change (if the file is
+ * sparse, but the amount of sparseness changes due to writes or
+ * punching holes), and physical size of a non-raw file can
+ * change.
+ */
if (virStorageSourceIsLocalStorage(disk->src)) {
if (!disk->src->path) {
virReportError(VIR_ERR_INVALID_ARG,
@@ -11095,15 +11115,15 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- info->physical = (unsigned long long)sb.st_blocks *
+ disk->src->physical = (unsigned long long)sb.st_blocks *
(unsigned long long)DEV_BSIZE;
#else
- info->physical = sb.st_size;
+ disk->src->physical = sb.st_size;
#endif
/* Regular files may be sparse, so logical size (capacity) is not same
* as actual physical above
*/
- info->capacity = sb.st_size;
+ disk->src->capacity = sb.st_size;
} else {
/* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
* be 64 bits on all platforms.
@@ -11114,17 +11134,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
_("failed to seek to end of %s"), path);
goto endjob;
}
- info->physical = end;
- info->capacity = end;
+ disk->src->physical = end;
+ disk->src->capacity = end;
}
/* If the file we probed has a capacity set, then override
* what we calculated from file/block extents */
if (meta->capacity)
- info->capacity = meta->capacity;
+ disk->src->capacity = meta->capacity;
/* Set default value .. */
- info->allocation = info->physical;
+ disk->src->allocation = disk->src->physical;
/* ..but if guest is not using raw disk format and on a block device,
* then query highest allocated extent from QEMU
@@ -11146,13 +11166,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockExtent(priv->mon,
disk->info.alias,
- &info->allocation);
+ &disk->src->allocation);
qemuDomainObjExitMonitor(driver, vm);
} else {
ret = 0;
}
+ if (ret == 0) {
+ info->capacity = disk->src->capacity;
+ info->allocation = disk->src->allocation;
+ info->physical = disk->src->physical;
+ }
+
endjob:
if (!qemuDomainObjEndJob(driver, vm))
vm = NULL;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index e05b843..b4c3808 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -257,8 +257,9 @@ struct _virStorageSource {
virStoragePermsPtr perms;
virStorageTimestampsPtr timestamps;
- unsigned long long allocation; /* in bytes, 0 if unknown */
unsigned long long capacity; /* in bytes, 0 if unknown */
+ unsigned long long allocation; /* in bytes, 0 if unknown */
+ unsigned long long physical; /* in bytes, 0 if unknown */
size_t nseclabels;
virSecurityDeviceLabelDefPtr *seclabels;
--
1.9.3
7:03 a.m.
New subject: [libvirt] [PATCH 04/12] qemu: let blockinfo reuse virStorageSource
On 12/06/14 09:14, Eric Blake wrote:
Right now, grabbing blockinfo always calls stat on the disk, then
opens the image to determine the capacity, using a throw-away
virStorageSourcePtr. This has a couple of drawbacks:
1. We are calling stat and opening a file on every invocation of
the API. However, there are cases where the stats should NOT be
changing between successive calls (if a domain is running, no
one should be changing the physical size of a block device or raw
image behind our backs; capacity of read-only files should not
be changing; and we are the gateway to the block-resize command
to know when the capacity of read-write files should be changing).
True, we still have to use stat in some cases (a sparse raw file
changes allocation if it is read-write and the amount of holes is
changing, and a read-write qcow2 image stored in a file changes
physical size if it was not fully pre-allocated). But for
read-only images, even this should be something we can remember
from the previous time, rather than repeating every call.
2. We want to enhance the power of virDomainListGetStats, by
sharing code. But we already have a virStorageSourcePtr for
each disk, and it would be easier to reuse the common structure
than to have to worry about the one-off virDomainBlockInfoPtr.
While this patch does not optimize reuse of information in point
1, it does get us closer to being able to do so; by updating a
structure that survives between consecutive calls.
* src/util/virstoragefile.h (_virStorageSource): Add physical, to
mirror virDomainBlockInfo.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Store into
storage source, then copy to block info.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++++++++++++++--------
src/util/virstoragefile.h | 3 ++-
2 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ae4485a..e873362 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11034,6 +11034,26 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
disk = vm->def->disks[idx];
+ /* FIXME: For an offline domain, we always want to check current
+ * on-disk statistics (as users have been known to change offline
+ * images behind our backs). For a running domain, however, it
+ * would be nice to avoid opening a file (particularly since
+ * reading a file while qemu is writing it risks the reader seeing
+ * bogus data), or even avoid a stat, if the information
+ * remembered from the prevoius run is still viable.
+ *
+ * For read-only disks, nothing should be changing unless the user
+ * has requested a block-commit action. For read-write disks, we
+ * know some special cases: capacity should not change without a
+ * block-resize (where capacity is the only stat that requires
+ * opening a file, and even then, only for non-raw files); and
+ * physical size of a raw image or of a block device should
+ * likewise not be changing without block-resize. On the other
+ * hand, allocation of a raw file can change (if the file is
+ * sparse, but the amount of sparseness changes due to writes or
+ * punching holes), and physical size of a non-raw file can
+ * change.
For a live VM we should grab all of the above directly from the monitor
and not ever touch the files on the disk. We do that already for the
bulk stats and for getting the right size when doing storage migration.
This function unfortunately is legacy code compared to the stuff I've
pointed out
+ */
if (virStorageSourceIsLocalStorage(disk->src)) {
if (!disk->src->path) {
virReportError(VIR_ERR_INVALID_ARG,
@@ -11095,15 +11115,15 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- info->physical = (unsigned long long)sb.st_blocks *
+ disk->src->physical = (unsigned long long)sb.st_blocks *
(unsigned long long)DEV_BSIZE;
#else
- info->physical = sb.st_size;
+ disk->src->physical = sb.st_size;
#endif
/* Regular files may be sparse, so logical size (capacity) is not same
* as actual physical above
*/
- info->capacity = sb.st_size;
+ disk->src->capacity = sb.st_size;
} else {
/* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
* be 64 bits on all platforms.
@@ -11114,17 +11134,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
_("failed to seek to end of %s"), path);
goto endjob;
}
- info->physical = end;
- info->capacity = end;
+ disk->src->physical = end;
+ disk->src->capacity = end;
}
/* If the file we probed has a capacity set, then override
* what we calculated from file/block extents */
if (meta->capacity)
- info->capacity = meta->capacity;
+ disk->src->capacity = meta->capacity;
/* Set default value .. */
- info->allocation = info->physical;
+ disk->src->allocation = disk->src->physical;
/* ..but if guest is not using raw disk format and on a block device,
* then query highest allocated extent from QEMU
@@ -11146,13 +11166,19 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockExtent(priv->mon,
disk->info.alias,
- &info->allocation);
+ &disk->src->allocation);
qemuDomainObjExitMonitor(driver, vm);
} else {
ret = 0;
}
+ if (ret == 0) {
+ info->capacity = disk->src->capacity;
+ info->allocation = disk->src->allocation;
+ info->physical = disk->src->physical;
+ }
+
endjob:
if (!qemuDomainObjEndJob(driver, vm))
vm = NULL;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index e05b843..b4c3808 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -257,8 +257,9 @@ struct _virStorageSource {
virStoragePermsPtr perms;
virStorageTimestampsPtr timestamps;
- unsigned long long allocation; /* in bytes, 0 if unknown */
Spurious move?
unsigned long long capacity; /* in bytes, 0 if unknown */
+ unsigned long long allocation; /* in bytes, 0 if unknown */
+ unsigned long long physical; /* in bytes, 0 if unknown */
size_t nseclabels;
virSecurityDeviceLabelDefPtr *seclabels;
Also an addition to virStorageSourceCopy is missing.
ACK with the tweak to virStorageSourceCopy
Peter
12:51 p.m.
New subject: [libvirt] [PATCH 04/12] qemu: let blockinfo reuse virStorageSource
On 12/08/2014 06:03 AM, Peter Krempa wrote:
On 12/06/14 09:14, Eric Blake wrote:
> Right now, grabbing blockinfo always calls stat on the disk, then
> opens the image to determine the capacity, using a throw-away
> virStorageSourcePtr. This has a couple of drawbacks:
>
> + * For read-only disks, nothing should be changing unless
the user
> + * has requested a block-commit action. For read-write disks, we
> + * know some special cases: capacity should not change without a
> + * block-resize (where capacity is the only stat that requires
> + * opening a file, and even then, only for non-raw files); and
> + * physical size of a raw image or of a block device should
> + * likewise not be changing without block-resize. On the other
> + * hand, allocation of a raw file can change (if the file is
> + * sparse, but the amount of sparseness changes due to writes or
> + * punching holes), and physical size of a non-raw file can
> + * change.
For a live VM we should grab all of the above directly from the monitor
and not ever touch the files on the disk. We do that already for the
bulk stats and for getting the right size when doing storage migration.
Agreed. On my next spin, I'll see about getting block info for live vms
done right. (But first, I'm still in the middle of building a scratch
image with this series, if only so that we have an easier binary to play
with for integration and testing purposes).
This function unfortunately is legacy code compared to the stuff I've
pointed out
Yes, blockinfo is quite old, so here's hoping I don't break any
semantics in touching it.
> +++ b/src/util/virstoragefile.h
> @@ -257,8 +257,9 @@ struct _virStorageSource {
>
> virStoragePermsPtr perms;
> virStorageTimestampsPtr timestamps;
> - unsigned long long allocation; /* in bytes, 0 if unknown */
Spurious move?
> unsigned long long capacity; /* in bytes, 0 if unknown */
> + unsigned long long allocation; /* in bytes, 0 if unknown */
> + unsigned long long physical; /* in bytes, 0 if unknown */
You know, Adam asked the same question last time :)
https://www.redhat.com/archives/libvir-list/2014-November/msg00599.html
It's intentional, to match the order in the public header for block
info. But I'll update the commit message to make it obvious.
> size_t nseclabels;
> virSecurityDeviceLabelDefPtr *seclabels;
>
Also an addition to virStorageSourceCopy is missing.
ACK with the tweak to virStorageSourceCopy
I'll probably wait on this patch until I see how the refactoring goes
for splitting offline vs. online stat gathering.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:14 a.m.
New subject: [libvirt] [PATCH 05/12] qemu: refactor blockinfo data gathering
Create a helper function that can be reused for gathering block
info from virDomainListGetStats.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Split guts...
(qemuStorageLimitsRefresh): ...into new helper function.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 196 ++++++++++++++++++++++++++-----------------------
1 file changed, 105 insertions(+), 91 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e873362..1e254bc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10981,59 +10981,25 @@ qemuDomainMemoryPeek(virDomainPtr dom,
}
+/* Refresh the capacity and allocation limits of a given storage
+ * source. Assumes that the caller has already obtained a domain job.
+ * Set *activeFail to true if data cannot be obtained because a
+ * transient guest is no longer active. */
static int
-qemuDomainGetBlockInfo(virDomainPtr dom,
- const char *path,
- virDomainBlockInfoPtr info,
- unsigned int flags)
+qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr vm, virDomainDiskDefPtr disk,
+ virStorageSourcePtr src, const char *path,
+ bool *activeFail)
{
- virQEMUDriverPtr driver = dom->conn->privateData;
- virDomainObjPtr vm;
int ret = -1;
int fd = -1;
off_t end;
virStorageSourcePtr meta = NULL;
- virDomainDiskDefPtr disk = NULL;
struct stat sb;
- int idx;
- int format;
- bool activeFail = false;
- virQEMUDriverConfigPtr cfg = NULL;
+ int format = src->format;
char *buf = NULL;
ssize_t len;
- virCheckFlags(0, -1);
-
- if (!(vm = qemuDomObjFromDomain(dom)))
- return -1;
-
- cfg = virQEMUDriverGetConfig(driver);
-
- if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
- goto cleanup;
-
- if (!path || path[0] == '\0') {
- virReportError(VIR_ERR_INVALID_ARG, "%s", _("NULL or empty
path"));
- goto cleanup;
- }
-
- /* Technically, we only need a job if we are going to query the
- * monitor, which is only for active domains that are using
- * non-raw block devices. But it is easier to share code if we
- * always grab a job; furthermore, grabbing the job ensures that
- * hot-plug won't change disk behind our backs. */
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
- goto cleanup;
-
- /* Check the path belongs to this domain. */
- if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("invalid path %s not assigned to domain"), path);
- goto endjob;
- }
-
- disk = vm->def->disks[idx];
-
/* FIXME: For an offline domain, we always want to check current
* on-disk statistics (as users have been known to change offline
* images behind our backs). For a running domain, however, it
@@ -11054,76 +11020,74 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* punching holes), and physical size of a non-raw file can
* change.
*/
- if (virStorageSourceIsLocalStorage(disk->src)) {
- if (!disk->src->path) {
+ if (virStorageSourceIsLocalStorage(src)) {
+ if (!src->path) {
virReportError(VIR_ERR_INVALID_ARG,
_("disk '%s' does not currently have a source
assigned"),
path);
- goto endjob;
+ goto cleanup;
}
- if ((fd = qemuOpenFile(driver, vm, disk->src->path, O_RDONLY,
+ if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY,
NULL, NULL)) == -1)
- goto endjob;
+ goto cleanup;
if (fstat(fd, &sb) < 0) {
virReportSystemError(errno,
- _("cannot stat file '%s'"),
disk->src->path);
- goto endjob;
+ _("cannot stat file '%s'"),
src->path);
+ goto cleanup;
}
if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
virReportSystemError(errno, _("cannot read header '%s'"),
- disk->src->path);
- goto endjob;
+ src->path);
+ goto cleanup;
}
} else {
- if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
- goto endjob;
+ if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0)
+ goto cleanup;
- if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
+ if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
&buf)) < 0)
- goto endjob;
+ goto cleanup;
- if (virStorageFileStat(disk->src, &sb) < 0) {
+ if (virStorageFileStat(src, &sb) < 0) {
virReportSystemError(errno, _("failed to stat remote file
'%s'"),
- NULLSTR(disk->src->path));
- goto endjob;
+ NULLSTR(src->path));
+ goto cleanup;
}
}
/* Probe for magic formats */
- if (virDomainDiskGetFormat(disk)) {
- format = virDomainDiskGetFormat(disk);
- } else {
+ if (!format) {
if (!cfg->allowDiskFormatProbing) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("no disk format for %s and probing is disabled"),
path);
- goto endjob;
+ goto cleanup;
}
- if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
+ if ((format = virStorageFileProbeFormatFromBuf(src->path,
buf, len)) < 0)
- goto endjob;
+ goto cleanup;
}
- if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
+ if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
format, NULL)))
- goto endjob;
+ goto cleanup;
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- disk->src->physical = (unsigned long long)sb.st_blocks *
+ src->physical = (unsigned long long)sb.st_blocks *
(unsigned long long)DEV_BSIZE;
#else
- disk->src->physical = sb.st_size;
+ src->physical = sb.st_size;
#endif
/* Regular files may be sparse, so logical size (capacity) is not same
* as actual physical above
*/
- disk->src->capacity = sb.st_size;
+ src->capacity = sb.st_size;
} else {
/* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
* be 64 bits on all platforms.
@@ -11132,24 +11096,24 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (end == (off_t)-1) {
virReportSystemError(errno,
_("failed to seek to end of %s"), path);
- goto endjob;
+ goto cleanup;
}
- disk->src->physical = end;
- disk->src->capacity = end;
+ src->physical = end;
+ src->capacity = end;
}
/* If the file we probed has a capacity set, then override
* what we calculated from file/block extents */
if (meta->capacity)
- disk->src->capacity = meta->capacity;
+ src->capacity = meta->capacity;
/* Set default value .. */
- disk->src->allocation = disk->src->physical;
+ src->allocation = src->physical;
/* ..but if guest is not using raw disk format and on a block device,
* then query highest allocated extent from QEMU
*/
- if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_BLOCK &&
+ if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK &&
format != VIR_STORAGE_FILE_RAW &&
S_ISBLK(sb.st_mode)) {
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -11158,37 +11122,87 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* depends on whether domain is persistent
*/
if (!virDomainObjIsActive(vm)) {
- activeFail = true;
+ *activeFail = true;
ret = 0;
- goto endjob;
+ goto cleanup;
}
qemuDomainObjEnterMonitor(driver, vm);
ret = qemuMonitorGetBlockExtent(priv->mon,
disk->info.alias,
- &disk->src->allocation);
+ &src->allocation);
qemuDomainObjExitMonitor(driver, vm);
} else {
ret = 0;
}
-
- if (ret == 0) {
- info->capacity = disk->src->capacity;
- info->allocation = disk->src->allocation;
- info->physical = disk->src->physical;
- }
-
- endjob:
- if (!qemuDomainObjEndJob(driver, vm))
- vm = NULL;
cleanup:
VIR_FREE(buf);
virStorageSourceFree(meta);
VIR_FORCE_CLOSE(fd);
- if (disk)
- virStorageFileDeinit(disk->src);
+ virStorageFileDeinit(src);
+ return ret;
+}
+
+static int
+qemuDomainGetBlockInfo(virDomainPtr dom,
+ const char *path,
+ virDomainBlockInfoPtr info,
+ unsigned int flags)
+{
+ virQEMUDriverPtr driver = dom->conn->privateData;
+ virDomainObjPtr vm;
+ int ret = -1;
+ virDomainDiskDefPtr disk = NULL;
+ int idx;
+ bool activeFail = false;
+ virQEMUDriverConfigPtr cfg = NULL;
+
+ virCheckFlags(0, -1);
+
+ if (!(vm = qemuDomObjFromDomain(dom)))
+ return -1;
+
+ cfg = virQEMUDriverGetConfig(driver);
+
+ if (virDomainGetBlockInfoEnsureACL(dom->conn, vm->def) < 0)
+ goto cleanup;
+
+ if (!path || path[0] == '\0') {
+ virReportError(VIR_ERR_INVALID_ARG, "%s", _("NULL or empty
path"));
+ goto cleanup;
+ }
+
+ /* Technically, we only need a job if we are going to query the
+ * monitor, which is only for active domains that are using
+ * non-raw block devices. But it is easier to share code if we
+ * always grab a job; furthermore, grabbing the job ensures that
+ * hot-plug won't change disk behind our backs. */
+ if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0)
+ goto cleanup;
+
+ /* Check the path belongs to this domain. */
+ if ((idx = virDomainDiskIndexByName(vm->def, path, false)) < 0) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("invalid path %s not assigned to domain"), path);
+ goto endjob;
+ }
+
+ disk = vm->def->disks[idx];
+
+ if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, disk, disk->src, path,
+ &activeFail)) < 0)
+ goto endjob;
+
+ info->capacity = disk->src->capacity;
+ info->allocation = disk->src->allocation;
+ info->physical = disk->src->physical;
+
+ endjob:
+ if (!qemuDomainObjEndJob(driver, vm))
+ vm = NULL;
+ cleanup:
/* If we failed to get data from a domain because it's inactive and
* it's not a persistent domain, then force failure.
*/
--
1.9.3
7:59 a.m.
New subject: [libvirt] [PATCH 05/12] qemu: refactor blockinfo data gathering
On 12/06/14 09:14, Eric Blake wrote:
Create a helper function that can be reused for gathering block
info from virDomainListGetStats.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Split guts...
(qemuStorageLimitsRefresh): ...into new helper function.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 196 ++++++++++++++++++++++++++-----------------------
1 file changed, 105 insertions(+), 91 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e873362..1e254bc 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10981,59 +10981,25 @@ qemuDomainMemoryPeek(virDomainPtr dom,
}
+/* Refresh the capacity and allocation limits of a given storage
+ * source. Assumes that the caller has already obtained a domain job.
+ * Set *activeFail to true if data cannot be obtained because a
+ * transient guest is no longer active. */
static int
-qemuDomainGetBlockInfo(virDomainPtr dom,
- const char *path,
- virDomainBlockInfoPtr info,
- unsigned int flags)
+qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
One argument per line looks better.
+ virDomainObjPtr vm, virDomainDiskDefPtr
disk,
+ virStorageSourcePtr src, const char *path,
disk, src and path together? That doesn't seem to make much sense.
While 'path' is used for error messages only, and I don't think it's
entirely necessary to use the user-provided string in error messages we
will be good of with just using src->path.
The 'disk' argument is used to gather the allocation stat in case of
block devices for an active domain. That is really strange. If you want
to make this function usable for any disk source, the disk alias won't
be the right way to get that.
I think a better approach will be to split the code into two helpers,
one being called on offline domains that gathers data from images on
disk and doesn't ever touch the monitor (basically the existing code in
qemuDomainGetBlockInfo minus the monitor call) and a second helper that
will gather the data from monitor calls and never touch files used for
online VMs. This is partially available in the existing helper to gather
domain block stats.
Peter
2:14 a.m.
New subject: [libvirt] [PATCH 06/12] getstats: start giving offline block stats
I noticed that for an offline domain, 'virsh domstats --block $dom'
was producing just the domain name, with no stats. But the older
'virsh domblkinfo' works just fine on offline domains. This patch
starts to get us closer, by at least reporting the disk names for
an offline domain.
With this patch, I now see the following for an offline domain
with one qcow2 disk and an empty cdrom drive:
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.1.name=hdc
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Don't short-circuit
output of block name.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1e254bc..404decd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18529,19 +18529,20 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
int rc;
virHashTablePtr stats = NULL;
qemuDomainObjPrivatePtr priv = dom->privateData;
+ bool abbreviated = false;
- if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
- return 0; /* it's ok, just go ahead silently */
+ if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
+ abbreviated = true; /* it's ok, just go ahead silently */
+ } else {
+ qemuDomainObjEnterMonitor(driver, dom);
+ rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
+ ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
+ qemuDomainObjExitMonitor(driver, dom);
- qemuDomainObjEnterMonitor(driver, dom);
- rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
- ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
- qemuDomainObjExitMonitor(driver, dom);
-
- if (rc < 0) {
- virResetLastError();
- ret = 0; /* still ok, again go ahead silently */
- goto cleanup;
+ if (rc < 0) {
+ virResetLastError();
+ abbreviated = true; /* still ok, again go ahead silently */
+ }
}
QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
@@ -18552,9 +18553,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);
- if (!disk->info.alias ||
- !(entry = virHashLookup(stats, disk->info.alias)))
+ if (abbreviated || !disk->info.alias ||
+ !(entry = virHashLookup(stats, disk->info.alias))) {
+ /* FIXME: we could still look up sizing by sharing code
+ * with qemuDomainGetBlockInfo */
continue;
+ }
QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
"rd.reqs", entry->rd_req);
--
1.9.3
8:02 a.m.
New subject: [libvirt] [PATCH 06/12] getstats: start giving offline block stats
On 12/06/14 09:14, Eric Blake wrote:
I noticed that for an offline domain, 'virsh domstats --block
$dom'
was producing just the domain name, with no stats. But the older
'virsh domblkinfo' works just fine on offline domains. This patch
starts to get us closer, by at least reporting the disk names for
an offline domain.
With this patch, I now see the following for an offline domain
with one qcow2 disk and an empty cdrom drive:
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.1.name=hdc
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Don't short-circuit
output of block name.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
ACK,
Peter
2:14 a.m.
New subject: [libvirt] [PATCH 07/12] getstats: report block sizes for offline domains
The prior refactoring can now be put to use. With the same domain
as the previous commit (one qcow2 disk and an empty cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.0.allocation=200704
block.0.capacity=42949672960
block.0.physical=200704
block.1.name=hdc
* src/qemu/qemu_driver.c (qemuStorageLimitsRefresh): Tweak
semantics of helper function.
(qemuDomainGetStatsBlock): Use it to report offline statistics.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 404decd..723391b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10983,7 +10983,9 @@ qemuDomainMemoryPeek(virDomainPtr dom,
/* Refresh the capacity and allocation limits of a given storage
* source. Assumes that the caller has already obtained a domain job.
- * Set *activeFail to true if data cannot be obtained because a
+ * Pass NULL for path to skip any errors about an empty drive.
+ * Pass NULL for activeFail to skip any monitor attempt, otherwise,
+ * set *activeFail to true if data cannot be obtained because a
* transient guest is no longer active. */
static int
qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
@@ -11000,6 +11002,12 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
char *buf = NULL;
ssize_t len;
+ if (!path) {
+ if (virStorageSourceIsEmpty(src))
+ return 0;
+ path = src->path;
+ }
+
/* FIXME: For an offline domain, we always want to check current
* on-disk statistics (as users have been known to change offline
* images behind our backs). For a running domain, however, it
@@ -11116,23 +11124,27 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK &&
format != VIR_STORAGE_FILE_RAW &&
S_ISBLK(sb.st_mode)) {
- qemuDomainObjPrivatePtr priv = vm->privateData;
-
- /* If the guest is not running, then success/failure return
- * depends on whether domain is persistent
- */
- if (!virDomainObjIsActive(vm)) {
- *activeFail = true;
+ if (!activeFail) {
+ disk->src->allocation = 0;
ret = 0;
- goto cleanup;
+ } else {
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
+ /* If the guest is not running, then success/failure return
+ * depends on whether domain is persistent
+ */
+ if (!virDomainObjIsActive(vm)) {
+ *activeFail = true;
+ ret = 0;
+ goto cleanup;
+ }
+
+ qemuDomainObjEnterMonitor(driver, vm);
+ ret = qemuMonitorGetBlockExtent(priv->mon,
+ disk->info.alias,
+ &src->allocation);
+ qemuDomainObjExitMonitor(driver, vm);
}
-
- qemuDomainObjEnterMonitor(driver, vm);
- ret = qemuMonitorGetBlockExtent(priv->mon,
- disk->info.alias,
- &src->allocation);
- qemuDomainObjExitMonitor(driver, vm);
-
} else {
ret = 0;
}
@@ -18530,6 +18542,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
virHashTablePtr stats = NULL;
qemuDomainObjPrivatePtr priv = dom->privateData;
bool abbreviated = false;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
abbreviated = true; /* it's ok, just go ahead silently */
@@ -18555,8 +18568,18 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
if (abbreviated || !disk->info.alias ||
!(entry = virHashLookup(stats, disk->info.alias))) {
- /* FIXME: we could still look up sizing by sharing code
- * with qemuDomainGetBlockInfo */
+ if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, disk->src, NULL, NULL) < 0)
+ goto cleanup;
+ if (disk->src->allocation)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "allocation",
disk->src->allocation);
+ if (disk->src->capacity)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "capacity",
disk->src->capacity);
+ if (disk->src->physical)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "physical",
disk->src->physical);
continue;
}
@@ -18593,6 +18616,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
cleanup:
virHashFree(stats);
+ virObjectUnref(cfg);
return ret;
}
--
1.9.3
8:11 a.m.
New subject: [libvirt] [PATCH 07/12] getstats: report block sizes for offline domains
On 12/06/14 09:14, Eric Blake wrote:
The prior refactoring can now be put to use. With the same domain
as the previous commit (one qcow2 disk and an empty cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.0.allocation=200704
block.0.capacity=42949672960
block.0.physical=200704
block.1.name=hdc
* src/qemu/qemu_driver.c (qemuStorageLimitsRefresh): Tweak
semantics of helper function.
(qemuDomainGetStatsBlock): Use it to report offline statistics.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 60 +++++++++++++++++++++++++++++++++++---------------
1 file changed, 42 insertions(+), 18 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 404decd..723391b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10983,7 +10983,9 @@ qemuDomainMemoryPeek(virDomainPtr dom,
/* Refresh the capacity and allocation limits of a given storage
* source. Assumes that the caller has already obtained a domain job.
- * Set *activeFail to true if data cannot be obtained because a
+ * Pass NULL for path to skip any errors about an empty drive.
+ * Pass NULL for activeFail to skip any monitor attempt, otherwise,
Having a separate offline-only helper in this case would avoid us having
this argument as it would never touch the monitor.
+ * set *activeFail to true if data cannot be obtained because a
* transient guest is no longer active. */
static int
qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
@@ -11000,6 +11002,12 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
char *buf = NULL;
ssize_t len;
+ if (!path) {
+ if (virStorageSourceIsEmpty(src))
+ return 0;
+ path = src->path;
+ }
Again, no need to pass path explicitly.
+
/* FIXME: For an offline domain, we always want to check current
* on-disk statistics (as users have been known to change offline
* images behind our backs). For a running domain, however, it
...
@@ -18530,6 +18542,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr
driver,
virHashTablePtr stats = NULL;
qemuDomainObjPrivatePtr priv = dom->privateData;
bool abbreviated = false;
+ virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
abbreviated = true; /* it's ok, just go ahead silently */
@@ -18555,8 +18568,18 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
if (abbreviated || !disk->info.alias ||
!(entry = virHashLookup(stats, disk->info.alias))) {
- /* FIXME: we could still look up sizing by sharing code
- * with qemuDomainGetBlockInfo */
+ if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, disk->src, NULL, NULL) < 0)
+ goto cleanup;
+ if (disk->src->allocation)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "allocation",
disk->src->allocation);
+ if (disk->src->capacity)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "capacity",
disk->src->capacity);
+ if (disk->src->physical)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
+ "physical",
disk->src->physical);
continue;
}
This part looks good. Having stats for offline VMs might be useful.
@@ -18593,6 +18616,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
cleanup:
virHashFree(stats);
+ virObjectUnref(cfg);
return ret;
}
Peter
2:14 a.m.
New subject: [libvirt] [PATCH 08/12] getstats: add block.n.path stat
I'm about to make block stats optionally more complex to cover
backing chains, where block.count will no longer equal the number
of <disks> for a domain. For these reasons, it is nicer if the
statistics output includes the source path (for local files).
This patch doesn't add anything for network disks, although we
may decide to add that later.
With this patch, I now see the following for the same domain as
in earlier patches (one qcow2 file, and an empty cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.0.path=/var/lib/libvirt/images/foo.qcow2
block.0.allocation=200704
block.0.capacity=42949672960
block.0.physical=200704
block.1.name=hdc
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
new field.
* tools/virsh.pod (domstats): Document new field.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Return the new
stat for local files/block devices.
(QEMU_ADD_NAME_PARAM): Add parameter.
(qemuDomainGetStatsInterface): Update caller.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/libvirt-domain.c | 3 +++
src/qemu/qemu_driver.c | 11 +++++++----
tools/virsh.pod | 2 ++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 87123d1..cb76d8c 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10910,6 +10910,9 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* "block.<num>.name" - name of the block device <num> as string.
* matches the target name (vda/sda/hda) of the
* block device.
+ * "block.<num>.path" - string describing the source of block device
<num>,
+ * if it is a file or block device (omitted for network
+ * sources and drives with no media inserted).
* "block.<num>.rd.reqs" - number of read requests as unsigned long
long.
* "block.<num>.rd.bytes" - number of read bytes as unsigned long long.
* "block.<num>.rd.times" - total time (ns) spent on reads as
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 723391b..5334f8d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18422,11 +18422,11 @@ do { \
goto cleanup; \
} while (0)
-#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
+#define QEMU_ADD_NAME_PARAM(record, maxparams, type, subtype, num, name) \
do { \
char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
- "%s.%zu.name", type, num); \
+ "%s.%zu.%s", type, num, subtype); \
if (virTypedParamsAddString(&(record)->params, \
&(record)->nparams, \
maxparams, \
@@ -18471,7 +18471,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
memset(&tmp, 0, sizeof(tmp));
QEMU_ADD_NAME_PARAM(record, maxparams,
- "net", i, dom->def->nets[i]->ifname);
+ "net", "name", i,
dom->def->nets[i]->ifname);
if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) {
virResetLastError();
@@ -18564,7 +18564,10 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
qemuBlockStats *entry;
virDomainDiskDefPtr disk = dom->def->disks[i];
- QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);
+ QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", i,
disk->dst);
+ if (virStorageSourceIsLocalStorage(disk->src) &&
disk->src->path)
+ QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
+ i, disk->src->path);
if (abbreviated || !disk->info.alias ||
!(entry = virHashLookup(stats, disk->info.alias))) {
diff --git a/tools/virsh.pod b/tools/virsh.pod
index c070261..cbd82275 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -883,6 +883,8 @@ I<--interface> returns:
I<--block> returns:
"block.count" - number of block devices on this domain,
"block.<num>.name" - name of the target of the block device <num>,
+"block.<num>.path" - file source of block device <num>, if it is a
+local file or block device,
"block.<num>.rd.reqs" - number of read requests,
"block.<num>.rd.bytes" - number of read bytes,
"block.<num>.rd.times" - total time (ns) spent on reads,
--
1.9.3
8:17 a.m.
New subject: [libvirt] [PATCH 08/12] getstats: add block.n.path stat
On 12/06/14 09:14, Eric Blake wrote:
I'm about to make block stats optionally more complex to cover
backing chains, where block.count will no longer equal the number
of <disks> for a domain. For these reasons, it is nicer if the
statistics output includes the source path (for local files).
This patch doesn't add anything for network disks, although we
may decide to add that later.
With this patch, I now see the following for the same domain as
in earlier patches (one qcow2 file, and an empty cdrom drive):
$ virsh domstats --block foo
Domain: 'foo'
block.count=2
block.0.name=hda
block.0.path=/var/lib/libvirt/images/foo.qcow2
block.0.allocation=200704
block.0.capacity=42949672960
block.0.physical=200704
block.1.name=hdc
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document
new field.
* tools/virsh.pod (domstats): Document new field.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Return the new
stat for local files/block devices.
(QEMU_ADD_NAME_PARAM): Add parameter.
(qemuDomainGetStatsInterface): Update caller.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/libvirt-domain.c | 3 +++
src/qemu/qemu_driver.c | 11 +++++++----
tools/virsh.pod | 2 ++
3 files changed, 12 insertions(+), 4 deletions(-)
ACK,
Peter
1:04 p.m.
New subject: [libvirt] [PATCH 08/12] getstats: add block.n.path stat
On 12/08/2014 07:17 AM, Peter Krempa wrote:
On 12/06/14 09:14, Eric Blake wrote:
> I'm about to make block stats optionally more complex to cover
> backing chains, where block.count will no longer equal the number
> of <disks> for a domain. For these reasons, it is nicer if the
> statistics output includes the source path (for local files).
> This patch doesn't add anything for network disks, although we
> may decide to add that later.
>
> With this patch, I now see the following for the same domain as
> in earlier patches (one qcow2 file, and an empty cdrom drive):
> $ virsh domstats --block foo
> Domain: 'foo'
> block.count=2
> block.0.name=hda
> block.0.path=/var/lib/libvirt/images/foo.qcow2
> block.0.allocation=200704
> block.0.capacity=42949672960
> block.0.physical=200704
> block.1.name=hdc
ACK,
2, 6, and 8 now pushed. Of course, the commit message for 8 had to be
tweaked a bit (since the output is waiting on the offline stats of patch
7, which in turn are waiting on the split of offline vs. online in the
helper to blockinfo).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:14 a.m.
New subject: [libvirt] [PATCH 09/12] getstats: prepare for dynamic block.count stat
A coming patch will make it optionally possible to list backing
chain block stats; in this mode of operation, block.counts is no
longer the number of <disks> in the domain, but the number of
blocks in the array being reported. We still want block.count
listed first, but rather than iterate the tree twice (once to
count, and once to list stats), it's easier to just touch things
up after the fact.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Compute count
after the fact.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5334f8d..cb80f49 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18543,6 +18543,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
qemuDomainObjPrivatePtr priv = dom->privateData;
bool abbreviated = false;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
+ int count_index = -1;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
abbreviated = true; /* it's ok, just go ahead silently */
@@ -18558,7 +18559,11 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
}
}
- QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
+ /* When listing backing chains, it's easier to fix up the count
+ * after the iteration than it is to iterate twice; but we still
+ * want count listed first. */
+ count_index = record->nparams;
+ QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
for (i = 0; i < dom->def->ndisks; i++) {
qemuBlockStats *entry;
@@ -18618,6 +18623,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
ret = 0;
cleanup:
+ if (count_index >= 0)
+ record->params[count_index].value.ui = i;
virHashFree(stats);
virObjectUnref(cfg);
return ret;
--
1.9.3
8:19 a.m.
New subject: [libvirt] [PATCH 09/12] getstats: prepare for dynamic block.count stat
On 12/06/14 09:14, Eric Blake wrote:
A coming patch will make it optionally possible to list backing
chain block stats; in this mode of operation, block.counts is no
longer the number of <disks> in the domain, but the number of
blocks in the array being reported. We still want block.count
listed first, but rather than iterate the tree twice (once to
count, and once to list stats), it's easier to just touch things
up after the fact.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Compute count
after the fact.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
ACK,
Peter
8:26 a.m.
New subject: [libvirt] [PATCH 09/12] getstats: prepare for dynamic block.count stat
On 12/08/14 15:19, Peter Krempa wrote:
On 12/06/14 09:14, Eric Blake wrote:
> A coming patch will make it optionally possible to list backing
> chain block stats; in this mode of operation, block.counts is no
> longer the number of <disks> in the domain, but the number of
> blocks in the array being reported. We still want block.count
> listed first, but rather than iterate the tree twice (once to
> count, and once to list stats), it's easier to just touch things
> up after the fact.
>
> * src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Compute count
> after the fact.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
Compiler complains:
qemu/qemu_driver.c: In function 'qemuDomainGetStatsBlock':
qemu/qemu_driver.c:18630:46: error: 'i' may be used uninitialized in this function
[-Werror=maybe-uninitialized]
record->params[count_index].value.ui = i;
^
CC lxc/libvirt_driver_lxc_impl_la-lxc_native.lo
CC lxc/libvirt_driver_lxc_impl_la-lxc_driver.lo
CC uml/libvirt_driver_uml_impl_la-uml_conf.lo
CC uml/libvirt_driver_uml_impl_la-uml_driver.lo
CC network/libvirt_driver_network_impl_la-bridge_driver.lo
CC network/libvirt_driver_network_impl_la-bridge_driver_platform.lo
ACK,
if you fix the issue above.
Peter
11:22 a.m.
New subject: [libvirt] [PATCH 09/12] getstats: prepare for dynamic block.count stat
On 12/08/2014 07:26 AM, Peter Krempa wrote:
On 12/08/14 15:19, Peter Krempa wrote:
> On 12/06/14 09:14, Eric Blake wrote:
>> A coming patch will make it optionally possible to list backing
>> chain block stats; in this mode of operation, block.counts is no
>> longer the number of <disks> in the domain, but the number of
>> blocks in the array being reported. We still want block.count
>> listed first, but rather than iterate the tree twice (once to
>> count, and once to list stats), it's easier to just touch things
>> up after the fact.
>>
>> * src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Compute count
>> after the fact.
>>
>> Signed-off-by: Eric Blake <eblake(a)redhat.com>
>> ---
>> src/qemu/qemu_driver.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
Compiler complains:
qemu/qemu_driver.c: In function 'qemuDomainGetStatsBlock':
qemu/qemu_driver.c:18630:46: error: 'i' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
record->params[count_index].value.ui = i;
^
You must be compiling at -O2 while I was not :)
Found the culprit: the QEMU_ADD_COUNT_PARAM macro has a hidden goto,
that can bypass the initialization of i=0 in the for loop; the solution
is trivial.
>
> ACK,
>
if you fix the issue above.
I'll be pushing the acked patches that can be reshuffled without
dependencies, and reposting the rest of the series with fixes
incorporated...
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index bb516af..78d1bdd 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -18539,7 +18539,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
int *maxparams,
unsigned int privflags)
{
- size_t i;
+ size_t i = 0;
int ret = -1;
int rc;
virHashTablePtr stats = NULL;
@@ -18568,7 +18568,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
count_index = record->nparams;
QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
- for (i = 0; i < dom->def->ndisks; i++) {
+ for (i; i < dom->def->ndisks; i++) {
qemuBlockStats *entry;
virDomainDiskDefPtr disk = dom->def->disks[i];
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:14 a.m.
New subject: [libvirt] [PATCH 10/12] getstats: add new flag for block backing chain
This patch introduces access to allocation information about
a backing chain of a live domain. While querying storage
volumes for read-only disks could provide some of the details,
there is one case where we have to rely on qemu: when doing
a block commit into a backing file, where that file is stored
in qcow2 format on a host block device, we want to know the
current highest write offset into that image, in order to know
if the disk must be resized larger. qemu-img does not
(currently) show this information, and none of the earlier
block APIs were extensible enough to expose it. But
virDomainListGetStats is perfect for the job!
We don't need a new group of statistics, as the existing block
group is sufficient. On the other hand, as existing libvirt
releases already report 1:1 mapping of block.count to <disk>
devices, changing the array size could confuse older clients;
and even with newer clients, the time and memory taken to
report additional statistics is not always necessary (backing
files are generally read-only except for block-commit, so while
read statistics may change, sizing statistics will not). So
the choice here is to add a new flag that only newer callers
will pass, when they are prepared for the additional information.
This patch introduces the new API, but it will take more
patches to get it implemented for qemu.
* include/libvirt/libvirt-domain.h
(VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING): New flag.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document it,
and add a new field when it is in use.
* tools/virsh-domain-monitor.c (cmdDomstats): Use new flag.
* tools/virsh.pod (domstats): Document it.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
include/libvirt/libvirt-domain.h | 1 +
src/libvirt-domain.c | 7 ++++++-
tools/virsh-domain-monitor.c | 7 +++++++
tools/virsh.pod | 8 ++++++--
4 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index ae2c49c..e6185b9 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1698,6 +1698,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_BACKING = 1 << 30, /* include backing chain
for block stats */
VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1 << 31, /* enforce requested
stats */
} virConnectGetAllDomainStatsFlags;
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cb76d8c..8c4ad7b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10903,13 +10903,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* "net.<num>.tx.errs" - transmission errors as unsigned long long.
* "net.<num>.tx.drop" - transmit packets dropped as unsigned long long.
*
- * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics.
+ * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics. By default,
+ * this information is limited to the active layer of each <disk> of the
+ * domain, but adding VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING to @flags
+ * will expand the array to cover backing chains.
* The typed parameter keys are in this format:
* "block.count" - number of block devices on this domain
* as unsigned int.
* "block.<num>.name" - name of the block device <num> as string.
* matches the target name (vda/sda/hda) of the
* block device.
+ * "block.<num>.backingIndex" - unsigned int giving the
<backingStore> index,
+ * when backing images are listed.
* "block.<num>.path" - string describing the source of block device
<num>,
* if it is a file or block device (omitted for network
* sources and drives with no media inserted).
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 259400f..214c0b2 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -2037,6 +2037,10 @@ static const vshCmdOptDef opts_domstats[] = {
.type = VSH_OT_BOOL,
.help = N_("enforce requested stats parameters"),
},
+ {.name = "backing",
+ .type = VSH_OT_BOOL,
+ .help = N_("add backing chain information to block stats"),
+ },
{.name = "domain",
.type = VSH_OT_ARGV,
.flags = VSH_OFLAG_NONE,
@@ -2130,6 +2134,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptBool(cmd, "enforce"))
flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS;
+ if (vshCommandOptBool(cmd, "backing"))
+ flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING;
+
if (vshCommandOptBool(cmd, "domain")) {
if (VIR_ALLOC_N(domlist, 1) < 0)
goto cleanup;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index cbd82275..378f1c0 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -825,7 +825,7 @@ 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<--state>]
+=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>]
[I<--state>]
[I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>]
[I<--block>]
[[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>]
[I<--list-transient>] [I<--list-running>] [I<--list-paused>]
@@ -880,7 +880,11 @@ I<--interface> returns:
"net.<num>.tx.errs" - number of transmission errors,
"net.<num>.tx.drop" - number of transmit packets dropped
-I<--block> returns:
+I<--block> returns information about disks associated with each
+domain. Using the I<--backing> flag extends this information to
+cover all resources in the backing chain, rather than the default
+of limiting information to the active layer for each guest disk.
+Information listed includes:
"block.count" - number of block devices on this domain,
"block.<num>.name" - name of the target of the block device <num>,
"block.<num>.path" - file source of block device <num>, if it is a
--
1.9.3
8:24 a.m.
New subject: [libvirt] [PATCH 10/12] getstats: add new flag for block backing chain
On 12/06/14 09:14, Eric Blake wrote:
ACK if you add the man page entry.
Peter
This patch introduces access to allocation information about
a backing chain of a live domain. While querying storage
volumes for read-only disks could provide some of the details,
there is one case where we have to rely on qemu: when doing
a block commit into a backing file, where that file is stored
in qcow2 format on a host block device, we want to know the
current highest write offset into that image, in order to know
if the disk must be resized larger. qemu-img does not
(currently) show this information, and none of the earlier
block APIs were extensible enough to expose it. But
virDomainListGetStats is perfect for the job!
We don't need a new group of statistics, as the existing block
group is sufficient. On the other hand, as existing libvirt
releases already report 1:1 mapping of block.count to <disk>
devices, changing the array size could confuse older clients;
and even with newer clients, the time and memory taken to
report additional statistics is not always necessary (backing
files are generally read-only except for block-commit, so while
read statistics may change, sizing statistics will not). So
the choice here is to add a new flag that only newer callers
will pass, when they are prepared for the additional information.
This patch introduces the new API, but it will take more
patches to get it implemented for qemu.
* include/libvirt/libvirt-domain.h
(VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING): New flag.
* src/libvirt-domain.c (virConnectGetAllDomainStats): Document it,
and add a new field when it is in use.
* tools/virsh-domain-monitor.c (cmdDomstats): Use new flag.
* tools/virsh.pod (domstats): Document it.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
include/libvirt/libvirt-domain.h | 1 +
src/libvirt-domain.c | 7 ++++++-
tools/virsh-domain-monitor.c | 7 +++++++
tools/virsh.pod | 8 ++++++--
4 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index cb76d8c..8c4ad7b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -10903,13 +10903,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
* "net.<num>.tx.errs" - transmission errors as unsigned long long.
* "net.<num>.tx.drop" - transmit packets dropped as unsigned long
long.
*
- * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics.
+ * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics. By default,
+ * this information is limited to the active layer of each <disk> of the
+ * domain, but adding VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING to @flags
+ * will expand the array to cover backing chains.
* The typed parameter keys are in this format:
* "block.count" - number of block devices on this domain
* as unsigned int.
* "block.<num>.name" - name of the block device <num> as string.
* matches the target name (vda/sda/hda) of the
* block device.
+ * "block.<num>.backingIndex" - unsigned int giving the
<backingStore> index,
+ * when backing images are listed.
So name will always be "vda/sda/hda" and this will change according to
the position in the backing chain?
Okay. That makes sense although it's not entirely obvious from the
description.
* "block.<num>.path" - string describing the source
of block device <num>,
* if it is a file or block device (omitted for network
* sources and drives with no media inserted).
...
diff --git a/tools/virsh.pod b/tools/virsh.pod
index cbd82275..378f1c0 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -825,7 +825,7 @@ 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<--state>]
+=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>]
[I<--state>]
[I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>]
[I<--block>]
[[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>]
[I<--list-transient>] [I<--list-running>] [I<--list-paused>]
@@ -880,7 +880,11 @@ I<--interface> returns:
"net.<num>.tx.errs" - number of transmission errors,
"net.<num>.tx.drop" - number of transmit packets dropped
-I<--block> returns:
+I<--block> returns information about disks associated with each
+domain. Using the I<--backing> flag extends this information to
+cover all resources in the backing chain, rather than the default
+of limiting information to the active layer for each guest disk.
+Information listed includes:
"block.count" - number of block devices on this domain,
"block.<num>.name" - name of the target of the block device
<num>,
"block.<num>.path" - file source of block device <num>, if it is a
The man page is missing entry for "block.<num>.backingIndex"
2:14 a.m.
New subject: [libvirt] [PATCH 11/12] getstats: split block stats reporting for easier recursion
In order to report stats on backing chains, we need to separate
the output of stats for one block from how we traverse blocks.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Split...
(qemuDomainGetStatsOneBlock): ...into new helper.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 127 ++++++++++++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb80f49..feaa4a2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18529,6 +18529,79 @@ do { \
goto cleanup; \
} while (0)
+
+static int
+qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
+ virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr dom,
+ virDomainStatsRecordPtr record,
+ int *maxparams,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr src,
+ size_t block_idx,
+ bool abbreviated,
+ virHashTablePtr stats)
+{
+ qemuBlockStats *entry;
+ int ret = -1;
+
+ QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name",
block_idx,
+ disk->dst);
+ if (virStorageSourceIsLocalStorage(src) && src->path)
+ QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
+ block_idx, src->path);
+
+ if (abbreviated || !disk->info.alias ||
+ !(entry = virHashLookup(stats, disk->info.alias))) {
+ if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, src, NULL, NULL) < 0)
+ goto cleanup;
+ if (src->allocation)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "allocation", src->allocation);
+ if (src->capacity)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "capacity", src->capacity);
+ if (src->physical)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "physical", src->physical);
+ ret = 0;
+ goto cleanup;
+ }
+
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "rd.reqs", entry->rd_req);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "rd.bytes", entry->rd_bytes);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "rd.times", entry->rd_total_times);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "wr.reqs", entry->wr_req);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "wr.bytes", entry->wr_bytes);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "wr.times", entry->wr_total_times);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "fl.reqs", entry->flush_req);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "fl.times", entry->flush_total_times);
+
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "allocation", entry->wr_highest_offset);
+
+ if (entry->capacity)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "capacity", entry->capacity);
+ if (entry->physical)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "physical", entry->physical);
+
+ ret = 0;
+ cleanup:
+ return ret;
+}
+
+
static int
qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
virDomainObjPtr dom,
@@ -18566,58 +18639,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
for (i = 0; i < dom->def->ndisks; i++) {
- qemuBlockStats *entry;
virDomainDiskDefPtr disk = dom->def->disks[i];
- QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", i,
disk->dst);
- if (virStorageSourceIsLocalStorage(disk->src) &&
disk->src->path)
- QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
- i, disk->src->path);
-
- if (abbreviated || !disk->info.alias ||
- !(entry = virHashLookup(stats, disk->info.alias))) {
- if (qemuStorageLimitsRefresh(driver, cfg, dom,
- disk, disk->src, NULL, NULL) < 0)
- goto cleanup;
- if (disk->src->allocation)
- QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "allocation",
disk->src->allocation);
- if (disk->src->capacity)
- QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "capacity",
disk->src->capacity);
- if (disk->src->physical)
- QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "physical",
disk->src->physical);
- continue;
- }
-
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "rd.reqs", entry->rd_req);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "rd.bytes", entry->rd_bytes);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "rd.times", entry->rd_total_times);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "wr.reqs", entry->wr_req);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "wr.bytes", entry->wr_bytes);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "wr.times", entry->wr_total_times);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "fl.reqs", entry->flush_req);
- QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
- "fl.times", entry->flush_total_times);
-
- QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "allocation", entry->wr_highest_offset);
-
- if (entry->capacity)
- QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "capacity", entry->capacity);
- if (entry->physical)
- QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, i,
- "physical", entry->physical);
-
+ if (qemuDomainGetStatsOneBlock(driver, cfg, dom, record, maxparams,
+ disk, disk->src, i, abbreviated,
+ stats) < 0)
+ goto cleanup;
}
ret = 0;
--
1.9.3
8:41 a.m.
New subject: [libvirt] [PATCH 11/12] getstats: split block stats reporting for easier recursion
On 12/06/14 09:14, Eric Blake wrote:
In order to report stats on backing chains, we need to separate
the output of stats for one block from how we traverse blocks.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Split...
(qemuDomainGetStatsOneBlock): ...into new helper.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 127 ++++++++++++++++++++++++++++++-------------------
1 file changed, 77 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index cb80f49..feaa4a2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18529,6 +18529,79 @@ do { \
goto cleanup; \
} while (0)
+
+static int
+qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
+ virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr dom,
+ virDomainStatsRecordPtr record,
+ int *maxparams,
+ virDomainDiskDefPtr disk,
+ virStorageSourcePtr src,
This is again a bit strange. I think that for other than top-level disks
also should be gathered via monitor. In that case we'll need a way to
'alias' them from qemu's point of view in an uniform way - perhaps -
node names?
+ size_t block_idx,
+ bool abbreviated,
+ virHashTablePtr stats)
+{
+ qemuBlockStats *entry;
+ int ret = -1;
+
+ QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name",
block_idx,
+ disk->dst);
+ if (virStorageSourceIsLocalStorage(src) && src->path)
+ QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
+ block_idx, src->path);
+
+ if (abbreviated || !disk->info.alias ||
+ !(entry = virHashLookup(stats, disk->info.alias))) {
+ if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, src, NULL, NULL) < 0)
...
+ goto cleanup;
+ if (src->allocation)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "allocation", src->allocation);
+ if (src->capacity)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "capacity", src->capacity);
+ if (src->physical)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "physical", src->physical);
+ ret = 0;
+ goto cleanup;
+ }
+
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "rd.reqs", entry->rd_req);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "rd.bytes", entry->rd_bytes);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "rd.times", entry->rd_total_times);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "wr.reqs", entry->wr_req);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "wr.bytes", entry->wr_bytes);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "wr.times", entry->wr_total_times);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "fl.reqs", entry->flush_req);
+ QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
+ "fl.times", entry->flush_total_times);
+
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "allocation", entry->wr_highest_offset);
+
+ if (entry->capacity)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "capacity", entry->capacity);
+ if (entry->physical)
+ QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
+ "physical", entry->physical);
+
+ ret = 0;
+ cleanup:
+ return ret;
+}
+
+
static int
qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
virDomainObjPtr dom,
The recursive approach will be necessary, but I don't really like this
step where we gather the data differently for non-toplevel images.
Peter
2:14 a.m.
New subject: [libvirt] [PATCH 12/12] getstats: start crawling backing chain for qemu
Wire up backing chain recursion. Note that for now, we just use
the same allocation numbers for read-only backing files as what
offline domains would report. It is not the correct allocation
number for qcow2 over block devices during block-commit, and it
misses out on the fact that qemu also reports read statistics on
backing files that are worth knowing about (seriously - for a
thin-provisioned setup, it would be nice to easily get at a count
of how many reads were serviced from the backing file in relation
to reads serviced by the active layer). But it is at least
sufficient to prove that the algorithm is working, and to let
other people start coding to the interface while waiting for
later patches that get the correct information.
For a running domain, where one of the two images has a backing
file, I see the traditional output:
$ virsh domstats --block testvm2
Domain: 'testvm2'
block.count=2
block.0.name=vda
block.0.path=/tmp/wrapper.qcow2
block.0.rd.reqs=1
block.0.rd.bytes=512
block.0.rd.times=28858
block.0.wr.reqs=0
block.0.wr.bytes=0
block.0.wr.times=0
block.0.fl.reqs=0
block.0.fl.times=0
block.0.allocation=0
block.0.capacity=1310720000
block.0.physical=200704
block.1.name=vdb
block.1.path=/dev/sda7
block.1.rd.reqs=0
block.1.rd.bytes=0
block.1.rd.times=0
block.1.wr.reqs=0
block.1.wr.bytes=0
block.1.wr.times=0
block.1.fl.reqs=0
block.1.fl.times=0
block.1.allocation=0
block.1.capacity=1310720000
vs. the new output:
$ virsh domstats --block --backing testvm2
Domain: 'testvm2'
block.count=3
block.0.name=vda
block.0.path=/tmp/wrapper.qcow2
block.0.rd.reqs=1
block.0.rd.bytes=512
block.0.rd.times=28858
block.0.wr.reqs=0
block.0.wr.bytes=0
block.0.wr.times=0
block.0.fl.reqs=0
block.0.fl.times=0
block.0.allocation=0
block.0.capacity=1310720000
block.0.physical=200704
block.1.name=vda
block.1.path=/dev/sda6
block.1.backingIndex=1
block.1.allocation=1073741824
block.1.capacity=1310720000
block.1.physical=1073741824
block.2.name=vdb
block.2.path=/dev/sda7
block.2.rd.reqs=0
block.2.rd.bytes=0
block.2.rd.times=0
block.2.wr.reqs=0
block.2.wr.bytes=0
block.2.wr.times=0
block.2.fl.reqs=0
block.2.fl.times=0
block.2.allocation=0
block.2.capacity=1310720000
* src/qemu/qemu_driver.c (QEMU_DOMAIN_STATS_BACKING): New internal
enum bit.
(qemuConnectGetAllDomainStats): Recognize new user flag, and pass
details to...
(qemuDomainGetStatsBlock): ...here, where we can do longer recursion.
(qemuDomainGetStatsOneBlock): Output new field.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 55 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 46 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index feaa4a2..b57beeb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18256,8 +18256,10 @@ qemuDomainGetStatsState(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
typedef enum {
- QEMU_DOMAIN_STATS_HAVE_JOB = (1 << 0), /* job is entered, monitor can be
- accessed */
+ QEMU_DOMAIN_STATS_HAVE_JOB = 1 << 0, /* job is entered, monitor can be
+ accessed */
+ QEMU_DOMAIN_STATS_BACKING = 1 << 1, /* include backing chain in
+ block stats */
} qemuDomainStatsFlags;
@@ -18502,6 +18504,19 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver
ATTRIBUTE_UNUSED,
#undef QEMU_ADD_NET_PARAM
+#define QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, num, name, value) \
+ do { \
+ char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
+ snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
+ "block.%zu.%s", num, name); \
+ if (virTypedParamsAddUInt(&(record)->params, \
+ &(record)->nparams, \
+ maxparams, \
+ param_name, \
+ value) < 0) \
+ goto cleanup; \
+ } while (0)
+
/* expects a LL, but typed parameter must be ULL */
#define QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, num, name, value) \
do { \
@@ -18539,6 +18554,7 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
virStorageSourcePtr src,
size_t block_idx,
+ unsigned int backing_idx,
bool abbreviated,
virHashTablePtr stats)
{
@@ -18550,8 +18566,16 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
if (virStorageSourceIsLocalStorage(src) && src->path)
QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
block_idx, src->path);
+ if (backing_idx)
+ QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, block_idx, "backingIndex",
+ backing_idx);
- if (abbreviated || !disk->info.alias ||
+ /* FIXME: qemu gives information on backing files, but we aren't
+ * currently storing it into the stats table - we need a common
+ * key in qemu_monitor_json.c:qemuMonitorGetAllBlockStatsInfo and
+ * here for getting at that information, probably something like
+ * asprintf("%s.%d", alias, backing_idx). */
+ if (abbreviated || backing_idx || !disk->info.alias ||
!(entry = virHashLookup(stats, disk->info.alias))) {
if (qemuStorageLimitsRefresh(driver, cfg, dom,
disk, src, NULL, NULL) < 0)
@@ -18617,6 +18641,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
bool abbreviated = false;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
int count_index = -1;
+ size_t visited = 0;
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
abbreviated = true; /* it's ok, just go ahead silently */
@@ -18640,18 +18665,26 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
for (i = 0; i < dom->def->ndisks; i++) {
virDomainDiskDefPtr disk = dom->def->disks[i];
+ virStorageSourcePtr src = disk->src;
+ unsigned int backing_idx = 0;
- if (qemuDomainGetStatsOneBlock(driver, cfg, dom, record, maxparams,
- disk, disk->src, i, abbreviated,
- stats) < 0)
- goto cleanup;
+ while (src && (!backing_idx ||
+ (privflags & QEMU_DOMAIN_STATS_BACKING))) {
+ if (qemuDomainGetStatsOneBlock(driver, cfg, dom, record, maxparams,
+ disk, src, visited, backing_idx,
+ abbreviated, stats) < 0)
+ goto cleanup;
+ visited++;
+ backing_idx++;
+ src = src->backingStore;
+ }
}
ret = 0;
cleanup:
if (count_index >= 0)
- record->params[count_index].value.ui = i;
+ record->params[count_index].value.ui = visited;
virHashFree(stats);
virObjectUnref(cfg);
return ret;
@@ -18792,11 +18825,13 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
unsigned int domflags = 0;
if (ndoms)
- virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
+ virCheckFlags(VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING |
+ VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
else
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_BACKING |
VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
@@ -18826,6 +18861,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
if (qemuDomainGetStatsNeedMonitor(stats))
privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
+ if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
+ privflags |= QEMU_DOMAIN_STATS_BACKING;
for (i = 0; i < ndoms; i++) {
domflags = privflags;
--
1.9.3
4:08 a.m.
New subject: [libvirt] [PATCH 12/12] getstats: start crawling backing chain for qemu
On 12/06/14 09:14, Eric Blake wrote:
Wire up backing chain recursion. Note that for now, we just use
...
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 55 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 46 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index feaa4a2..b57beeb 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
...
@@ -18550,8 +18566,16 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr
driver,
if (virStorageSourceIsLocalStorage(src) && src->path)
QEMU_ADD_NAME_PARAM(record, maxparams, "block", "path",
block_idx, src->path);
+ if (backing_idx)
+ QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, block_idx, "backingIndex",
+ backing_idx);
- if (abbreviated || !disk->info.alias ||
+ /* FIXME: qemu gives information on backing files, but we aren't
+ * currently storing it into the stats table - we need a common
+ * key in qemu_monitor_json.c:qemuMonitorGetAllBlockStatsInfo and
+ * here for getting at that information, probably something like
+ * asprintf("%s.%d", alias, backing_idx). */
Breaks syntax-check:
src/qemu/qemu_driver.c:18577: * asprintf("%s.%d", alias,
backing_idx). */
maint.mk: use virAsprintf, not asprintf
+ if (abbreviated || backing_idx || !disk->info.alias ||
!(entry = virHashLookup(stats, disk->info.alias))) {
if (qemuStorageLimitsRefresh(driver, cfg, dom,
disk, src, NULL, NULL) < 0)
Rest of review will follow in-order.
Peter
8:47 a.m.
New subject: [libvirt] [PATCH 12/12] getstats: start crawling backing chain for qemu
On 12/06/14 09:14, Eric Blake wrote:
Wire up backing chain recursion. Note that for now, we just use
the same allocation numbers for read-only backing files as what
offline domains would report. It is not the correct allocation
number for qcow2 over block devices during block-commit, and it
misses out on the fact that qemu also reports read statistics on
backing files that are worth knowing about (seriously - for a
thin-provisioned setup, it would be nice to easily get at a count
of how many reads were serviced from the backing file in relation
to reads serviced by the active layer). But it is at least
sufficient to prove that the algorithm is working, and to let
other people start coding to the interface while waiting for
later patches that get the correct information.
Is that a proof-of-concept then? I'd like to avoid adding stats fields
that will change, especially this close to the release as we're running
into the risk of baking it in.
For a running domain, where one of the two images has a backing
file, I see the traditional output:
$ virsh domstats --block testvm2
Domain: 'testvm2'
block.count=2
block.0.name=vda
block.0.path=/tmp/wrapper.qcow2
block.0.rd.reqs=1
block.0.rd.bytes=512
block.0.rd.times=28858
block.0.wr.reqs=0
block.0.wr.bytes=0
block.0.wr.times=0
block.0.fl.reqs=0
block.0.fl.times=0
block.0.allocation=0
block.0.capacity=1310720000
block.0.physical=200704
block.1.name=vdb
block.1.path=/dev/sda7
block.1.rd.reqs=0
block.1.rd.bytes=0
block.1.rd.times=0
block.1.wr.reqs=0
block.1.wr.bytes=0
block.1.wr.times=0
block.1.fl.reqs=0
block.1.fl.times=0
block.1.allocation=0
block.1.capacity=1310720000
vs. the new output:
$ virsh domstats --block --backing testvm2
Domain: 'testvm2'
block.count=3
block.0.name=vda
block.0.path=/tmp/wrapper.qcow2
block.0.rd.reqs=1
block.0.rd.bytes=512
block.0.rd.times=28858
block.0.wr.reqs=0
block.0.wr.bytes=0
block.0.wr.times=0
block.0.fl.reqs=0
block.0.fl.times=0
block.0.allocation=0
block.0.capacity=1310720000
block.0.physical=200704
block.1.name=vda
block.1.path=/dev/sda6
block.1.backingIndex=1
block.1.allocation=1073741824
block.1.capacity=1310720000
block.1.physical=1073741824
block.2.name=vdb
block.2.path=/dev/sda7
block.2.rd.reqs=0
block.2.rd.bytes=0
block.2.rd.times=0
block.2.wr.reqs=0
block.2.wr.bytes=0
block.2.wr.times=0
block.2.fl.reqs=0
block.2.fl.times=0
block.2.allocation=0
block.2.capacity=1310720000
ACK to the idea of the stat output idea ...
* src/qemu/qemu_driver.c (QEMU_DOMAIN_STATS_BACKING): New internal
enum bit.
(qemuConnectGetAllDomainStats): Recognize new user flag, and pass
details to...
(qemuDomainGetStatsBlock): ...here, where we can do longer recursion.
(qemuDomainGetStatsOneBlock): Output new field.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 55 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 46 insertions(+), 9 deletions(-)
.. but we shouldn't add code that reports wrong data.
Peter
3637
days inactive
3639
days old
30 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa