[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv3 6/8] qemu: bulk stats: implement block group



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 redhat com>
> ---
>  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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]