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

Re: [libvirt] [PATCH 2/2] getstats: add block.n.source stat



On 11/25/14 06:21, Eric Blake wrote:
> I noticed that for an offline domain, 'virsh domstats --block $dom'
> was producing just the domain name, with no stats.  But the older
> 'virsh domblkinfo' works just fine on offline domains.  Furthermore,
> I'm about to make block stats optionally more complex to cover
> backing chains, where block.count will no longer equal the number
> of <disks> for a domain.  For these reasons, it is nicer if the
> statistics output always includes minimal information on every
> resource being described, with enough to correlate back to host
> resources, and even when some statistics are available only on a
> running domain.
> 
> With this patch, I now see the following for an offline domain
> with one disk and an empty cdrom drive:
> $ virsh domstats --block foo
> Domain: 'foo'
>   block.count=2
>   block.0.name=hda
>   block.0.source=/dev/sda4
>   block.1.name=hdc
> 
> * src/libvirt-domain.c (virConnectGetAllDomainStats)
> (virDomainListGetStats): Document new field; clarify that not all
> fields must be present for a group to be supported.
> * tools/virsh.pod (domstats): Document new field.
> * src/qemu/qemu_driver.c (qemuDomainGetStatsBlock): Return the new
> stat always, even for offline domains.
> (QEMU_ADD_NAME_PARAM): Add parameter.
> (qemuDomainGetStatsInterface): Update caller.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
>  src/libvirt-domain.c   | 16 +++++++++++++---
>  src/qemu/qemu_driver.c | 36 ++++++++++++++++++++----------------
>  tools/virsh.pod        |  1 +
>  3 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 2b0defc..8cd21ae 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -10907,6 +10907,8 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   * "block.<num>.name" - name of the block device <num> as string.
>   *                      matches the target name (vda/sda/hda) of the
>   *                      block device.
> + * "block.<num>.source" - string describing the source of block device <num>,
> + *                        such as the host path of a file or device.

Fair enough as long as we document that we only return it for
non-network sources. We had quite a few problems with network sources so
I'd rather not expose it for those due to possible ambiguity.

>   * "block.<num>.rd.reqs" - number of read requests as unsigned long long.
>   * "block.<num>.rd.bytes" - number of read bytes as unsigned long long.
>   * "block.<num>.rd.times" - total time (ns) spent on reads as
> @@ -10937,7 +10939,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   *
>   * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
>   * the function return error in case some of the stat types in @stats were
> - * not recognized by the daemon.
> + * not recognized by the daemon.  However, even with this flag, a hypervisor
> + * may omit individual fields within a group if the information is not
> + * available; as an extreme example, a supported group may produce zero
> + * fields for offline domains if the statistics are meaningful only for a
> + * running domain.
>   *
>   * Similarly to virConnectListAllDomains, @flags can contain various flags to
>   * filter the list of domains to provide stats for.
> @@ -11017,9 +11023,13 @@ virConnectGetAllDomainStats(virConnectPtr conn,
>   *
>   * Specifying VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS as @flags makes
>   * the function return error in case some of the stat types in @stats were
> - * not recognized by the daemon.
> + * not recognized by the daemon.  However, even with this flag, a hypervisor
> + * may omit individual fields within a group if the information is not
> + * available; as an extreme example, a supported group may produce zero
> + * fields for offline domains if the statistics are meaningful only for a
> + * running domain.
>   *

The code changes are not entirely related to this doc change.

> - * Note that any of the domain list filtering flags in @flags will be rejected
> + * Note that any of the domain list filtering flags in @flags may be rejected
>   * by this function.
>   *
>   * Returns the count of returned statistics structures on success, -1 on error.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 07da3e3..9295a05 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18408,11 +18408,11 @@ do { \
>          return -1; \
>  } while (0)
> 
> -#define QEMU_ADD_NAME_PARAM(record, maxparams, type, num, name) \
> +#define QEMU_ADD_NAME_PARAM(record, maxparams, type, subtype, num, name) \
>  do { \
>      char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; \
>      snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, \
> -             "%s.%zu.name", type, num); \
> +             "%s.%zu.%s", type, num, subtype); \
>      if (virTypedParamsAddString(&(record)->params, \
>                                  &(record)->nparams, \
>                                  maxparams, \
> @@ -18457,7 +18457,7 @@ qemuDomainGetStatsInterface(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
>          memset(&tmp, 0, sizeof(tmp));
> 
>          QEMU_ADD_NAME_PARAM(record, maxparams,
> -                            "net", i, dom->def->nets[i]->ifname);
> +                            "net", "name", i, dom->def->nets[i]->ifname);
> 
>          if (virNetInterfaceStats(dom->def->nets[i]->ifname, &tmp) < 0) {
>              virResetLastError();
> @@ -18526,19 +18526,20 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>      int rc;
>      virHashTablePtr stats = NULL;
>      qemuDomainObjPrivatePtr priv = dom->privateData;
> +    bool abbreviated = false;
> 
> -    if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom))
> -        return 0; /* it's ok, just go ahead silently */
> +    if (!HAVE_JOB(privflags) || !virDomainObjIsActive(dom)) {
> +        abbreviated = true; /* it's ok, just go ahead silently */
> +    } else {
> +        qemuDomainObjEnterMonitor(driver, dom);
> +        rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
> +        ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
> +        qemuDomainObjExitMonitor(driver, dom);
> 
> -    qemuDomainObjEnterMonitor(driver, dom);
> -    rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats);
> -    ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats));
> -    qemuDomainObjExitMonitor(driver, dom);
> -
> -    if (rc < 0) {
> -        virResetLastError();
> -        ret = 0; /* still ok, again go ahead silently */
> -        goto cleanup;
> +        if (rc < 0) {
> +            virResetLastError();
> +            abbreviated = true; /* still ok, again go ahead silently */
> +        }
>      }
> 
>      QEMU_ADD_COUNT_PARAM(record, maxparams, "block", dom->def->ndisks);
> @@ -18547,9 +18548,12 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>          qemuBlockStats *entry;
>          virDomainDiskDefPtr disk = dom->def->disks[i];
> 
> -        QEMU_ADD_NAME_PARAM(record, maxparams, "block", i, disk->dst);
> +        QEMU_ADD_NAME_PARAM(record, maxparams, "block", "name", i, disk->dst);
> +        if (disk->src && disk->src->path)

&& virStorageSourceIsLocalStorage(disk->src)

> +            QEMU_ADD_NAME_PARAM(record, maxparams, "block", "source",
> +                                i, disk->src->path);
> 
> -        if (!disk->info.alias ||
> +        if (abbreviated || !disk->info.alias ||
>              !(entry = virHashLookup(stats, disk->info.alias)))
>              continue;
> 
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 7cde3fd..e038c7e 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -883,6 +883,7 @@ I<--interface> returns:
>  I<--block> returns:
>  "block.count" - number of block devices on this domain,
>  "block.<num>.name" - name of the target of the block device <num>,
> +"block.<num>.source" - name of the source of block device <num>,
>  "block.<num>.rd.reqs" - number of read requests,
>  "block.<num>.rd.bytes" - number of read bytes,
>  "block.<num>.rd.times" - total time (ns) spent on reads,
> 

Peter

Attachment: signature.asc
Description: OpenPGP digital signature


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