[libvirt] [PATCH v5] libxl: implement virDomainBlockStats

[Jim Fehlig]

On 07/25/2016 05:45 PM, Joao Martins wrote:
> Introduce initial support for domainBlockStats API call that
> allow us to query block device statistics. openstack nova

s/openstack/OpenStack/

> uses this API call to query block statistics, alongside
> virDomainMemoryStats and virDomainInterfaceStats.  Note that
> this patch only introduces it for VBD for starters. QDisk
> would come in a separate patch series.
>
> A new statistics data structure is introduced to fit common
> statistics among others specific to the underlying block
> backends. For the VBD statistics on linux these are exported
> via sysfs on the path:
>
> "/sys/bus/xen-backend/devices/vbd-<domid>-<devid>/statistics"
>
> To calculate the block devno libxlDiskPathToID is introduced.
> Each backend implements its own function to extract statistics
> and let there be handled the different platforms.

I reworded this to

    Each backend implements its own function to extract statistics,
    allowing support for multiple backends and different platforms.

>
> VBD stats are exposed in reqs and number of sectors from
> blkback, and it's up to us to convert it to sector sizes.
> The sector size is gathered through xenstore in the device
> backend entry "physical-sector-size".
>
> BlockStatsFlags variant is also implemented which has the
> added benefit of getting the number of flush requests.
>
> Signed-off-by: Joao Martins <joao.m.martins at oracle.com>
> ---
> Changes since v4:
>  - No need to handle virtpath as devid - return id = -1 instead
>  - Rework compilation of BlockStatsVBD compilation on other platforms.
>  - Use disk->dst as opposed to plain path to also cover the case when
>  input virtpath is path to file. Before it was returning "cannot find
>  device number"
>
> Changes since v3:
>  - No need for error in libxlDiskSectorSize()
>  - Fix an identation issue.
>  - Rework cleanup paths on libxlDiskSectorSize()
>  - Do not unlock vm if libxlDomainObjEndJob() returns false and
>  adjust to the newly introduced virDomainObjEndAPI()
>  - Bump version to 2.1.0
>
> Changes since v1:
>  - Fix identation issues
>  - Set ret to LIBXL_VBD_SECTOR_SIZE
>  - Reuse VIR_STRDUP error instead of doing virReportError
>  when we fail to set stats->backend
>  - Change virAsprintf(...) error checking
>  - Change error to VIR_ERR_OPERATION_FAILED when xenstore path
>  does not exist and when failing to read stat.
>  - Resolve issues with 'make syntax-check' with cppi installed.
>  - Remove libxlDiskPathMatches in favor of using virutil
>  virDiskNameParse to fetch disk and partition index.
>  - Rename libxlDiskPathParse to libxlDiskPathToID and rework
>  function to just convert disk and partition index to devno.
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_driver.c | 367 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 367 insertions(+)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index f153f69..b33c898 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -30,6 +30,7 @@
>  #include <math.h>
>  #include <libxl.h>
>  #include <libxl_utils.h>
> +#include <xenstore.h>
>  #include <fcntl.h>
>  #include <regex.h>
>  
> @@ -72,6 +73,7 @@ VIR_LOG_INIT("libxl.libxl_driver");
>  #define LIBXL_DOM_REQ_HALT     4
>  
>  #define LIBXL_NB_TOTAL_CPU_STAT_PARAM 1
> +#define LIBXL_NB_TOTAL_BLK_STAT_PARAM 6
>  
>  #define HYPERVISOR_CAPABILITIES "/proc/xen/capabilities"
>  #define HYPERVISOR_XENSTORED "/dev/xen/xenstored"
> @@ -100,6 +102,25 @@ struct _libxlOSEventHookInfo {
>      int id;
>  };
>  
> +/* Object used to store disk statistics across multiple xen backends */
> +typedef struct _libxlBlockStats libxlBlockStats;
> +typedef libxlBlockStats *libxlBlockStatsPtr;
> +struct _libxlBlockStats {
> +    long long rd_req;
> +    long long rd_bytes;
> +    long long wr_req;
> +    long long wr_bytes;
> +    long long f_req;
> +
> +    char *backend;
> +    union {
> +        struct {
> +            long long ds_req;
> +            long long oo_req;
> +        } vbd;
> +    } u;
> +};
> +
>  /* Function declarations */
>  static int
>  libxlDomainManagedSaveLoad(virDomainObjPtr vm,
> @@ -5031,6 +5052,350 @@ libxlDomainGetJobStats(virDomainPtr dom,
>  }
>  
>  static int
> +libxlDiskPathToID(const char *virtpath)
> +{
> +    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
> +    int disk, partition, chrused;
> +    int fmt, id;
> +    size_t i;
> +
> +    fmt = id = -1;
> +
> +    /* Find any disk prefixes we know about */
> +    for (i = 0; i < ARRAY_CARDINALITY(drive_prefix); i++) {
> +        if (STRPREFIX(virtpath, drive_prefix[i]) &&
> +            !virDiskNameParse(virtpath, &disk, &partition)) {
> +            fmt = i;
> +            break;
> +        }
> +    }
> +
> +    /* Handle it same way as xvd */
> +    if (fmt < 0 &&
> +        (sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused) >= 2
> +         && chrused == strlen(virtpath)))
> +        fmt = 0;
> +
> +    /* Test indexes ranges and calculate the device id */
> +    switch (fmt) {
> +    case 0: /* xvd */
> +        if (disk <= 15 && partition <= 15)
> +            id = (202 << 8) | (disk << 4) | partition;
> +        else if ((disk <= ((1<<20)-1)) || partition <= 255)
> +            id = (1 << 28) | (disk << 8) | partition;
> +        break;
> +    case 1: /* hd */
> +        if (disk <= 3 && partition <= 63)
> +            id = ((disk < 2 ? 3 : 22) << 8) | ((disk & 1) << 6) | partition;
> +        break;
> +    case 2: /* sd */
> +        if (disk <= 15 && (partition <= 15))
> +            id = (8 << 8) | (disk << 4) | partition;
> +        break;
> +    default: /* invalid */
> +        break;
> +    }
> +    return id;
> +}
> +
> +#define LIBXL_VBD_SECTOR_SIZE 512
> +
> +static int
> +libxlDiskSectorSize(int domid, int devno)
> +{
> +    char *path, *val;
> +    struct xs_handle *handle;
> +    int ret = LIBXL_VBD_SECTOR_SIZE;
> +    unsigned int len;
> +
> +    handle = xs_daemon_open_readonly();
> +    if (!handle) {
> +        VIR_WARN("cannot read sector size");
> +        return ret;
> +    }
> +
> +    path = val = NULL;
> +    if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend",
> +                    domid, devno) < 0)
> +        goto cleanup;
> +
> +    if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL)
> +        goto cleanup;
> +
> +    VIR_FREE(path);
> +    if (virAsprintf(&path, "%s/physical-sector-size", val) < 0)
> +        goto cleanup;
> +
> +    VIR_FREE(val);
> +    if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL)
> +        goto cleanup;
> +
> +    if (sscanf(val, "%d", &ret) != 1)
> +        ret = LIBXL_VBD_SECTOR_SIZE;
> +
> + cleanup:
> +    VIR_FREE(val);
> +    VIR_FREE(path);
> +    xs_daemon_close(handle);
> +    return ret;
> +}
> +
> +#ifdef __linux__
> +static int
> +libxlDomainBlockStatsVBD(virDomainObjPtr vm,
> +                         const char *dev,
> +                         libxlBlockStatsPtr stats)
> +{
> +    int ret = -1;
> +    int devno = libxlDiskPathToID(dev);
> +    int size = libxlDiskSectorSize(vm->def->id, devno);

To avoid calling libxlDiskSectorSize() on an invalid devno, I moved it after the
devno validity check.

Looks good otherwise. ACK. I've pushed it with the tweaked commit message and
below diff squashed in.

Regards,
Jim

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index b33c898..cb501cf 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5148,7 +5148,7 @@ libxlDomainBlockStatsVBD(virDomainObjPtr vm,
 {
     int ret = -1;
     int devno = libxlDiskPathToID(dev);
-    int size = libxlDiskSectorSize(vm->def->id, devno);
+    int size;
     char *path, *name, *val;
     unsigned long long stat;
 
@@ -5158,6 +5158,9 @@ libxlDomainBlockStatsVBD(virDomainObjPtr vm,
                        "%s", _("cannot find device number"));
         return ret;
     }
+
+    size = libxlDiskSectorSize(vm->def->id, devno);
+
     if (VIR_STRDUP(stats->backend, "vbd") < 0)
         return ret;