[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. 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
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@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
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@redhat.com> --- src/qemu/qemu_driver.c | 65 ++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)
ACK, Peter
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@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
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@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
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
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@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
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@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
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@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
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@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
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@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
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@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
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
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
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@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
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@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 &&
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@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
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
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
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
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@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
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@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
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
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@redhat.com> --- src/qemu/qemu_driver.c | 207 ++++++++++++++++++++++++++----------------------- 1 file changed, 110 insertions(+), 97 deletions(-)
ACK, Peter
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@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
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@redhat.com> --- src/qemu/qemu_driver.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
ACK, Peter
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@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
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@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
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@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
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
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@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
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@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
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
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@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
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@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
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@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
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
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa