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

[Peter Krempa]

On 09/12/14 13:48, 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 at redhat.com>
> ---
>  include/libvirt/libvirt.h.in |   1 +
>  src/libvirt.c                |  20 +++++++
>  src/qemu/qemu_driver.c       |  96 ++++++++++++++++++++++++++++++
>  src/qemu/qemu_monitor.c      |  26 +++++++++
>  src/qemu/qemu_monitor.h      |  20 +++++++
>  src/qemu/qemu_monitor_json.c | 136 +++++++++++++++++++++++++++++--------------
>  src/qemu/qemu_monitor_json.h |   4 ++
>  7 files changed, 260 insertions(+), 43 deletions(-)
> 


> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7e5d707..4644f4a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -9687,6 +9687,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);


Humm, is it worth doing this helper? This pretty much can be inlined as
it has only one caller.

> +
> +    qemuDomainObjExitMonitor(driver, vm);
> +
> +    return ret;
> +}
> +
> +
>  static int
>  qemuDomainBlockStatsFlags(virDomainPtr dom,
>                            const char *path,
> @@ -17541,6 +17566,76 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>  
>  #undef QEMU_ADD_NET_PARAM
>  
> +/* expects a LL, but typed parameter must be ULL */
> +#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.%zu.%s", num, name); \
> +    if (value >= 0 && virTypedParamsAddULLong(&(record)->params, \
> +                                              &(record)->nparams, \
> +                                              maxparams, \
> +                                              param_name, \
> +                                              value) < 0) \
> +        goto cleanup; \
> +} while (0)
> +
> +static int
> +qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> +                        virDomainObjPtr dom,
> +                        virDomainStatsRecordPtr record,
> +                        int *maxparams,
> +                        unsigned int privflags)
> +{
> +    size_t i;
> +    int ret = -1;
> +    int nstats = 0;
> +    qemuBlockStatsPtr stats = NULL;
> +
> +    if (!HAVE_MONITOR(privflags) || !virDomainObjIsActive(dom))
> +        return 0; /* it's ok, just go ahead silently */
> +
> +    if (VIR_ALLOC_N(stats, dom->def->ndisks) < 0)
> +        return -1;
> +
> +    nstats = qemuDomainHelperGetBlockStats(driver, dom, stats,
> +                                           dom->def->ndisks);
> +    if (nstats < 0)

Are we erroring out on block stats failure? Other statistics gatherers
just skip it if it's not available.

> +        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_LL
> +
>  #undef QEMU_ADD_NAME_PARAM
>  
>  #undef QEMU_ADD_COUNT_PARAM

Otherwise looks good.

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140912/6d7f29e3/attachment-0001.sig>