On 09/08/14 15:05, Francesco Romani wrote:
> This patch implements the VIR_DOMAIN_STATS_BLOCK
> group of statistics.
>
> To do so, an helper function to get the block stats
> of all the disks of a domain is added.
>
> Signed-off-by: Francesco Romani <fromani@xxxxxxxxxx>
> ---
> include/libvirt/libvirt.h.in | 1 +
> src/libvirt.c | 17 ++++++
> src/qemu/qemu_driver.c | 93 +++++++++++++++++++++++++++++
> src/qemu/qemu_monitor.c | 22 +++++++
> src/qemu/qemu_monitor.h | 20 +++++++
> src/qemu/qemu_monitor_json.c | 135 ++++++++++++++++++++++++++++++-------------
> src/qemu/qemu_monitor_json.h | 4 ++
> src/qemu/qemu_monitor_text.c | 13 +++++
> src/qemu/qemu_monitor_text.h | 4 ++
> 9 files changed, 269 insertions(+), 40 deletions(-)
>
> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
> index 93aa1fb..c8e0089 100644
> --- a/include/libvirt/libvirt.h.in
> +++ b/include/libvirt/libvirt.h.in
> @@ -2515,6 +2515,7 @@ typedef enum {
> VIR_DOMAIN_STATS_BALLOON = (1 << 2), /* return domain balloon info */
> VIR_DOMAIN_STATS_VCPU = (1 << 3), /* return domain virtual CPU info */
> VIR_DOMAIN_STATS_INTERFACE = (1 << 4), /* return domain interfaces info */
> + VIR_DOMAIN_STATS_BLOCK = (1 << 5), /* return domain block info */
> } virDomainStatsTypes;
>
> typedef enum {
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 8aa6cb1..e041fa2 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -21596,6 +21596,23 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> * "net.<num>.tx.errs" - transmission errors as long long.
> * "net.<num>.tx.drop" - transmit packets dropped as long long.
> *
> + * VIR_DOMAIN_STATS_BLOCK: Return block devices statistics.
> + * 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 name of the block device.
> + * "block.<num>.rd.reqs" - number of read requests as long long.
> + * "block.<num>.rd.bytes" - number of read bytes as long long.
> + * "block.<num>.rd.times" - total time (ns) spent on reads as long long.
> + * "block.<num>.wr.reqs" - number of write requests as long long.
> + * "block.<num>.wr.bytes" - number of written bytes as long long.
> + * "block.<num>.wr.times" - total time (ns) spent on writes as long long.
> + * "block.<num>.fl.reqs" - total flush requests as long long.
> + * "block.<num>.fl.times" - total time (ns) spent on cache flushing
> + * as long long.
> + * "block.<num>.errors" - Xen only: the 'oo_req' value as long long.
Same as for the interface stats. How about using unsigned long long and
dropping stats that are not available?
> + *
> * Using 0 for @stats returns all stats groups supported by the given
> * hypervisor.
> *
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 989eb3e..93afb7e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9678,6 +9678,31 @@ qemuDomainBlockStats(virDomainPtr dom,
> return ret;
> }
>
> +
> +/*
> + * Returns at most the first `nstats' stats, then stops.
> + * Returns the number of stats filled.
> + */
> +static int
> +qemuDomainHelperGetBlockStats(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuBlockStatsPtr stats,
> + int nstats)
> +{
> + int ret;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + ret = qemuMonitorGetAllBlockStatsInfo(priv->mon, NULL,
> + stats, nstats);
> +
> + qemuDomainObjExitMonitor(driver, vm);
> +
> + return ret;
> +}
> +
> +
> static int
> qemuDomainBlockStatsFlags(virDomainPtr dom,
> const char *path,
> @@ -17620,6 +17645,73 @@ qemuDomainGetStatsInterface(virConnectPtr conn ATTRIBUTE_UNUSED,
>
> #undef QEMU_ADD_NET_PARAM
>
> +#define QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, num, name, value) \
> +do { \
> + char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> + "block.%lu.%s", num, name); \
> + if (virTypedParamsAddLLong(&(record)->params, \
> + &(record)->nparams, \
> + maxparams, \
> + param_name, \
> + value) < 0) \
> + goto cleanup; \
> +} while (0)
> +
> +static int
> +qemuDomainGetStatsBlock(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virDomainObjPtr dom,
> + virDomainStatsRecordPtr record,
> + int *maxparams,
> + unsigned int privflags ATTRIBUTE_UNUSED)
> +{
> + size_t i;
> + int ret = -1;
> + int nstats = 0;
> + qemuBlockStatsPtr stats = NULL;
> + virQEMUDriverPtr driver = conn->privateData;
> +
> + if (VIR_ALLOC_N(stats, dom->def->ndisks) < 0)
> + return -1;
> +
> + nstats = qemuDomainHelperGetBlockStats(driver, dom, stats,
> + dom->def->ndisks);
> + if (nstats < 0)
> + goto cleanup;
> +
> + QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
> +
> + for (i = 0; i < nstats; i++) {
> + QEMU_ADD_NAME_PARAM(record, maxparams,
> + "block", i, dom->def->disks[i]->dst);
> +
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "rd.reqs", stats[i].rd_req);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "rd.bytes", stats[i].rd_bytes);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "rd.times", stats[i].rd_total_times);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "wr.reqs", stats[i].wr_req);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "wr.bytes", stats[i].wr_bytes);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "wr.times", stats[i].wr_total_times);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "fl.reqs", stats[i].flush_req);
> + QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, i,
> + "fl.times", stats[i].flush_total_times);
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(stats);
> + return ret;
> +}
> +
> +#undef QEMU_ADD_BLOCK_PARAM
> +
> #undef QEMU_ADD_NAME_PARAM
>
> #undef QEMU_ADD_COUNT_PARAM
> @@ -17643,6 +17735,7 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
> { qemuDomainGetStatsBalloon, VIR_DOMAIN_STATS_BALLOON, true },
> { qemuDomainGetStatsVcpu, VIR_DOMAIN_STATS_VCPU, false },
> { qemuDomainGetStatsInterface, VIR_DOMAIN_STATS_INTERFACE, false },
> + { qemuDomainGetStatsBlock, VIR_DOMAIN_STATS_BLOCK, true },
> { NULL, 0, false }
> };
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 702404a..d8ce875 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -29,6 +29,8 @@
> #include <unistd.h>
> #include <fcntl.h>
>
> +#include "internal.h"
> +
> #include "qemu_monitor.h"
> #include "qemu_monitor_text.h"
> #include "qemu_monitor_json.h"
> @@ -1760,6 +1762,26 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
> return ret;
> }
>
> +int
> +qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
> + const char *dev_name,
> + qemuBlockStatsPtr stats,
> + int nstats)
> +{
> + int ret;
> + VIR_DEBUG("mon=%p dev=%s", mon, dev_name);
> +
> + if (mon->json) {
> + ret = qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name,
> + stats, nstats);
> + } else {
> + ret = qemuMonitorTextGetAllBlockStatsInfo(mon, dev_name,
> + stats, nstats);
> + }
> +
> + return ret;
> +}
> +
> /* Return 0 and update @nparams with the number of block stats
> * QEMU supports if success. Return -1 if failure.
> */
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index ced198e..8e3fb44 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -346,6 +346,26 @@ int qemuMonitorGetBlockStatsInfo(qemuMonitorPtr mon,
> long long *flush_req,
> long long *flush_total_times,
> long long *errs);
> +
> +typedef struct _qemuBlockStats qemuBlockStats;
> +typedef qemuBlockStats *qemuBlockStatsPtr;
> +struct _qemuBlockStats {
> + long long rd_req;
> + long long rd_bytes;
> + long long wr_req;
> + long long wr_bytes;
> + long long rd_total_times;
> + long long wr_total_times;
> + long long flush_req;
> + long long flush_total_times;
> +};
> +
> +int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
> + const char *dev_name,
> + qemuBlockStatsPtr stats,
> + int nstats)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3);
> +
> int qemuMonitorGetBlockStatsParamsNumber(qemuMonitorPtr mon,
> int *nparams);
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 4373ba2..aa95e71 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1734,13 +1734,9 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
> long long *flush_total_times,
> long long *errs)
> {
> - int ret;
> - size_t i;
> - bool found = false;
> - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats",
> - NULL);
> - virJSONValuePtr reply = NULL;
> - virJSONValuePtr devices;
> + qemuBlockStats stats;
> + int nstats = 1;
> + int ret = -1;
>
> *rd_req = *rd_bytes = -1;
> *wr_req = *wr_bytes = *errs = -1;
> @@ -1754,9 +1750,51 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
> if (flush_total_times)
> *flush_total_times = -1;
>
> + if (qemuMonitorJSONGetAllBlockStatsInfo(mon, dev_name,
> + &stats, nstats) != nstats)
Writing '1' instead of 'nstats' would make it a bit more readable as you
don't expect to get more or less stats.
> + goto cleanup;
> +
> + *rd_req = stats.rd_req;
> + *rd_bytes = stats.rd_bytes;
> + *wr_req = stats.wr_req;
> + *wr_bytes = stats.wr_bytes;
> + *errs = -1; /* QEMU does not have this */
> +
> + if (rd_total_times)
> + *rd_total_times = stats.rd_total_times;
> + if (wr_total_times)
> + *wr_total_times = stats.wr_total_times;
> + if (flush_req)
> + *flush_req = stats.flush_req;
> + if (flush_total_times)
> + *flush_total_times = stats.flush_total_times;
> +
> + ret = 0;
> +
> + cleanup:
> + return ret;
> +}
> +
> +
> +int qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
> + const char *dev_name,
> + qemuBlockStatsPtr bstats,
> + int nstats)
> +{
> + int ret;
> + size_t i;
> + bool found = false;
> + virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-blockstats",
> + NULL);
> + virJSONValuePtr reply = NULL;
> + virJSONValuePtr devices;
> +
> if (!cmd)
> return -1;
>
> + if (!bstats || nstats <= 0)
> + return -1;
These are okay, they don't report an error, but also are for develoeper
errors only.
> +
> ret = qemuMonitorJSONCommand(mon, cmd, &reply);
>
> if (ret == 0)
> @@ -1772,108 +1810,125 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
> goto cleanup;
> }
>
> + ret = 0;
Here you set ret to 0.
> for (i = 0; i < virJSONValueArraySize(devices); i++) {
After a few iterations it may be > 0. (Or 0 on the first)
> virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
> virJSONValuePtr stats;
> - const char *thisdev;
> if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("blockstats device entry was not in expected format"));
> + _("blockstats device entry was not "
> + "in expected format"));
> goto cleanup;
If you hit this error, you return ret, which is now in success mode so
no one will see it.
> }
>
> - if ((thisdev = virJSONValueObjectGetString(dev, "device")) == NULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("blockstats device entry was not in expected format"));
> - goto cleanup;
> + if (ret > nstats) {
> + break;
> }
>
> - /* New QEMU has separate names for host & guest side of the disk
> - * and libvirt gives the host side a 'drive-' prefix. The passed
> - * in dev_name is the guest side though
> + /* If dev_name is specified, we are looking for a specific device,
> + * so we must be stricter.
> */
> - if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX))
> - thisdev += strlen(QEMU_DRIVE_HOST_PREFIX);
> + if (dev_name) {
> + const char *thisdev = virJSONValueObjectGetString(dev, "device");
> + if (!thisdev) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("blockstats device entry was not "
> + "in expected format"));
> + goto cleanup;
> + }
>
> - if (STRNEQ(thisdev, dev_name))
> - continue;
> + /* New QEMU has separate names for host & guest side of the disk
> + * and libvirt gives the host side a 'drive-' prefix. The passed
> + * in dev_name is the guest side though
> + */
> + if (STRPREFIX(thisdev, QEMU_DRIVE_HOST_PREFIX))
> + thisdev += strlen(QEMU_DRIVE_HOST_PREFIX);
> +
> + if (STRNEQ(thisdev, dev_name))
> + continue;
> +
> + found = true;
> + }
>
> - found = true;
> 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"));
> + _("blockstats stats entry was not "
> + "in expected format"));
> goto cleanup;
> }
>
> - if (virJSONValueObjectGetNumberLong(stats, "rd_bytes", rd_bytes) < 0) {
> + 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", rd_req) < 0) {
> + if (virJSONValueObjectGetNumberLong(stats, "rd_operations",
> + &bstats->rd_req) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot read %s statistic"),
> "rd_operations");
> goto cleanup;
> }
> - if (rd_total_times &&
> - virJSONValueObjectHasKey(stats, "rd_total_time_ns") &&
> + if (virJSONValueObjectHasKey(stats, "rd_total_time_ns") &&
> (virJSONValueObjectGetNumberLong(stats, "rd_total_time_ns",
> - rd_total_times) < 0)) {
> + &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", wr_bytes) < 0) {
> + 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", wr_req) < 0) {
> + if (virJSONValueObjectGetNumberLong(stats, "wr_operations",
> + &bstats->wr_req) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot read %s statistic"),
> "wr_operations");
> goto cleanup;
> }
> - if (wr_total_times &&
> - virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
> + if (virJSONValueObjectHasKey(stats, "wr_total_time_ns") &&
> (virJSONValueObjectGetNumberLong(stats, "wr_total_time_ns",
> - wr_total_times) < 0)) {
> + &bstats->wr_total_times) < 0)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot read %s statistic"),
> "wr_total_time_ns");
> goto cleanup;
> }
> - if (flush_req &&
> - virJSONValueObjectHasKey(stats, "flush_operations") &&
> + if (virJSONValueObjectHasKey(stats, "flush_operations") &&
> (virJSONValueObjectGetNumberLong(stats, "flush_operations",
> - flush_req) < 0)) {
> + &bstats->flush_req) < 0)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot read %s statistic"),
> "flush_operations");
> goto cleanup;
> }
> - if (flush_total_times &&
> - virJSONValueObjectHasKey(stats, "flush_total_time_ns") &&
> + if (virJSONValueObjectHasKey(stats, "flush_total_time_ns") &&
> (virJSONValueObjectGetNumberLong(stats, "flush_total_time_ns",
> - flush_total_times) < 0)) {
> + &bstats->flush_total_times) < 0)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot read %s statistic"),
> "flush_total_time_ns");
> goto cleanup;
> }
> +
> + ret++;
> + bstats++;
> }
>
> - if (!found) {
> + if (dev_name && !found) {
'found' here will be true if and only if ret == 1, so you don't really
need a separate variable
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot find statistics for device '%s'"), dev_name);
> + ret = 0;
I'd return -1 here as you've reported an error.
> goto cleanup;
> }
> - ret = 0;
>
> cleanup:
> virJSONValueFree(cmd);
> diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c
> index 2bc8261..5cdbef3 100644
> --- a/src/qemu/qemu_monitor_text.c
> +++ b/src/qemu/qemu_monitor_text.c
> @@ -1048,6 +1048,19 @@ int qemuMonitorTextGetBlockExtent(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> return -1;
> }
>
> +
> +int
> +qemuMonitorTextGetAllBlockStatsInfo(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> + const char *dev_name ATTRIBUTE_UNUSED,
> + qemuBlockStatsPtr blockstats ATTRIBUTE_UNUSED,
> + int nstats ATTRIBUTE_UNUSED)
> +{
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("unable to query all block stats with this QEMU"));
You can save adding this function and fold it right into the monitor
wrapper function as we do now with commadns that don't have HMP
counterparts.
> + return -1;
> +}
> +
> +
> /* Return 0 on success, -1 on failure, or -2 if not supported. Size
> * is in bytes. */
> int qemuMonitorTextBlockResize(qemuMonitorPtr mon,
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list