9:04 a.m.
v1 was here:
https://www.redhat.com/archives/libvir-list/2014-December/msg00370.html
ACKed patches from that series have been pushed. In this series,
3, 4, and 6 are new, and others try to address some of the feedback
and deal with rebased design decisions that resulted. I still don't
have things reporting quite as nicely as I would like (it turns out
that we HAVE to stat() a file for an online domain to learn its
allocation, and that for block devices, we HAVE to open()/lseek() to
learn its physical size; meanwhile, I still want to fix
virDomainGetBlockInfo to avoid read()ing a file while qemu is
active by reusing the code that getStats uses). But I'm posting
another round now, to hopefully get early patches ACKed and into
the tree, and to demonstrate that I now have recursive stat
collection for an active domain relying solely on qemu rather than
read()ing the backing files directly.
Eric Blake (12):
qemu: refactor blockinfo job handling
qemu: let blockinfo reuse virStorageSource
getstats: prepare monitor collection for recursion
getstats: perform recursion in monitor collection
getstats: rearrange blockinfo gathering
qemu: fix bugs in blockstats
qemu: refactor blockinfo data gathering
getstats: report block sizes for offline domains
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 | 24 +-
src/libvirt-domain.c | 7 +-
src/qemu/qemu_domain.c | 19 ++
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 477 ++++++++++++++++++++++++---------------
src/qemu/qemu_migration.c | 3 +-
src/qemu/qemu_monitor.c | 24 +-
src/qemu/qemu_monitor.h | 6 +-
src/qemu/qemu_monitor_json.c | 291 ++++++++++++++----------
src/qemu/qemu_monitor_json.h | 6 +-
src/util/virstoragefile.c | 3 +-
src/util/virstoragefile.h | 3 +-
tools/virsh-domain-monitor.c | 7 +
tools/virsh.pod | 8 +-
14 files changed, 558 insertions(+), 321 deletions(-)
--
1.9.3
9:04 a.m.
New subject: [libvirt] [PATCH v2 01/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 | 65 ++++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f652237..8775ab2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11010,7 +11010,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
int format;
bool activeFail = false;
virQEMUDriverConfigPtr cfg = NULL;
- char *alias = NULL;
char *buf = NULL;
ssize_t len;
@@ -11029,11 +11028,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];
@@ -11043,36 +11050,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;
}
}
@@ -11084,17 +11091,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) {
@@ -11116,7 +11123,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;
@@ -11144,35 +11151,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)
@@ -11186,7 +11182,8 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
_("domain is not running"));
ret = -1;
}
- virObjectUnlock(vm);
+ if (vm)
+ virObjectUnlock(vm);
virObjectUnref(cfg);
return ret;
}
--
1.9.3
1:20 p.m.
New subject: [libvirt] [PATCH v2 01/12] qemu: refactor blockinfo job handling
On 12/16/14 09:04, 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.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 65 ++++++++++++++++++++++++--------------------------
1 file changed, 31 insertions(+), 34 deletions(-)
ACK,
Peter
9:04 a.m.
New subject: [libvirt] [PATCH v2 02/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; rearrange fields to match public struct.
(virStorageSourceCopy): Copy the new field.
* 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.c | 3 ++-
src/util/virstoragefile.h | 3 ++-
3 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8775ab2..b13c5e1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11045,6 +11045,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 previous 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,
@@ -11106,15 +11126,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.
@@ -11125,17 +11145,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
@@ -11157,13 +11177,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.c b/src/util/virstoragefile.c
index c424251..bd2e315 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1835,8 +1835,9 @@ virStorageSourceCopy(const virStorageSource *src,
ret->type = src->type;
ret->protocol = src->protocol;
ret->format = src->format;
- ret->allocation = src->allocation;
ret->capacity = src->capacity;
+ ret->allocation = src->allocation;
+ ret->physical = src->physical;
ret->readonly = src->readonly;
ret->shared = src->shared;
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
1:41 p.m.
New subject: [libvirt] [PATCH v2 02/12] qemu: let blockinfo reuse virStorageSource
On 12/16/14 09:04, 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; rearrange fields to match public struct.
(virStorageSourceCopy): Copy the new field.
* 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.c | 3 ++-
src/util/virstoragefile.h | 3 ++-
3 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8775ab2..b13c5e1 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11045,6 +11045,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 previous 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.
+ */
I still don't see this comment vanishing in this series. Are you planing
on getting rid of the code that opens the file on disk while the VM is
alive?
if (virStorageSourceIsLocalStorage(disk->src)) {
if (!disk->src->path) {
virReportError(VIR_ERR_INVALID_ARG,
ACK
Peter
11:04 p.m.
New subject: [libvirt] [PATCH v2 02/12] qemu: let blockinfo reuse virStorageSource
On 12/16/2014 05:41 AM, Peter Krempa wrote:
On 12/16/14 09:04, 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:
>
>
> 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.
>
>
> + /* 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 previous 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.
> + */
I still don't see this comment vanishing in this series. Are you planing
on getting rid of the code that opens the file on disk while the VM is
alive?
Correct, that's where my other email (subject "virDomainGetBlockInfo
meanings") would come into play - I'm still trying to work on additional
patches to reuse code correctly and minimize file probes where possible,
but those can be followup patches. So I will go ahead and push the
patches that have been ACKed so far.
> if (virStorageSourceIsLocalStorage(disk->src)) {
> if (!disk->src->path) {
> virReportError(VIR_ERR_INVALID_ARG,
ACK
Thanks for the reviews :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 03/12] getstats: prepare monitor collection for recursion
A future patch will allow recursion into backing chains when
collecting block stats. This patch should not change behavior,
but merely moves out the common code that will be reused once
recursion is enabled, and adds the parameter that will turn on
recursion.
* src/qemu/qemu_monitor.h (qemuMonitorGetAllBlockStatsInfo)
(qemuMonitorBlockStatsUpdateCapacity): Add recursion parameter,
although it is ignored for now.
* src/qemu/qemu_monitor.h (qemuMonitorGetAllBlockStatsInfo)
(qemuMonitorBlockStatsUpdateCapacity): Likewise.
* src/qemu/qemu_monitor_json.h
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Likewise.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Add parameter, and
split...
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne): ...into helpers.
(qemuMonitorJSONGetBlockStatsInfo): Update caller.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Update caller.
* src/qemu/qemu_migration.c (qemuMigrationCookieAddNBD): Likewise.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 +-
src/qemu/qemu_migration.c | 3 +-
src/qemu/qemu_monitor.c | 24 ++--
src/qemu/qemu_monitor.h | 6 +-
src/qemu/qemu_monitor_json.c | 257 +++++++++++++++++++++++++------------------
src/qemu/qemu_monitor_json.h | 6 +-
6 files changed, 175 insertions(+), 126 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b13c5e1..7aaee96 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18539,8 +18539,9 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
abbreviated = true; /* it's ok, just go ahead silently */
} else {
qemuDomainObjEnterMonitor(driver, dom);
- rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
- ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
+ rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats, false);
+ ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
+ false));
qemuDomainObjExitMonitor(driver, dom);
if (rc < 0) {
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0acbb57..834093d 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -571,7 +571,8 @@ qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
goto cleanup;
qemuDomainObjEnterMonitor(driver, vm);
- if (qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats) < 0) {
+ if (qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
+ false) < 0) {
qemuDomainObjExitMonitor(driver, vm);
goto cleanup;
}
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index c9c84f9..100bbd0 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1783,16 +1783,16 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
return ret;
}
-/* Fills the first 'nstats' block stats. 'stats' must be an array.
- * Returns <0 on error, otherwise the number of block stats retrieved.
- * if 'dev_name' is != NULL, look for this device only and skip
- * any other. In that case return value cannot be greater than 1.
+
+/* Creates a hash table in 'ret_stats' with all block stats.
+ * Returns <0 on error, 0 on success.
*/
int
qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
- virHashTablePtr *ret_stats)
+ virHashTablePtr *ret_stats,
+ bool backingChain)
{
- VIR_DEBUG("mon=%p ret_stats=%p", mon, ret_stats);
+ VIR_DEBUG("mon=%p ret_stats=%p, backing=%d", mon, ret_stats,
backingChain);
if (!mon->json) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1800,15 +1800,17 @@ qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
return -1;
}
- return qemuMonitorJSONGetAllBlockStatsInfo(mon, ret_stats);
+ return qemuMonitorJSONGetAllBlockStatsInfo(mon, ret_stats, backingChain);
}
/* Updates "stats" to fill virtual and physical size of the image */
-int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
- virHashTablePtr stats)
+int
+qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
+ virHashTablePtr stats,
+ bool backingChain)
{
- VIR_DEBUG("mon=%p, stats=%p", mon, stats);
+ VIR_DEBUG("mon=%p, stats=%p, backing=%d", mon, stats, backingChain);
if (!mon->json) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
@@ -1816,7 +1818,7 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
return -1;
}
- return qemuMonitorJSONBlockStatsUpdateCapacity(mon, stats);
+ return qemuMonitorJSONBlockStatsUpdateCapacity(mon, stats, backingChain);
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 21533a4..edab66f 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -382,11 +382,13 @@ struct _qemuBlockStats {
};
int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
- virHashTablePtr *ret_stats)
+ virHashTablePtr *ret_stats,
+ bool backingChain)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
- virHashTablePtr stats)
+ virHashTablePtr stats,
+ bool backingChain)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 162579b..3a13890 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1627,7 +1627,7 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
if (flush_total_times)
*flush_total_times = -1;
- if (qemuMonitorJSONGetAllBlockStatsInfo(mon, &blockstats) < 0)
+ if (qemuMonitorJSONGetAllBlockStatsInfo(mon, &blockstats, false) < 0)
goto cleanup;
if (!(stats = virHashLookup(blockstats, dev_name))) {
@@ -1693,16 +1693,112 @@ qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev,
}
-int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
- virHashTablePtr *ret_stats)
+static int
+qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
+ const char *dev_name,
+ virHashTablePtr hash,
+ bool backingChain ATTRIBUTE_UNUSED)
{
- int ret = -1;
- int rc;
- size_t i;
- virJSONValuePtr cmd;
- virJSONValuePtr reply = NULL;
- virJSONValuePtr devices;
qemuBlockStatsPtr bstats = NULL;
+ virJSONValuePtr stats;
+ int ret = -1;
+
+ if (VIR_ALLOC(bstats) < 0)
+ goto cleanup;
+
+ if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL ||
+ stats->type != VIR_JSON_TYPE_OBJECT) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("blockstats stats entry was not "
+ "in expected format"));
+ goto cleanup;
+ }
+
+ if (virJSONValueObjectGetNumberLong(stats, "rd_bytes",
+ &bstats->rd_bytes) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_bytes");
+ goto cleanup;
+ }
+ if (virJSONValueObjectGetNumberLong(stats, "rd_operations",
+ &bstats->rd_req) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_operations");
+ goto cleanup;
+ }
+ if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") &&
+ (virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns",
+ &bstats->rd_total_times) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "rd_total_time_ns");
+ goto cleanup;
+ }
+ if (virJSONValueObjectGetNumberLong(stats, "wr_bytes",
+ &bstats->wr_bytes) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "wr_bytes");
+ goto cleanup;
+ }
+ if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
+ &bstats->wr_req) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "wr_operations");
+ goto cleanup;
+ }
+ if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
+ (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns",
+ &bstats->wr_total_times) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "wr_total_time_ns");
+ goto cleanup;
+ }
+ if (virJSONValueObjectHasKey(stats, "flush_operations") &&
+ (virJSONValueObjectGetNumberLong(stats, "flush_operations",
+ &bstats->flush_req) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "flush_operations");
+ goto cleanup;
+ }
+ if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") &&
+ (virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns",
+ &bstats->flush_total_times) < 0)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot read %s statistic"),
+ "flush_total_time_ns");
+ goto cleanup;
+ }
+
+ /* it's ok to not have this information here. Just skip silently. */
+ qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
+
+ if (virHashAddEntry(hash, dev_name, bstats) < 0)
+ goto cleanup;
+ bstats = NULL;
+ ret = 0;
+ cleanup:
+ VIR_FREE(bstats);
+ return ret;
+}
+
+
+int
+qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
+ virHashTablePtr *ret_stats,
+ bool backingChain)
+{
+ int ret = -1;
+ int rc;
+ size_t i;
+ virJSONValuePtr cmd;
+ virJSONValuePtr reply = NULL;
+ virJSONValuePtr devices;
virHashTablePtr hash = NULL;
if (!(cmd = qemuMonitorJSONMakeCommand("query-blockstats", NULL)))
@@ -1726,12 +1822,8 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
for (i = 0; i < virJSONValueArraySize(devices); i++) {
virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
- virJSONValuePtr stats;
const char *dev_name;
- if (VIR_ALLOC(bstats) < 0)
- goto cleanup;
-
if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("blockstats device entry was not "
@@ -1749,81 +1841,10 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
if (STRPREFIX(dev_name, QEMU_DRIVE_HOST_PREFIX))
dev_name += strlen(QEMU_DRIVE_HOST_PREFIX);
- if ((stats = virJSONValueObjectGet(dev, "stats")) == NULL ||
- stats->type != VIR_JSON_TYPE_OBJECT) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("blockstats stats entry was not "
- "in expected format"));
+ if (qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, hash,
+ backingChain) < 0)
goto cleanup;
- }
- if (virJSONValueObjectGetNumberLong(stats, "rd_bytes",
- &bstats->rd_bytes) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_bytes");
- goto cleanup;
- }
- if (virJSONValueObjectGetNumberLong(stats, "rd_operations",
- &bstats->rd_req) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_operations");
- goto cleanup;
- }
- if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") &&
- (virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns",
- &bstats->rd_total_times) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "rd_total_time_ns");
- goto cleanup;
- }
- if (virJSONValueObjectGetNumberLong(stats, "wr_bytes",
- &bstats->wr_bytes) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_bytes");
- goto cleanup;
- }
- if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
- &bstats->wr_req) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_operations");
- goto cleanup;
- }
- if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
- (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns",
- &bstats->wr_total_times) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "wr_total_time_ns");
- goto cleanup;
- }
- if (virJSONValueObjectHasKey(stats, "flush_operations") &&
- (virJSONValueObjectGetNumberLong(stats, "flush_operations",
- &bstats->flush_req) < 0)) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "flush_operations");
- goto cleanup;
- }
- if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") &&
- (virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns",
- &bstats->flush_total_times) < 0))
{
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot read %s statistic"),
- "flush_total_time_ns");
- goto cleanup;
- }
-
- /* it's ok to not have this information here. Just skip silently. */
- qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
-
- if (virHashAddEntry(hash, dev_name, bstats) < 0)
- goto cleanup;
- bstats = NULL;
}
*ret_stats = hash;
@@ -1831,7 +1852,6 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
ret = 0;
cleanup:
- VIR_FREE(bstats);
virHashFree(hash);
virJSONValueFree(cmd);
virJSONValueFree(reply);
@@ -1839,8 +1859,45 @@ int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
}
-int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
- virHashTablePtr stats)
+static int
+qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image,
+ const char *dev_name,
+ virHashTablePtr stats,
+ bool backingChain ATTRIBUTE_UNUSED)
+{
+ qemuBlockStatsPtr bstats;
+ int ret = -1;
+
+ if (!(bstats = virHashLookup(stats, dev_name))) {
+ if (VIR_ALLOC(bstats) < 0)
+ goto cleanup;
+
+ if (virHashAddEntry(stats, dev_name, bstats) < 0) {
+ VIR_FREE(bstats);
+ goto cleanup;
+ }
+ }
+
+ /* After this point, we ignore failures; the stats were
+ * zero-initialized when created which is a sane fallback. */
+ ret = 0;
+ if (virJSONValueObjectGetNumberUlong(image, "virtual-size",
+ &bstats->capacity) < 0)
+ goto cleanup;
+
+ /* if actual-size is missing, image is not thin provisioned */
+ if (virJSONValueObjectGetNumberUlong(image, "actual-size",
+ &bstats->physical) < 0)
+ bstats->physical = bstats->capacity;
+ cleanup:
+ return ret;
+}
+
+
+int
+qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
+ virHashTablePtr stats,
+ bool backingChain)
{
int ret = -1;
int rc;
@@ -1869,7 +1926,6 @@ int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
virJSONValuePtr inserted;
virJSONValuePtr image;
- qemuBlockStatsPtr bstats;
const char *dev_name;
if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
@@ -1894,24 +1950,9 @@ int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
!(image = virJSONValueObjectGet(inserted, "image")))
continue;
- if (!(bstats = virHashLookup(stats, dev_name))) {
- if (VIR_ALLOC(bstats) < 0)
- goto cleanup;
-
- if (virHashAddEntry(stats, dev_name, bstats) < 0) {
- VIR_FREE(bstats);
- goto cleanup;
- }
- }
-
- if (virJSONValueObjectGetNumberUlong(image, "virtual-size",
- &bstats->capacity) < 0)
- continue;
-
- /* if actual-size is missing, image is not thin provisioned */
- if (virJSONValueObjectGetNumberUlong(image, "actual-size",
- &bstats->physical) < 0)
- bstats->physical = bstats->capacity;
+ if (qemuMonitorJSONBlockStatsUpdateCapacityOne(image, dev_name, stats,
+ backingChain) < 0)
+ goto cleanup;
}
ret = 0;
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index ae20fb1..222f11e 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -80,9 +80,11 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
long long *flush_total_times,
long long *errs);
int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
- virHashTablePtr *ret_stats);
+ virHashTablePtr *ret_stats,
+ bool backingChain);
int qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
- virHashTablePtr stats);
+ virHashTablePtr stats,
+ bool backingChain);
int qemuMonitorJSONGetBlockStatsParamsNumber(qemuMonitorPtr mon,
int *nparams);
int qemuMonitorJSONGetBlockExtent(qemuMonitorPtr mon,
--
1.9.3
1:54 p.m.
New subject: [libvirt] [PATCH v2 03/12] getstats: prepare monitor collection for recursion
On 12/16/14 09:04, Eric Blake wrote:
A future patch will allow recursion into backing chains when
collecting block stats. This patch should not change behavior,
but merely moves out the common code that will be reused once
recursion is enabled, and adds the parameter that will turn on
recursion.
* src/qemu/qemu_monitor.h (qemuMonitorGetAllBlockStatsInfo)
(qemuMonitorBlockStatsUpdateCapacity): Add recursion parameter,
although it is ignored for now.
* src/qemu/qemu_monitor.h (qemuMonitorGetAllBlockStatsInfo)
(qemuMonitorBlockStatsUpdateCapacity): Likewise.
* src/qemu/qemu_monitor_json.h
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Likewise.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Add parameter, and
split...
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne): ...into helpers.
(qemuMonitorJSONGetBlockStatsInfo): Update caller.
* src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Update caller.
* src/qemu/qemu_migration.c (qemuMigrationCookieAddNBD): Likewise.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 5 +-
src/qemu/qemu_migration.c | 3 +-
src/qemu/qemu_monitor.c | 24 ++--
src/qemu/qemu_monitor.h | 6 +-
src/qemu/qemu_monitor_json.c | 257 +++++++++++++++++++++++++------------------
src/qemu/qemu_monitor_json.h | 6 +-
6 files changed, 175 insertions(+), 126 deletions(-)
ACK,
Peter
9:04 a.m.
New subject: [libvirt] [PATCH v2 04/12] getstats: perform recursion in monitor collection
When requested in a later patch, the QMP command results are now
examined recursively. As qemu_driver will eventually have to
read items out of the hash table as stored by this patch, the
computation of backing alias string is done in a shared location.
* src/qemu/qemu_domain.h (qemuDomainStorageAlias): New prototype.
* src/qemu/qemu_domain.c (qemuDomainStorageAlias): Implement it.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne): Perform recursion.
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Update callers.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_domain.c | 16 +++++++++++++++
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_monitor_json.c | 48 ++++++++++++++++++++++++++++++++------------
3 files changed, 52 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 220304f..02887cd 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2693,6 +2693,22 @@ qemuDomainStorageFileInit(virQEMUDriverPtr driver,
}
+char *
+qemuDomainStorageAlias(const char *device, int depth)
+{
+ char *alias;
+
+ if (STRPREFIX(device, QEMU_DRIVE_HOST_PREFIX))
+ device += strlen(QEMU_DRIVE_HOST_PREFIX);
+
+ if (!depth)
+ ignore_value(VIR_STRDUP(alias, device));
+ else
+ ignore_value(virAsprintf(&alias, "%s.%d", device, depth));
+ return alias;
+}
+
+
int
qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
virDomainObjPtr vm,
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index efabd82..288b601 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -375,6 +375,7 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
int qemuDomainStorageFileInit(virQEMUDriverPtr driver,
virDomainObjPtr vm,
virStorageSourcePtr src);
+char *qemuDomainStorageAlias(const char *device, int depth);
int qemuDomainCleanupAdd(virDomainObjPtr vm,
qemuDomainCleanupCallback cb);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3a13890..e567aa7 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -1696,13 +1696,18 @@ qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev,
static int
qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
const char *dev_name,
+ int depth,
virHashTablePtr hash,
- bool backingChain ATTRIBUTE_UNUSED)
+ bool backingChain)
{
qemuBlockStatsPtr bstats = NULL;
virJSONValuePtr stats;
int ret = -1;
+ char *entry_name = qemuDomainStorageAlias(dev_name, depth);
+ virJSONValuePtr backing;
+ if (!entry_name)
+ goto cleanup;
if (VIR_ALLOC(bstats) < 0)
goto cleanup;
@@ -1778,12 +1783,20 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
/* it's ok to not have this information here. Just skip silently. */
qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
- if (virHashAddEntry(hash, dev_name, bstats) < 0)
+ if (virHashAddEntry(hash, entry_name, bstats) < 0)
goto cleanup;
bstats = NULL;
+
+ if (backingChain &&
+ (backing = virJSONValueObjectGet(dev, "backing")) &&
+ qemuMonitorJSONGetOneBlockStatsInfo(backing, dev_name, depth + 1,
+ hash, true) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
VIR_FREE(bstats);
+ VIR_FREE(entry_name);
return ret;
}
@@ -1838,10 +1851,7 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
goto cleanup;
}
- if (STRPREFIX(dev_name, QEMU_DRIVE_HOST_PREFIX))
- dev_name += strlen(QEMU_DRIVE_HOST_PREFIX);
-
- if (qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, hash,
+ if (qemuMonitorJSONGetOneBlockStatsInfo(dev, dev_name, 0, hash,
backingChain) < 0)
goto cleanup;
@@ -1862,17 +1872,20 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
static int
qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image,
const char *dev_name,
+ int depth,
virHashTablePtr stats,
- bool backingChain ATTRIBUTE_UNUSED)
+ bool backingChain)
{
qemuBlockStatsPtr bstats;
int ret = -1;
+ char *entry_name = qemuDomainStorageAlias(dev_name, depth);
+ virJSONValuePtr backing;
- if (!(bstats = virHashLookup(stats, dev_name))) {
+ if (!(bstats = virHashLookup(stats, entry_name))) {
if (VIR_ALLOC(bstats) < 0)
goto cleanup;
- if (virHashAddEntry(stats, dev_name, bstats) < 0) {
+ if (virHashAddEntry(stats, entry_name, bstats) < 0) {
VIR_FREE(bstats);
goto cleanup;
}
@@ -1889,7 +1902,18 @@ qemuMonitorJSONBlockStatsUpdateCapacityOne(virJSONValuePtr image,
if (virJSONValueObjectGetNumberUlong(image, "actual-size",
&bstats->physical) < 0)
bstats->physical = bstats->capacity;
+
+ if (backingChain &&
+ (backing = virJSONValueObjectGet(image, "backing-image"))) {
+ ret = qemuMonitorJSONBlockStatsUpdateCapacityOne(backing,
+ dev_name,
+ depth + 1,
+ stats,
+ true);
+ }
+
cleanup:
+ VIR_FREE(entry_name);
return ret;
}
@@ -1942,15 +1966,13 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
goto cleanup;
}
- if (STRPREFIX(dev_name, QEMU_DRIVE_HOST_PREFIX))
- dev_name += strlen(QEMU_DRIVE_HOST_PREFIX);
-
/* drive may be empty */
if (!(inserted = virJSONValueObjectGet(dev, "inserted")) ||
!(image = virJSONValueObjectGet(inserted, "image")))
continue;
- if (qemuMonitorJSONBlockStatsUpdateCapacityOne(image, dev_name, stats,
+ if (qemuMonitorJSONBlockStatsUpdateCapacityOne(image, dev_name, 0,
+ stats,
backingChain) < 0)
goto cleanup;
}
--
1.9.3
2:41 p.m.
New subject: [libvirt] [PATCH v2 04/12] getstats: perform recursion in monitor collection
On 12/16/14 09:04, Eric Blake wrote:
When requested in a later patch, the QMP command results are now
examined recursively. As qemu_driver will eventually have to
read items out of the hash table as stored by this patch, the
computation of backing alias string is done in a shared location.
* src/qemu/qemu_domain.h (qemuDomainStorageAlias): New prototype.
* src/qemu/qemu_domain.c (qemuDomainStorageAlias): Implement it.
* src/qemu/qemu_monitor_json.c
(qemuMonitorJSONGetOneBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacityOne): Perform recursion.
(qemuMonitorJSONGetAllBlockStatsInfo)
(qemuMonitorJSONBlockStatsUpdateCapacity): Update callers.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_domain.c | 16 +++++++++++++++
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_monitor_json.c | 48 ++++++++++++++++++++++++++++++++------------
3 files changed, 52 insertions(+), 13 deletions(-)
ACK,
finally code that grabs the data from the monitor.
Peter
9:04 a.m.
New subject: [libvirt] [PATCH v2 05/12] getstats: rearrange blockinfo gathering
We want to avoid read()ing a file while qemu is running. We still
have to open() block devices to determine their physical size, but
that is safer. This patch rearranges code to make it easier to
skip the metadata collection when possible.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
disk up front. Perform stat first. Place metadata reading next
to use.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 61 ++++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7aaee96..f87c44b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11044,6 +11044,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
}
disk = vm->def->disks[idx];
+ if (virStorageSourceIsEmpty(disk->src)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("disk '%s' does not currently have a source
assigned"),
+ path);
+ goto endjob;
+ }
/* FIXME: For an offline domain, we always want to check current
* on-disk statistics (as users have been known to change offline
@@ -11066,10 +11072,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* change.
*/
if (virStorageSourceIsLocalStorage(disk->src)) {
- if (!disk->src->path) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("disk '%s' does not currently have a source
assigned"),
- path);
+ /* Yes, this is a mild TOCTTOU race, but if someone is
+ * changing files in the background behind libvirt's back,
+ * they deserve bogus information. */
+ if (stat(disk->src->path, &sb) < 0) {
+ virReportSystemError(errno,
+ _("cannot stat file '%s'"),
disk->src->path);
goto endjob;
}
@@ -11077,12 +11085,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
NULL, NULL)) == -1)
goto endjob;
- if (fstat(fd, &sb) < 0) {
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
disk->src->path);
- goto endjob;
- }
-
if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
virReportSystemError(errno, _("cannot read header '%s'"),
disk->src->path);
@@ -11092,37 +11094,17 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (virStorageFileInitAs(disk->src, cfg->user, cfg->group) < 0)
goto endjob;
- if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
- &buf)) < 0)
- goto endjob;
-
if (virStorageFileStat(disk->src, &sb) < 0) {
virReportSystemError(errno, _("failed to stat remote file
'%s'"),
NULLSTR(disk->src->path));
goto endjob;
}
- }
-
- /* Probe for magic formats */
- if (virDomainDiskGetFormat(disk)) {
- format = virDomainDiskGetFormat(disk);
- } else {
- if (!cfg->allowDiskFormatProbing) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("no disk format for %s and probing is disabled"),
- path);
- goto endjob;
- }
- if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
- buf, len)) < 0)
+ if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
+ &buf)) < 0)
goto endjob;
}
- if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
- format, NULL)))
- goto endjob;
-
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
@@ -11151,6 +11133,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* If the file we probed has a capacity set, then override
* what we calculated from file/block extents */
+ if (!(format = virDomainDiskGetFormat(disk))) {
+ if (!cfg->allowDiskFormatProbing) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("no disk format for %s and probing is disabled"),
+ path);
+ goto endjob;
+ }
+
+ if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
+ buf, len)) < 0)
+ goto endjob;
+ }
+ if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
+ format, NULL)))
+ goto endjob;
if (meta->capacity)
disk->src->capacity = meta->capacity;
--
1.9.3
2:54 p.m.
New subject: [libvirt] [PATCH v2 05/12] getstats: rearrange blockinfo gathering
On 12/16/14 09:04, Eric Blake wrote:
We want to avoid read()ing a file while qemu is running. We still
have to open() block devices to determine their physical size, but
that is safer. This patch rearranges code to make it easier to
skip the metadata collection when possible.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
disk up front. Perform stat first. Place metadata reading next
to use.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 61 ++++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7aaee96..f87c44b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11066,10 +11072,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* change.
*/
if (virStorageSourceIsLocalStorage(disk->src)) {
- if (!disk->src->path) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("disk '%s' does not currently have a source
assigned"),
- path);
+ /* Yes, this is a mild TOCTTOU race, but if someone is
+ * changing files in the background behind libvirt's back,
+ * they deserve bogus information. */
+ if (stat(disk->src->path, &sb) < 0) {
+ virReportSystemError(errno,
+ _("cannot stat file '%s'"),
disk->src->path);
goto endjob;
}
Um this will cause problems on NFS. The code after that that you are
moving uses the FD to do the stat() call. The fd is opened by the helper
that opens the file with correct perms.
@@ -11077,12 +11085,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
NULL, NULL)) == -1)
goto endjob;
- if (fstat(fd, &sb) < 0) {
- virReportSystemError(errno,
- _("cannot stat file '%s'"),
disk->src->path);
- goto endjob;
- }
-
if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) {
virReportSystemError(errno, _("cannot read header '%s'"),
disk->src->path);
Otherwise looks okay.
Peter
12:33 a.m.
New subject: [libvirt] [PATCH v2 05/12] getstats: rearrange blockinfo gathering
On 12/16/2014 06:54 AM, Peter Krempa wrote:
On 12/16/14 09:04, Eric Blake wrote:
> We want to avoid read()ing a file while qemu is running. We still
> have to open() block devices to determine their physical size, but
> that is safer. This patch rearranges code to make it easier to
> skip the metadata collection when possible.
>
> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
> disk up front. Perform stat first. Place metadata reading next
> to use.
>
> + if (stat(disk->src->path, &sb) < 0) {
> + virReportSystemError(errno,
> + _("cannot stat file '%s'"),
disk->src->path);
> goto endjob;
> }
Um this will cause problems on NFS. The code after that that you are
moving uses the FD to do the stat() call. The fd is opened by the helper
that opens the file with correct perms.
Oh, you're right. I'll see how hairy it is to pull this patch out of
the current series, and save the rework of avoiding open()ing the files
to later (I may end up with a situation that is less efficient, by
possibly open()ing some files twice for offline domains, once for
capacity, and once for determining the last offset of a block device,
instead of the current code that tries to do both from a single fd but
becomes harder to untangle). 1-4 are pushed, and I'm seeing what
happens to the rest of the series if I leave this out for now...
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:30 a.m.
New subject: [libvirt] [PATCH v2 05/12] getstats: rearrange blockinfo gathering
On 12/16/2014 04:33 PM, Eric Blake wrote:
On 12/16/2014 06:54 AM, Peter Krempa wrote:
> On 12/16/14 09:04, Eric Blake wrote:
>> We want to avoid read()ing a file while qemu is running. We still
>> have to open() block devices to determine their physical size, but
>> that is safer. This patch rearranges code to make it easier to
>> skip the metadata collection when possible.
>>
>> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): Check for empty
>> disk up front. Perform stat first. Place metadata reading next
>> to use.
>>
>> + if (stat(disk->src->path, &sb) < 0) {
>> + virReportSystemError(errno,
>> + _("cannot stat file '%s'"),
disk->src->path);
>> goto endjob;
>> }
>
> Um this will cause problems on NFS. The code after that that you are
> moving uses the FD to do the stat() call. The fd is opened by the helper
> that opens the file with correct perms.
Oh, you're right. I'll see how hairy it is to pull this patch out of
the current series, and save the rework of avoiding open()ing the files
to later (I may end up with a situation that is less efficient, by
possibly open()ing some files twice for offline domains, once for
capacity, and once for determining the last offset of a block device,
instead of the current code that tries to do both from a single fd but
becomes harder to untangle). 1-4 are pushed, and I'm seeing what
happens to the rest of the series if I leave this out for now...
> Otherwise looks okay.
>
> Peter
Turned out to be a little bit hairy; what I ended up pushing was JUST
the refactoring for earlier checking for empty images and later grouping
of read-related code, leaving the open-code untouched for later:
diff --git c/src/qemu/qemu_driver.c i/src/qemu/qemu_driver.c
index 75ea218..3d81a5b 100644
--- c/src/qemu/qemu_driver.c
+++ i/src/qemu/qemu_driver.c
@@ -11057,6 +11057,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
disk = vm->def->disks[idx];
src = disk->src;
+ if (virStorageSourceIsEmpty(src)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("disk '%s' does not currently have a source
assigned"),
+ path);
+ goto endjob;
+ }
/* FIXME: For an offline domain, we always want to check current
* on-disk statistics (as users have been known to change offline
@@ -11079,13 +11085,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* change.
*/
if (virStorageSourceIsLocalStorage(src)) {
- if (!src->path) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("disk '%s' does not currently have a
source assigned"),
- path);
- goto endjob;
- }
-
if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY,
NULL, NULL)) == -1)
goto endjob;
@@ -11116,26 +11115,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
}
}
- /* Probe for magic formats */
- if (virDomainDiskGetFormat(disk)) {
- format = virDomainDiskGetFormat(disk);
- } else {
- if (!cfg->allowDiskFormatProbing) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("no disk format for %s and probing is
disabled"),
- path);
- goto endjob;
- }
-
- if ((format = virStorageFileProbeFormatFromBuf(src->path,
- buf, len)) < 0)
- goto endjob;
- }
-
- if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
- format, NULL)))
- goto endjob;
-
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
@@ -11164,6 +11143,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* If the file we probed has a capacity set, then override
* what we calculated from file/block extents */
+ if (!(format = src->format)) {
+ if (!cfg->allowDiskFormatProbing) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("no disk format for %s and probing is
disabled"),
+ path);
+ goto endjob;
+ }
+
+ if ((format = virStorageFileProbeFormatFromBuf(src->path,
+ buf, len)) < 0)
+ goto endjob;
+ }
+ if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
+ format, NULL)))
+ goto endjob;
if (meta->capacity)
src->capacity = meta->capacity;
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats
The documentation for virDomainBlockInfo was confusing: it stated
that 'physical' was the size of the container, then gave an example
of it being the amount of storage used by a sparse file (that is,
for a sparse raw image on a regular file, the wording implied
capacity==physical, while allocation was smaller; but the example
instead claimed physical==allocation). Since we use 'physical' for
the last offset of a block device, we should do likewise for
regular files.
Furthermore, the example claimed that for a qcow2 regular file,
allocation==physical. At the time the code was first written,
this was true (qcow2 files were allocated sequentially, and were
never sparse, so the last sector written happened to also match
the disk space occupied); but modern qemu does much better and
can punch holes for a qcow2 with allocation < physical.
Basically, after this patch, the three fields are now reliably
mapped as:
'capacity' - how much storage the guest can see (equal to
physical for raw images, determined by image metadata otherwise)
'allocation' - how much storage the image occupies (similar to
what 'du' would report)
'physical' - the last offset of the image (similar to what 'ls'
would report)
'capacity' can be larger than 'physical' (such as for a qcow2
image that does not vary much from a backing file) or smaller
(such as for a qcow2 file with lots of internal snapshots).
Likewise, 'allocation' can be (slightly) larger than 'physical'
(such as counting the tail of cluster allocations required to
round a file size up to filesystem granularity) or smaller
(for a sparse file). A block-resize operation changes capacity
(which, for raw images, also changes physical); many non-raw
images automatically grow physical and allocation as necessary
when starting with an allocation smaller than capacity; and even
when capacity and physical stay unchanged, allocation can change
when converting sectors from holes to data or back.
Note that this does not change semantics for qcow2 images stored
on block devices; there, we still rely on qemu to report the
highest written extent for allocation. So using this API to
track when to extend a block device because a qcow2 image is
about to exceed a threshold will not see any changes.
* include/libvirt/libvirt-domain.h (_virDomainBlockInfo): Tweak
documentation to match saner definition.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): For regular
files, physical size is capacity, not allocation.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
include/libvirt/libvirt-domain.h | 23 ++++++++++++++--------
src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++------------------
2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index ae2c49c..baef32d 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1356,7 +1356,7 @@ int virDomainBlockResize (virDomainPtr dom,
/** virDomainBlockInfo:
*
* This struct provides information about the size of a block device
- * backing store
+ * backing store.
*
* Examples:
*
@@ -1364,13 +1364,13 @@ int virDomainBlockResize (virDomainPtr dom,
* * capacity, allocation, physical: All the same
*
* - Sparse raw file in filesystem:
- * * capacity: logical size of the file
- * * allocation, physical: number of blocks allocated to file
+ * * capacity, size: logical size of the file
+ * * allocation: disk space occupied by file
*
* - qcow2 file in filesystem
* * capacity: logical size from qcow2 header
- * * allocation, physical: logical size of the file /
- * highest qcow extent (identical)
+ * * allocation: disk space occupied by file
+ * * physical: reported size of qcow2 file
*
* - qcow2 file in a block device
* * capacity: logical size from qcow2 header
@@ -1380,9 +1380,16 @@ int virDomainBlockResize (virDomainPtr dom,
typedef struct _virDomainBlockInfo virDomainBlockInfo;
typedef virDomainBlockInfo *virDomainBlockInfoPtr;
struct _virDomainBlockInfo {
- unsigned long long capacity; /* logical size in bytes of the block device backing
image */
- unsigned long long allocation; /* highest allocated extent in bytes of the block
device backing image */
- unsigned long long physical; /* physical size in bytes of the container of the
backing image */
+ unsigned long long capacity; /* logical size in bytes of the
+ * image (how much storage the
+ * guest will see) */
+ unsigned long long allocation; /* host storage in bytes occupied
+ * by the image (such as highest
+ * allocated extent if there are no
+ * holes, similar to 'du') */
+ unsigned long long physical; /* host physical size in bytes of
+ * the image container (last
+ * offset, similar to 'ls')*/
};
int virDomainGetBlockInfo(virDomainPtr dom,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f87c44b..5e9c133 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11108,18 +11108,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- disk->src->physical = (unsigned long long)sb.st_blocks *
+ disk->src->allocation = (unsigned long long)sb.st_blocks *
(unsigned long long)DEV_BSIZE;
#else
+ disk->src->allocation = sb.st_size;
+#endif
+ /* Allocation tracks when the file is sparse, physical is the
+ * last offset of the file. */
disk->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;
} else {
- /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
- * be 64 bits on all platforms.
+ /* NB. Because we configure with AC_SYS_LARGEFILE, off_t
+ * should be 64 bits on all platforms. For block devices, we
+ * have to seek (safe even if someone else is writing) to
+ * determine physical size, and assume that allocation is the
+ * same as physical (but can refine that assumption later if
+ * qemu is still running).
*/
end = lseek(fd, 0, SEEK_END);
if (end == (off_t)-1) {
@@ -11128,11 +11131,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto endjob;
}
disk->src->physical = end;
- disk->src->capacity = end;
+ disk->src->allocation = end;
}
- /* If the file we probed has a capacity set, then override
- * what we calculated from file/block extents */
+ /* Raw files: capacity is physical size. For all other files: if
+ * the metadata has a capacity, use that, otherwise fall back to
+ * physical size. */
if (!(format = virDomainDiskGetFormat(disk))) {
if (!cfg->allowDiskFormatProbing) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -11145,16 +11149,16 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
buf, len)) < 0)
goto endjob;
}
- if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
- format, NULL)))
+ if (format == VIR_STORAGE_FILE_RAW) {
+ disk->src->capacity = disk->src->physical;
+ } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf,
+ len, format, NULL))) {
goto endjob;
- if (meta->capacity)
- disk->src->capacity = meta->capacity;
+ disk->src->capacity = meta->capacity ? meta->capacity
+ : disk->src->physical;
+ }
- /* Set default value .. */
- disk->src->allocation = disk->src->physical;
-
- /* ..but if guest is not using raw disk format and on a block device,
+ /* 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 &&
--
1.9.3
2:17 p.m.
New subject: [libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats
Ran the series through my local Coverity checker.... found one issue....
On 12/16/2014 03:04 AM, Eric Blake wrote:
The documentation for virDomainBlockInfo was confusing: it stated
that 'physical' was the size of the container, then gave an example
of it being the amount of storage used by a sparse file (that is,
for a sparse raw image on a regular file, the wording implied
capacity==physical, while allocation was smaller; but the example
instead claimed physical==allocation). Since we use 'physical' for
the last offset of a block device, we should do likewise for
regular files.
Furthermore, the example claimed that for a qcow2 regular file,
allocation==physical. At the time the code was first written,
this was true (qcow2 files were allocated sequentially, and were
never sparse, so the last sector written happened to also match
the disk space occupied); but modern qemu does much better and
can punch holes for a qcow2 with allocation < physical.
Basically, after this patch, the three fields are now reliably
mapped as:
'capacity' - how much storage the guest can see (equal to
physical for raw images, determined by image metadata otherwise)
'allocation' - how much storage the image occupies (similar to
what 'du' would report)
'physical' - the last offset of the image (similar to what 'ls'
would report)
'capacity' can be larger than 'physical' (such as for a qcow2
image that does not vary much from a backing file) or smaller
(such as for a qcow2 file with lots of internal snapshots).
Likewise, 'allocation' can be (slightly) larger than 'physical'
(such as counting the tail of cluster allocations required to
round a file size up to filesystem granularity) or smaller
(for a sparse file). A block-resize operation changes capacity
(which, for raw images, also changes physical); many non-raw
images automatically grow physical and allocation as necessary
when starting with an allocation smaller than capacity; and even
when capacity and physical stay unchanged, allocation can change
when converting sectors from holes to data or back.
Note that this does not change semantics for qcow2 images stored
on block devices; there, we still rely on qemu to report the
highest written extent for allocation. So using this API to
track when to extend a block device because a qcow2 image is
about to exceed a threshold will not see any changes.
* include/libvirt/libvirt-domain.h (_virDomainBlockInfo): Tweak
documentation to match saner definition.
* src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): For regular
files, physical size is capacity, not allocation.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
include/libvirt/libvirt-domain.h | 23 ++++++++++++++--------
src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++------------------
2 files changed, 38 insertions(+), 27 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index ae2c49c..baef32d 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1356,7 +1356,7 @@ int virDomainBlockResize (virDomainPtr dom,
/** virDomainBlockInfo:
*
* This struct provides information about the size of a block device
- * backing store
+ * backing store.
*
* Examples:
*
@@ -1364,13 +1364,13 @@ int virDomainBlockResize (virDomainPtr dom,
* * capacity, allocation, physical: All the same
*
* - Sparse raw file in filesystem:
- * * capacity: logical size of the file
- * * allocation, physical: number of blocks allocated to file
+ * * capacity, size: logical size of the file
+ * * allocation: disk space occupied by file
*
* - qcow2 file in filesystem
* * capacity: logical size from qcow2 header
- * * allocation, physical: logical size of the file /
- * highest qcow extent (identical)
+ * * allocation: disk space occupied by file
+ * * physical: reported size of qcow2 file
*
* - qcow2 file in a block device
* * capacity: logical size from qcow2 header
@@ -1380,9 +1380,16 @@ int virDomainBlockResize (virDomainPtr dom,
typedef struct _virDomainBlockInfo virDomainBlockInfo;
typedef virDomainBlockInfo *virDomainBlockInfoPtr;
struct _virDomainBlockInfo {
- unsigned long long capacity; /* logical size in bytes of the block device backing
image */
- unsigned long long allocation; /* highest allocated extent in bytes of the block
device backing image */
- unsigned long long physical; /* physical size in bytes of the container of the
backing image */
+ unsigned long long capacity; /* logical size in bytes of the
+ * image (how much storage the
+ * guest will see) */
+ unsigned long long allocation; /* host storage in bytes occupied
+ * by the image (such as highest
+ * allocated extent if there are no
+ * holes, similar to 'du') */
+ unsigned long long physical; /* host physical size in bytes of
+ * the image container (last
+ * offset, similar to 'ls')*/
};
int virDomainGetBlockInfo(virDomainPtr dom,
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f87c44b..5e9c133 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11108,18 +11108,21 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- disk->src->physical = (unsigned long long)sb.st_blocks *
+ disk->src->allocation = (unsigned long long)sb.st_blocks *
(unsigned long long)DEV_BSIZE;
#else
+ disk->src->allocation = sb.st_size;
+#endif
+ /* Allocation tracks when the file is sparse, physical is the
+ * last offset of the file. */
disk->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;
} else {
- /* NB. Because we configure with AC_SYS_LARGEFILE, off_t should
- * be 64 bits on all platforms.
+ /* NB. Because we configure with AC_SYS_LARGEFILE, off_t
+ * should be 64 bits on all platforms. For block devices, we
+ * have to seek (safe even if someone else is writing) to
+ * determine physical size, and assume that allocation is the
+ * same as physical (but can refine that assumption later if
+ * qemu is still running).
*/
end = lseek(fd, 0, SEEK_END);
if (end == (off_t)-1) {
@@ -11128,11 +11131,12 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
goto endjob;
}
disk->src->physical = end;
- disk->src->capacity = end;
+ disk->src->allocation = end;
}
- /* If the file we probed has a capacity set, then override
- * what we calculated from file/block extents */
+ /* Raw files: capacity is physical size. For all other files: if
+ * the metadata has a capacity, use that, otherwise fall back to
+ * physical size. */
if (!(format = virDomainDiskGetFormat(disk))) {
if (!cfg->allowDiskFormatProbing) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -11145,16 +11149,16 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
buf, len)) < 0)
goto endjob;
}
- if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
- format, NULL)))
+ if (format == VIR_STORAGE_FILE_RAW) {
+ disk->src->capacity = disk->src->physical;
+ } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf,
+ len, format, NULL))) {
goto endjob;
- if (meta->capacity)
- disk->src->capacity = meta->capacity;
+ disk->src->capacity = meta->capacity ? meta->capacity
+ : disk->src->physical;
We'll never get to this line because of the goto endjob (which gets
propagated in next patch as goto cleanup).
John
+ }
- /* Set default value .. */
- disk->src->allocation = disk->src->physical;
-
- /* ..but if guest is not using raw disk format and on a block device,
+ /* 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
&&
3:40 p.m.
New subject: [libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats
On 12/16/14 14:17, John Ferlan wrote:
Ran the series through my local Coverity checker.... found one issue....
On 12/16/2014 03:04 AM, Eric Blake wrote:
> The documentation for virDomainBlockInfo was confusing: it stated
> that 'physical' was the size of the container, then gave an example
> of it being the amount of storage used by a sparse file (that is,
> for a sparse raw image on a regular file, the wording implied
> capacity==physical, while allocation was smaller; but the example
> instead claimed physical==allocation). Since we use 'physical' for
> the last offset of a block device, we should do likewise for
> regular files.
>
> Furthermore, the example claimed that for a qcow2 regular file,
> allocation==physical. At the time the code was first written,
> this was true (qcow2 files were allocated sequentially, and were
> never sparse, so the last sector written happened to also match
> the disk space occupied); but modern qemu does much better and
> can punch holes for a qcow2 with allocation < physical.
>
> Basically, after this patch, the three fields are now reliably
> mapped as:
> 'capacity' - how much storage the guest can see (equal to
> physical for raw images, determined by image metadata otherwise)
> 'allocation' - how much storage the image occupies (similar to
> what 'du' would report)
> 'physical' - the last offset of the image (similar to what 'ls'
> would report)
>
> 'capacity' can be larger than 'physical' (such as for a qcow2
> image that does not vary much from a backing file) or smaller
> (such as for a qcow2 file with lots of internal snapshots).
> Likewise, 'allocation' can be (slightly) larger than 'physical'
> (such as counting the tail of cluster allocations required to
> round a file size up to filesystem granularity) or smaller
> (for a sparse file). A block-resize operation changes capacity
> (which, for raw images, also changes physical); many non-raw
> images automatically grow physical and allocation as necessary
> when starting with an allocation smaller than capacity; and even
> when capacity and physical stay unchanged, allocation can change
> when converting sectors from holes to data or back.
>
> Note that this does not change semantics for qcow2 images stored
> on block devices; there, we still rely on qemu to report the
> highest written extent for allocation. So using this API to
> track when to extend a block device because a qcow2 image is
> about to exceed a threshold will not see any changes.
>
> * include/libvirt/libvirt-domain.h (_virDomainBlockInfo): Tweak
> documentation to match saner definition.
> * src/qemu/qemu_driver.c (qemuDomainGetBlockInfo): For regular
> files, physical size is capacity, not allocation.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> include/libvirt/libvirt-domain.h | 23 ++++++++++++++--------
> src/qemu/qemu_driver.c | 42 ++++++++++++++++++++++------------------
> 2 files changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f87c44b..5e9c133 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11145,16 +11149,16 @@ qemuDomainGetBlockInfo(virDomainPtr
dom,
> buf, len)) < 0)
> goto endjob;
> }
> - if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf, len,
> - format, NULL)))
> + if (format == VIR_STORAGE_FILE_RAW) {
> + disk->src->capacity = disk->src->physical;
> + } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path,
buf,
> + len, format, NULL))) {
> goto endjob;
> - if (meta->capacity)
> - disk->src->capacity = meta->capacity;
> + disk->src->capacity = meta->capacity ? meta->capacity
> + : disk->src->physical;
Also if you are going to use a ternary, please leave all sections on one
line for clarity.
We'll never get to this line because of the goto endjob (which gets
propagated in next patch as goto cleanup).
John
Peter
12:53 a.m.
New subject: [libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats
On 12/16/2014 06:17 AM, John Ferlan wrote:
Ran the series through my local Coverity checker.... found one issue....
> - format,
NULL)))
> + if (format == VIR_STORAGE_FILE_RAW) {
> + disk->src->capacity = disk->src->physical;
> + } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path,
buf,
> + len, format, NULL))) {
> goto endjob;
> - if (meta->capacity)
> - disk->src->capacity = meta->capacity;
> + disk->src->capacity = meta->capacity ? meta->capacity
> + : disk->src->physical;
We'll never get to this line because of the goto endjob (which gets
propagated in next patch as goto cleanup).
Here's what I re-wrote this to:
if (format == VIR_STORAGE_FILE_RAW)
src->capacity = src->physical;
else if ((meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf,
len, format, NULL)))
src->capacity = meta->capacity ? meta->capacity : src->physical;
else
goto endjob;
I didn't see an explicit ACK on this patch, but as 7/12 is the same
material just refactored, I'll go ahead and push the corrected form of
this at the same time as that one.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
12:57 p.m.
New subject: [libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats
On 12/16/2014 06:53 PM, Eric Blake wrote:
On 12/16/2014 06:17 AM, John Ferlan wrote:
>
> Ran the series through my local Coverity checker.... found one issue....
>
>> - format, NULL)))
>> + if (format == VIR_STORAGE_FILE_RAW) {
>> + disk->src->capacity = disk->src->physical;
>> + } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path,
buf,
>> + len, format, NULL))) {
>> goto endjob;
>> - if (meta->capacity)
>> - disk->src->capacity = meta->capacity;
>> + disk->src->capacity = meta->capacity ? meta->capacity
>> + : disk->src->physical;
>
> We'll never get to this line because of the goto endjob (which gets
> propagated in next patch as goto cleanup).
Here's what I re-wrote this to:
if (format == VIR_STORAGE_FILE_RAW)
src->capacity = src->physical;
else if ((meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf,
len, format, NULL)))
src->capacity = meta->capacity ? meta->capacity : src->physical;
else
goto endjob;
I didn't see an explicit ACK on this patch, but as 7/12 is the same
material just refactored, I'll go ahead and push the corrected form of
this at the same time as that one.
Coverity complained this morning because there's a call to
virStorageFileGetMetadataFromBuf in the block just before this:
(18) Event alloc_fn: Storage is returned from allocation function
"virStorageFileGetMetadataFromBuf". [details]
(19) Event var_assign: Assigning: "meta" = storage returned from
"virStorageFileGetMetadataFromBuf(src->path, buf, len, format, NULL)".
(20) Event cond_false: Condition "!(meta =
virStorageFileGetMetadataFromBuf(src->path, buf, len, format, NULL))", taking
false branch
Also see events: [overwrite_var]
11119 if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf, len,
11120 format, NULL)))
(21) Event if_end: End of if statement
11121 goto cleanup;
(22) Event cond_false: Condition "format == VIR_STORAGE_FILE_RAW", taking false
branch
11122 if (format == VIR_STORAGE_FILE_RAW)
11123 src->capacity = src->physical;
(23) Event overwrite_var: Overwriting "meta" in "meta =
virStorageFileGetMetadataFromBuf(src->path, buf, len, format, NULL)" leaks the
storage that "meta" points to.
Also see events: [alloc_fn][var_assign]
11124 else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf,
11125 len, format, NULL)))
11126 src->capacity = meta->capacity ? meta->capacity :
src->physical;
11127 else
11128 goto cleanup;
11129
John
12:16 a.m.
New subject: [libvirt] [PATCH v2 06/12] qemu: fix bugs in blockstats
On 12/17/2014 04:57 AM, John Ferlan wrote:
On 12/16/2014 06:53 PM, Eric Blake wrote:
> On 12/16/2014 06:17 AM, John Ferlan wrote:
>>
>> Ran the series through my local Coverity checker.... found one issue....
>>
> I didn't see an explicit ACK on this patch, but as 7/12 is
the same
> material just refactored, I'll go ahead and push the corrected form of
> this at the same time as that one.
>
Coverity complained this morning because there's a call to
virStorageFileGetMetadataFromBuf in the block just before this:
(18) Event alloc_fn: Storage is returned from allocation function
"virStorageFileGetMetadataFromBuf". [details]
(19) Event var_assign: Assigning: "meta" = storage returned from
"virStorageFileGetMetadataFromBuf(src->path, buf, len, format, NULL)".
(20) Event cond_false: Condition "!(meta =
virStorageFileGetMetadataFromBuf(src->path, buf, len, format, NULL))", taking
false branch
Also see events: [overwrite_var]
Blah. In all my rebasing, I duplicated a line, and indeed caused a
memory leak. Trivial patch coming up soon.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 07/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 | 207 ++++++++++++++++++++++++++-----------------------
1 file changed, 110 insertions(+), 97 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5e9c133..13ec903 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10992,65 +10992,23 @@ qemuDomainMemoryPeek(virDomainPtr dom,
}
+/* Refresh the capacity and allocation limits of a given storage
+ * source. Assumes that the caller has already obtained a domain
+ * job. */
static int
-qemuDomainGetBlockInfo(virDomainPtr dom,
- const char *path,
- virDomainBlockInfoPtr info,
- unsigned int flags)
+qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr vm, virDomainDiskDefPtr disk,
+ virStorageSourcePtr src)
{
- 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];
- if (virStorageSourceIsEmpty(disk->src)) {
- virReportError(VIR_ERR_INVALID_ARG,
- _("disk '%s' does not currently have a source
assigned"),
- path);
- goto endjob;
- }
-
/* 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
@@ -11071,51 +11029,51 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* punching holes), and physical size of a non-raw file can
* change.
*/
- if (virStorageSourceIsLocalStorage(disk->src)) {
+ if (virStorageSourceIsLocalStorage(src)) {
/* Yes, this is a mild TOCTTOU race, but if someone is
* changing files in the background behind libvirt's back,
* they deserve bogus information. */
- if (stat(disk->src->path, &sb) < 0) {
+ if (stat(src->path, &sb) < 0) {
virReportSystemError(errno,
- _("cannot stat file '%s'"),
disk->src->path);
- goto endjob;
+ _("cannot stat file '%s'"),
src->path);
+ 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 ((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 (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;
}
- if ((len = virStorageFileReadHeader(disk->src, VIR_STORAGE_MAX_HEADER,
+ if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER,
&buf)) < 0)
- goto endjob;
+ goto cleanup;
}
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- disk->src->allocation = (unsigned long long)sb.st_blocks *
+ src->allocation = (unsigned long long)sb.st_blocks *
(unsigned long long)DEV_BSIZE;
#else
- disk->src->allocation = sb.st_size;
+ src->allocation = sb.st_size;
#endif
/* Allocation tracks when the file is sparse, physical is the
* last offset of the file. */
- disk->src->physical = sb.st_size;
+ src->physical = sb.st_size;
} else {
/* NB. Because we configure with AC_SYS_LARGEFILE, off_t
* should be 64 bits on all platforms. For block devices, we
@@ -11127,11 +11085,11 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
end = lseek(fd, 0, SEEK_END);
if (end == (off_t)-1) {
virReportSystemError(errno,
- _("failed to seek to end of %s"), path);
- goto endjob;
+ _("failed to seek to end of %s"),
src->path);
+ goto cleanup;
}
- disk->src->physical = end;
- disk->src->allocation = end;
+ src->physical = end;
+ src->allocation = end;
}
/* Raw files: capacity is physical size. For all other files: if
@@ -11141,29 +11099,96 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
if (!cfg->allowDiskFormatProbing) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("no disk format for %s and probing is disabled"),
- path);
- goto endjob;
+ src->path);
+ goto cleanup;
}
- if ((format = virStorageFileProbeFormatFromBuf(disk->src->path,
+ if ((format = virStorageFileProbeFormatFromBuf(src->path,
buf, len)) < 0)
- goto endjob;
+ goto cleanup;
}
if (format == VIR_STORAGE_FILE_RAW) {
- disk->src->capacity = disk->src->physical;
- } else if (!(meta = virStorageFileGetMetadataFromBuf(disk->src->path, buf,
+ src->capacity = src->physical;
+ } else if (!(meta = virStorageFileGetMetadataFromBuf(src->path, buf,
len, format, NULL))) {
- goto endjob;
- disk->src->capacity = meta->capacity ? meta->capacity
- : disk->src->physical;
+ goto cleanup;
+ src->capacity = meta->capacity ? meta->capacity : src->physical;
}
/* 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)) {
+ S_ISBLK(sb.st_mode))
+ src->allocation = 0;
+
+ ret = 0;
+ cleanup:
+ VIR_FREE(buf);
+ virStorageSourceFree(meta);
+ VIR_FORCE_CLOSE(fd);
+ 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 (virStorageSourceIsEmpty(disk->src)) {
+ virReportError(VIR_ERR_INVALID_ARG,
+ _("disk '%s' does not currently have a source
assigned"),
+ path);
+ goto endjob;
+ }
+
+ if ((ret = qemuStorageLimitsRefresh(driver, cfg, vm, disk, disk->src)) < 0)
+ goto endjob;
+
+ if (!disk->src->allocation) {
qemuDomainObjPrivatePtr priv = vm->privateData;
/* If the guest is not running, then success/failure return
@@ -11171,7 +11196,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
*/
if (!virDomainObjIsActive(vm)) {
activeFail = true;
- ret = 0;
goto endjob;
}
@@ -11180,27 +11204,16 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
disk->info.alias,
&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;
- }
+ 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);
-
/* 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
3:56 p.m.
New subject: [libvirt] [PATCH v2 07/12] qemu: refactor blockinfo data gathering
On 12/16/14 09:04, 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 | 207 ++++++++++++++++++++++++++-----------------------
1 file changed, 110 insertions(+), 97 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5e9c133..13ec903 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10992,65 +10992,23 @@ qemuDomainMemoryPeek(virDomainPtr dom,
}
+/* Refresh the capacity and allocation limits of a given storage
+ * source. Assumes that the caller has already obtained a domain
+ * job. */
static int
-qemuDomainGetBlockInfo(virDomainPtr dom,
- const char *path,
- virDomainBlockInfoPtr info,
- unsigned int flags)
+qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg,
+ virDomainObjPtr vm, virDomainDiskDefPtr disk,
+ virStorageSourcePtr src)
One argument per line please.
{
- virQEMUDriverPtr driver = dom->conn->privateData;
- virDomainObjPtr vm;
int ret = -1;
int fd = -1;
off_t end;
virStorageSourcePtr meta = NULL;
- virDomainDiskDefPtr disk = NULL;
[...]
@@ -11071,51 +11029,51 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
* punching holes), and physical size of a non-raw file can
* change.
*/
- if (virStorageSourceIsLocalStorage(disk->src)) {
+ if (virStorageSourceIsLocalStorage(src)) {
/* Yes, this is a mild TOCTTOU race, but if someone is
* changing files in the background behind libvirt's back,
* they deserve bogus information. */
- if (stat(disk->src->path, &sb) < 0) {
+ if (stat(src->path, &sb) < 0) {
Here you'll get a context conflict after fixing earlier patch.
virReportSystemError(errno,
- _("cannot stat file '%s'"),
disk->src->path);
- goto endjob;
+ _("cannot stat file '%s'"),
src->path);
+ goto cleanup;
}
[...]
ACK,
Peter
7:29 a.m.
New subject: [libvirt] [PATCH v2 07/12] qemu: refactor blockinfo data gathering
On 12/16/2014 07:56 AM, Peter Krempa wrote:
On 12/16/14 09:04, 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.
>
> +qemuStorageLimitsRefresh(virQEMUDriverPtr driver,
virQEMUDriverConfigPtr cfg,
> + virDomainObjPtr vm, virDomainDiskDefPtr disk,
> + virStorageSourcePtr src)
One argument per line please.
Fixed (which included dropping 'disk' as it was no longer used),
> + if (virStorageSourceIsLocalStorage(src)) {
> /* Yes, this is a mild TOCTTOU race, but if someone is
> * changing files in the background behind libvirt's back,
> * they deserve bogus information. */
> - if (stat(disk->src->path, &sb) < 0) {
> + if (stat(src->path, &sb) < 0) {
Here you'll get a context conflict after fixing earlier patch.
retested that my adjustments were sane,
> virReportSystemError(errno,
> - _("cannot stat file '%s'"),
disk->src->path);
> - goto endjob;
> + _("cannot stat file '%s'"),
src->path);
> + goto cleanup;
> }
>
[...]
ACK,
and pushed through here.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:18 p.m.
New subject: [libvirt] [PATCH v2 07/12] qemu: refactor blockinfo data gathering
On 12/16/14 09:04, 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 | 207 ++++++++++++++++++++++++++-----------------------
1 file changed, 110 insertions(+), 97 deletions(-)
ACK,
Peter
9:04 a.m.
New subject: [libvirt] [PATCH v2 08/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 | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 13ec903..ef73502 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18548,6 +18548,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 */
@@ -18577,8 +18578,20 @@ 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 (virStorageSourceIsEmpty(disk->src))
+ continue;
+ if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, disk->src) < 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;
}
@@ -18615,6 +18628,7 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
cleanup:
virHashFree(stats);
+ virObjectUnref(cfg);
return ret;
}
--
1.9.3
4:03 p.m.
New subject: [libvirt] [PATCH v2 08/12] getstats: report block sizes for offline domains
On 12/16/14 09:04, 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 | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
ACK,
Peter
8:10 a.m.
New subject: [libvirt] [PATCH v2 08/12] getstats: report block sizes for offline domains
On 12/16/2014 08:03 AM, Peter Krempa wrote:
On 12/16/14 09:04, 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.
This was actually simplified in earlier patches, and no longer needed
tweaking.
> (qemuDomainGetStatsBlock): Use it to report offline statistics.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
ACK,
Will push shortly, after updating the commit message to be accurate (I
originally wrote it before block.0.path was implemented, but rebasing
let that addition go in earlier).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 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 | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ef73502..1127312 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18542,13 +18542,14 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
int *maxparams,
unsigned int privflags)
{
- size_t i;
+ size_t i = 0;
int ret = -1;
int rc;
virHashTablePtr stats = NULL;
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 */
@@ -18565,9 +18566,13 @@ 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++) {
+ for (; i < dom->def->ndisks; i++) {
qemuBlockStats *entry;
virDomainDiskDefPtr disk = dom->def->disks[i];
@@ -18627,6 +18632,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
4:09 p.m.
New subject: [libvirt] [PATCH v2 09/12] getstats: prepare for dynamic block.count stat
On 12/16/14 09:04, 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 | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ef73502..1127312 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18542,13 +18542,14 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
int *maxparams,
unsigned int privflags)
{
- size_t i;
+ size_t i = 0;
int ret = -1;
int rc;
virHashTablePtr stats = NULL;
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 */
@@ -18565,9 +18566,13 @@ 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;
The index is remembered prior ..
+ QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
to adding the element into the array. As the macro may jump to cleanup
precisely at this moment, if adding of the element failed you'd ...
- for (i = 0; i < dom->def->ndisks; i++) {
+ for (; i < dom->def->ndisks; i++) {
qemuBlockStats *entry;
virDomainDiskDefPtr disk = dom->def->disks[i];
@@ -18627,6 +18632,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
ret = 0;
cleanup:
+ if (count_index >= 0)
+ record->params[count_index].value.ui = i;
Touch a non-existent field here.
if (ret == 0 && count_index >= 0) ...
virHashFree(stats);
virObjectUnref(cfg);
return ret;
ACK with the tweak above.
Peter
8:13 a.m.
New subject: [libvirt] [PATCH v2 09/12] getstats: prepare for dynamic block.count stat
On 12/16/2014 08:09 AM, Peter Krempa wrote:
On 12/16/14 09:04, 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.
>
> + count_index = record->nparams;
The index is remembered prior ..
> + QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
to adding the element into the array. As the macro may jump to cleanup
precisely at this moment, if adding of the element failed you'd ...
>
> - for (i = 0; i < dom->def->ndisks; i++) {
> + for (; i < dom->def->ndisks; i++) {
> qemuBlockStats *entry;
> virDomainDiskDefPtr disk = dom->def->disks[i];
>
> @@ -18627,6 +18632,8 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> ret = 0;
>
> cleanup:
> + if (count_index >= 0)
> + record->params[count_index].value.ui = i;
Touch a non-existent field here.
Ooh, good catch.
if (ret == 0 && count_index >= 0) ...
> virHashFree(stats);
> virObjectUnref(cfg);
> return ret;
>
ACK with the tweak above.
Fixed, and will push shortly.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 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 baef32d..0b1a2d6 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1705,6 +1705,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 0eba2c1..8c5e426 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
4:11 p.m.
New subject: [libvirt] [PATCH v2 10/12] getstats: add new flag for block backing chain
On 12/16/14 09:04, Eric Blake wrote:
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 baef32d..0b1a2d6 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1705,6 +1705,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.
You add the field to this help, but ...
* "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 0eba2c1..8c5e426 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
... not to the manpage.
ACK with ^^.
Peter
9:32 a.m.
New subject: [libvirt] [PATCH v2 10/12] getstats: add new flag for block backing chain
On 12/16/2014 08:11 AM, Peter Krempa wrote:
On 12/16/14 09:04, Eric Blake wrote:
> 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!
>
> * block device.
> + * "block.<num>.backingIndex" - unsigned int giving the
<backingStore> index,
> + * when backing images are listed.
You add the field to this help, but ...
> -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
>
... not to the manpage.
ACK with ^^.
Fixed and pushed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:04 a.m.
New subject: [libvirt] [PATCH v2 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 | 133 ++++++++++++++++++++++++++++++-------------------
1 file changed, 81 insertions(+), 52 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1127312..8dc5dd5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18535,6 +18535,83 @@ 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 (virStorageSourceIsEmpty(src)) {
+ ret = 0;
+ goto cleanup;
+ }
+ if (qemuStorageLimitsRefresh(driver, cfg, dom,
+ disk, src) < 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,
@@ -18573,60 +18650,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
QEMU_ADD_COUNT_PARAM(record, maxparams, "block", 0);
for (; 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 (virStorageSourceIsEmpty(disk->src))
- continue;
- if (qemuStorageLimitsRefresh(driver, cfg, dom,
- disk, disk->src) < 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
9:04 a.m.
New subject: [libvirt] [PATCH v2 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.
* src/qemu/qemu_domain.c (qemuDomainStorageAlias): Tolerate NULL
alias input for offline domain.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_domain.c | 3 +++
src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 02887cd..73eabee 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2698,6 +2698,9 @@ qemuDomainStorageAlias(const char *device, int depth)
{
char *alias;
+ if (!device)
+ return NULL;
+
if (STRPREFIX(device, QEMU_DRIVE_HOST_PREFIX))
device += strlen(QEMU_DRIVE_HOST_PREFIX);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8dc5dd5..f70182f 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -18260,8 +18260,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;
@@ -18508,6 +18510,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 { \
@@ -18545,20 +18560,24 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
virDomainDiskDefPtr disk,
virStorageSourcePtr src,
size_t block_idx,
+ unsigned int backing_idx,
bool abbreviated,
virHashTablePtr stats)
{
qemuBlockStats *entry;
int ret = -1;
+ char *alias = qemuDomainStorageAlias(disk->info.alias, backing_idx);
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 (backing_idx)
+ QEMU_ADD_BLOCK_PARAM_UI(record, maxparams, block_idx, "backingIndex",
+ backing_idx);
- if (abbreviated || !disk->info.alias ||
- !(entry = virHashLookup(stats, disk->info.alias))) {
+ if (abbreviated || !alias || !(entry = virHashLookup(stats, alias))) {
if (virStorageSourceIsEmpty(src)) {
ret = 0;
goto cleanup;
@@ -18608,6 +18627,7 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
ret = 0;
cleanup:
+ VIR_FREE(alias);
return ret;
}
@@ -18627,14 +18647,16 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
bool abbreviated = false;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
int count_index = -1;
+ size_t visited = 0;
+ bool backing = !!(privflags & QEMU_DOMAIN_STATS_BACKING);
if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
abbreviated = true; /* it's ok, just go ahead silently */
} else {
qemuDomainObjEnterMonitor(driver, dom);
- rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats, false);
+ rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats, backing);
ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
- false));
+ backing));
qemuDomainObjExitMonitor(driver, dom);
if (rc < 0) {
@@ -18651,18 +18673,25 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
for (; 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 || 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;
@@ -18803,11 +18832,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)
@@ -18857,6 +18888,8 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
domflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
/* else: without a job it's still possible to gather some data */
+ if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
+ domflags |= QEMU_DOMAIN_STATS_BACKING;
if (qemuDomainGetStats(conn, dom, stats, &tmp, domflags) < 0)
goto endjob;
--
1.9.3
4:45 p.m.
New subject: [libvirt] [PATCH v2 12/12] getstats: start crawling backing chain for qemu
On 12/16/14 09:04, 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.
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.
* src/qemu/qemu_domain.c (qemuDomainStorageAlias): Tolerate NULL
alias input for offline domain.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_domain.c | 3 +++
src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++++++++++++++++++-----------
2 files changed, 48 insertions(+), 12 deletions(-)
@@ -18651,18 +18673,25 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr
driver,
for (; 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 || backing)) {
I'd make the special case of the first element a bit more obvious by
using "backing_idx == 0" rather than the negation sign used usually with
pointers.
+ 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;
As an additional thought. The stats are now extermely long for VMs with
a long backing chain and few of the stats tend to be 0 always. We could
start skipping them perhaps (at least for the non-top, devices)
ACK.
Peter
10:09 a.m.
New subject: [libvirt] [PATCH v2 12/12] getstats: start crawling backing chain for qemu
On 12/16/2014 08:45 AM, Peter Krempa wrote:
On 12/16/14 09:04, 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
Stale comments; I'll update them.
> 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.
>
> $ 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
actually even longer - I get block.1.rd.reqs and so forth as well.
> - if (qemuDomainGetStatsOneBlock(driver, cfg, dom, record,
maxparams,
> - disk, disk->src, i, abbreviated,
> - stats) < 0)
> - goto cleanup;
> + while (src && (!backing_idx || backing)) {
I'd make the special case of the first element a bit more obvious by
using "backing_idx == 0" rather than the negation sign used usually with
pointers.
Will fix.
> + 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;
As an additional thought. The stats are now extermely long for VMs with
a long backing chain and few of the stats tend to be 0 always. We could
start skipping them perhaps (at least for the non-top, devices)
That can be a followup patch (I agree with it, but it's too late for me
to fix tonight...)
ACK.
I've pushed 11 and 12 with fixes. Thanks for the reviews.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:07 a.m.
New subject: [libvirt] [PATCH v2 00/12] more progress towards backing chain allocation stats
v1 was here:
https://www.redhat.com/archives/libvir-list/2014-December/msg00370.html
ACKed patches from that series have been pushed. In this series,
3, 4, and 6 are new, and others try to address some of the feedback
and deal with rebased design decisions that resulted. I still don't
have things reporting quite as nicely as I would like (it turns out
that we HAVE to stat() a file for an online domain to learn its
allocation, and that for block devices, we HAVE to open()/lseek() to
learn its physical size; meanwhile, I still want to fix
virDomainGetBlockInfo to avoid read()ing a file while qemu is
active by reusing the code that getStats uses). But I'm posting
another round now, to hopefully get early patches ACKed and into
the tree, and to demonstrate that I now have recursive stat
collection for an active domain relying solely on qemu rather than
read()ing the backing files directly.
Oh, and patches are also available at
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):
qemu: refactor blockinfo job handling
qemu: let blockinfo reuse virStorageSource
getstats: prepare monitor collection for recursion
getstats: perform recursion in monitor collection
getstats: rearrange blockinfo gathering
qemu: fix bugs in blockstats
qemu: refactor blockinfo data gathering
getstats: report block sizes for offline domains
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 | 24 +-
src/libvirt-domain.c | 7 +-
src/qemu/qemu_domain.c | 19 ++
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_driver.c | 477
++++++++++++++++++++++++---------------
src/qemu/qemu_migration.c | 3 +-
src/qemu/qemu_monitor.c | 24 +-
src/qemu/qemu_monitor.h | 6 +-
src/qemu/qemu_monitor_json.c | 291 ++++++++++++++----------
src/qemu/qemu_monitor_json.h | 6 +-
src/util/virstoragefile.c | 3 +-
src/util/virstoragefile.h | 3 +-
tools/virsh-domain-monitor.c | 7 +
tools/virsh.pod | 8 +-
14 files changed, 558 insertions(+), 321 deletions(-)
--
1.9.3
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3564
days inactive
3565
days old
37 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa