Re: [PATCH RFC 4/7] libxl: implement virDomainBlockStats

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

 



Joao Martins wrote:
> Introduce initial support for domainBlockStats API call that
> allow us to query block device statistics. openstack nova
> uses this API call to query block statistics, alongside
> virDomainMemoryStats and virDomainInterfaceStats.  Note that
> this patch only introduces it for VBD for starters. QDisk
> will 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 devid two libxl functions were ported
> (libxlDiskPathMatches and libxlDiskPathParse) to libvirt to
> make sure the devid is calculate in the same way as libxl.
> Each backend implements its own function to extract statistics
> and let there be handled the different platforms. An alternative
> would be to reuse libvirt xen driver function.
>
> 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". This adds up an extra
> dependency namely of xenstore for doing the xs_read.
>
> 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@xxxxxxxxxx>
> ---
>  configure.ac             |   2 +-
>  src/libxl/libxl_driver.c | 420 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 421 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index ef7fbdb..56fb266 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -896,7 +896,7 @@ if test "$with_libxl" != "no" ; then
>          LIBS="$LIBS $LIBXL_LIBS"
>          AC_CHECK_LIB([xenlight], [libxl_ctx_alloc], [
>              with_libxl=yes
> -            LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl"
> +            LIBXL_LIBS="$LIBXL_LIBS -lxenlight -lxenctrl -lxenstore"
>          ],[
>              if test "$with_libxl" = "yes"; then
>                  fail=1
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index dc83083..fd952a3 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -59,6 +59,7 @@
>  #include "network/bridge_driver.h"
>  #include "locking/domain_lock.h"
>  #include "virstats.h"
> +#include <xenstore.h>
>  
>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>  
> @@ -75,6 +76,7 @@ VIR_LOG_INIT("libxl.libxl_driver");
>  #define LIBXL_CONFIG_FORMAT_SEXPR "xen-sxpr"
>  
>  #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"
> @@ -103,6 +105,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,
> @@ -4641,6 +4662,403 @@ libxlDomainIsUpdated(virDomainPtr dom)
>  }
>  
>  static int
> +libxlDiskPathMatches(const char *virtpath, const char *devtype,
> +                     int *index_r, int max_index,
> +                     int *partition_r, int max_partition)
> +{
> +    const char *p;
> +    char *ep;
> +    int tl, c;
> +    long pl;
> +
> +    tl = strlen(devtype);
> +    if (memcmp(virtpath, devtype, tl))
> +        return 0;
> +
> +    /* We decode the drive letter as if it were in base 52
> +     * with digits a-zA-Z, more or less */
> +    *index_r = -1;
> +    p = virtpath + tl;
> +    for (;;) {
> +        c = *p++;
> +        if (c >= 'a' && c <= 'z') {
> +            c -= 'a';
> +        } else {
> +            --p;
> +            break;
> +        }
> +        (*index_r)++;
> +        (*index_r) *= 26;
> +        (*index_r) += c;
> +
> +        if (*index_r > max_index)
> +            return 0;
> +    }
> +
> +    if (!*p) {
> +        *partition_r = 0;
> +        return 1;
> +    }
> +
> +    if (*p == '0')
> +        return 0; /* leading zeroes not permitted in partition number */
> +
> +    if (virStrToLong_l(p, &ep, 10, &pl) < 0)
> +        return 0;
> +
> +    if (pl > max_partition || *ep)
> +        return 0;
> +
> +    *partition_r = pl;
> +    return 1;
> +}
> +
> +static int
> +libxlDiskPathParse(const char *virtpath, int *pdisk, int *ppartition)
> +{
> +    int disk, partition;
> +    char *ep;
> +    unsigned long ul;
> +    int chrused;
> +
> +    chrused = -1;
> +    if ((sscanf(virtpath, "d%ip%i%n", &disk, &partition, &chrused)  >= 2
> +         && chrused == strlen(virtpath) && disk < (1<<20) && partition < 256)
> +        ||
> +        libxlDiskPathMatches(virtpath, "xvd",
> +                             &disk, (1<<20)-1,
> +                             &partition, 255)) {
> +        if (pdisk) *pdisk = disk;
> +        if (ppartition) *ppartition = partition;
> +        if (disk <= 15 && partition <= 15)
> +            return (202 << 8) | (disk << 4) | partition;
> +        else
> +            return (1 << 28) | (disk << 8) | partition;
> +    }
> +
> +    errno = virStrToLong_ul(virtpath, &ep, 0, &ul);
> +    if (!errno && !*ep && ul <= INT_MAX) {
> +        /* FIXME: should parse ul to determine these. */
> +        if (pdisk || ppartition)
> +            return -1;
> +        return ul;
> +    }
> +
> +    if (libxlDiskPathMatches(virtpath, "hd",
> +                             &disk, 3,
> +                             &partition, 63)) {
> +        if (pdisk) *pdisk = disk;
> +        if (ppartition) *ppartition = partition;
> +        return ((disk<2 ? 3 : 22) << 8) | ((disk & 1) << 6) | partition;
> +    }
> +    if (libxlDiskPathMatches(virtpath, "sd",
> +                             &disk, 15,
> +                             &partition, 15)) {
> +        if (pdisk) *pdisk = disk;
> +        if (ppartition) *ppartition = partition;
> +        return (8 << 8) | (disk << 4) | partition;
> +    }
> +    return -1;
> +}
> +
> +#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) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("cannot read sector size"));
> +        return ret;
> +    }
> +
> +    if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend",
> +                domid, devno) < 0)
>   

Indentation looks off.

> +        goto close;
> +
> +    if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL)
> +        goto cleanup;
> +
> +    VIR_FREE(path);
> +    if (virAsprintf(&path, "%s/physical-sector-size", val) < 0) {
> +        VIR_FREE(val);
> +        goto close;
> +    }
> +
> +    VIR_FREE(val);
> +    if ((val = xs_read(handle, XBT_NULL, path, &len)) == NULL)
> +        goto cleanup;
> +
> +    if (sscanf(val, "%d", &ret) != 1)
> +        ret = -1;
>   

Should ret be set back to LIBXL_VBD_SECTOR_SIZE? '-1' wouldn't be a good
value for the calling function.

> +
> +    VIR_FREE(val);
> +
> + cleanup:
> +    VIR_FREE(path);
> + close:
> +    xs_daemon_close(handle);
> +    return ret;
> +}
> +
> +static int
> +libxlDomainBlockStatsVBD(virDomainObjPtr vm,
> +                         const char *dev,
> +                         libxlBlockStatsPtr stats)
> +{
> +    int ret = -1;
> +    int devno = libxlDiskPathParse(dev, NULL, NULL);
> +    int size = libxlDiskSectorSize(vm->def->id, devno);
> +#ifdef __linux__
> +    char *path, *name, *val;
> +    unsigned long long stat;
> +
> +    if (VIR_STRDUP(stats->backend, "vbd") < 0) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("cannot set backend"));
>   

VIR_STRDUP() already reports OOM error, which is about the only one it
can encounter.

> +        return -1;
> +    }
> +
> +    ret = virAsprintf(&path, "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics",
> +                vm->def->id, devno);
>   

The usual pattern is 'if (virAsprintf(...) < 0)'.

> +
> +    if (!virFileExists(path)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("cannot open bus path"));
>   

I think the error should be VIR_ERR_OPERATION_FAILED.

> +        goto cleanup;
> +    }
> +
> +#define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL)            \
>   

Indentation is off here. Fails 'make syntax-check' with cppi installed.

> +    if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \
> +        (virFileReadAll(name, 256, &val) < 0) ||       \
> +        (sscanf(val, "%llu", &stat) != 1)) {          \
> +        virReportError(VIR_ERR_OPERATION_INVALID, \
> +                       _("cannot read %s"), name);    \
>   

VIR_ERR_OPERATION_FAILED?

> +        goto cleanup;                                 \
> +    }                                                 \
> +    VAR += (stat * MUL);                              \
> +    VIR_FREE(name);                                   \
> +    VIR_FREE(val);
> +
> +    LIBXL_SET_VBDSTAT("f_req",  stats->f_req,  1)
> +    LIBXL_SET_VBDSTAT("wr_req", stats->wr_req, 1)
> +    LIBXL_SET_VBDSTAT("rd_req", stats->rd_req, 1)
> +    LIBXL_SET_VBDSTAT("wr_sect", stats->wr_bytes, size)
> +    LIBXL_SET_VBDSTAT("rd_sect", stats->rd_bytes, size)
> +
> +    LIBXL_SET_VBDSTAT("ds_req", stats->u.vbd.ds_req, size)
> +    LIBXL_SET_VBDSTAT("oo_req", stats->u.vbd.oo_req, 1)
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(name);
> +    VIR_FREE(path);
> +    VIR_FREE(val);
>   

This will only cleanup 'name' and 'val' from the last invocation of the
LIBXL_SET_VBDSTAT macro. 'name' and 'val' from the prior invocations
will be leaked.

> +
> +#undef LIBXL_SET_VBDSTAT
>   

Another case of failing 'make syntax-check' with cppi installed.

Regards,
Jim

> +
> +#else
> +    virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                   "%s", _("platform unsupported"));
> +#endif
> +    return ret;
> +}
> +
> +static int
> +libxlDomainBlockStatsGatherSingle(virDomainObjPtr vm,
> +                                  const char *path,
> +                                  libxlBlockStatsPtr stats)
> +{
> +    virDomainDiskDefPtr disk;
> +    const char *disk_drv;
> +    int ret = -1, disk_fmt;
> +
> +    if (!(disk = virDomainDiskByName(vm->def, path, false))) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("invalid path: %s"), path);
> +        return ret;
> +    }
> +
> +    disk_fmt = virDomainDiskGetFormat(disk);
> +    if (!(disk_drv = virDomainDiskGetDriver(disk)))
> +        disk_drv = "qemu";
> +
> +    if (STREQ(disk_drv, "phy")) {
> +        if (disk_fmt != VIR_STORAGE_FILE_RAW &&
> +            disk_fmt != VIR_STORAGE_FILE_NONE) {
> +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                           _("unsupported format %s"),
> +                           virStorageFileFormatTypeToString(disk_fmt));
> +            return ret;
> +        }
> +
> +        ret = libxlDomainBlockStatsVBD(vm, path, stats);
> +    } else {
> +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                           _("unsupported disk driver %s"),
> +                           disk_drv);
> +            return ret;
> +    }
> +    return ret;
> +}
> +
> +static int
> +libxlDomainBlockStatsGather(virDomainObjPtr vm,
> +                            const char *path,
> +                            libxlBlockStatsPtr stats)
> +{
> +    int ret = -1;
> +    if (*path) {
> +        if (libxlDomainBlockStatsGatherSingle(vm, path, stats) < 0)
> +            return ret;
> +    } else {
> +        size_t i;
> +        for (i = 0; i < vm->def->ndisks; ++i) {
> +            if (libxlDomainBlockStatsGatherSingle(vm, vm->def->disks[i]->dst,
> +                                                  stats) < 0)
> +                return ret;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int
> +libxlDomainBlockStats(virDomainPtr dom,
> +                      const char *path,
> +                      virDomainBlockStatsPtr stats)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    libxlBlockStats blkstats;
> +    int ret = -1;
> +
> +    if (!(vm = libxlDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainBlockStatsEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
> +        goto cleanup;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto endjob;
> +    }
> +
> +    memset(&blkstats, 0, sizeof(libxlBlockStats));
> +    if ((ret = libxlDomainBlockStatsGather(vm, path, &blkstats)) < 0)
> +        goto endjob;
> +
> +    stats->rd_req = blkstats.rd_req;
> +    stats->rd_bytes = blkstats.rd_bytes;
> +    stats->wr_req = blkstats.wr_req;
> +    stats->wr_bytes = blkstats.wr_bytes;
> +    if (STREQ_NULLABLE(blkstats.backend, "vbd"))
> +        stats->errs = blkstats.u.vbd.oo_req;
> +    else
> +        stats->errs = -1;
> +
> + endjob:
> +    if (!libxlDomainObjEndJob(driver, vm)) {
> +        virObjectUnlock(vm);
> +        vm = NULL;
> +    }
> +
> + cleanup:
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}
> +
> +static int
> +libxlDomainBlockStatsFlags(virDomainPtr dom,
> +                           const char *path,
> +                           virTypedParameterPtr params,
> +                           int *nparams,
> +                           unsigned int flags)
> +{
> +    libxlDriverPrivatePtr driver = dom->conn->privateData;
> +    virDomainObjPtr vm;
> +    libxlBlockStats blkstats;
> +    int nstats;
> +    int ret = -1;
> +
> +    virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +    flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
> +
> +    if (!(vm = libxlDomObjFromDomain(dom)))
> +        goto cleanup;
> +
> +    if (virDomainBlockStatsFlagsEnsureACL(dom->conn, vm->def) < 0)
> +        goto cleanup;
> +
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_QUERY) < 0)
> +        goto cleanup;
> +
> +    if (!virDomainObjIsActive(vm)) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       "%s", _("domain is not running"));
> +        goto endjob;
> +    }
> +
> +    /* return count of supported stats */
> +    if (*nparams == 0) {
> +        *nparams = LIBXL_NB_TOTAL_BLK_STAT_PARAM;
> +        ret = 0;
> +        goto endjob;
> +    }
> +
> +    memset(&blkstats, 0, sizeof(libxlBlockStats));
> +    if ((ret = libxlDomainBlockStatsGather(vm, path, &blkstats)) < 0)
> +        goto endjob;
> +
> +    nstats = 0;
> +
> +#define LIBXL_BLKSTAT_ASSIGN_PARAM(VAR, NAME)                              \
> +    if (nstats < *nparams && (blkstats.VAR) != -1) {                       \
> +        if (virTypedParameterAssign(params + nstats, NAME,                 \
> +                                    VIR_TYPED_PARAM_LLONG, (blkstats.VAR)) < 0) \
> +            goto endjob;                                                   \
> +        nstats++;                                                          \
> +    }
> +
> +    LIBXL_BLKSTAT_ASSIGN_PARAM(wr_bytes, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES);
> +    LIBXL_BLKSTAT_ASSIGN_PARAM(wr_req, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ);
> +
> +    LIBXL_BLKSTAT_ASSIGN_PARAM(rd_bytes, VIR_DOMAIN_BLOCK_STATS_READ_BYTES);
> +    LIBXL_BLKSTAT_ASSIGN_PARAM(rd_req, VIR_DOMAIN_BLOCK_STATS_READ_REQ);
> +
> +    LIBXL_BLKSTAT_ASSIGN_PARAM(f_req, VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ);
> +
> +    if (STREQ_NULLABLE(blkstats.backend, "vbd"))
> +        LIBXL_BLKSTAT_ASSIGN_PARAM(u.vbd.oo_req, VIR_DOMAIN_BLOCK_STATS_ERRS);
> +
> +    *nparams = nstats;
> +
> +#undef LIBXL_BLKSTAT_ASSIGN_PARAM
> +
> + endjob:
> +    if (!libxlDomainObjEndJob(driver, vm)) {
> +        virObjectUnlock(vm);
> +        vm = NULL;
> +    }
> +
> + cleanup:
> +    if (vm)
> +        virObjectUnlock(vm);
> +    return ret;
> +}
> +
> +static int
>  libxlDomainInterfaceStats(virDomainPtr dom,
>                            const char *path,
>                            virDomainInterfaceStatsPtr stats)
> @@ -5459,6 +5877,8 @@ static virHypervisorDriver libxlHypervisorDriver = {
>  #endif
>      .nodeGetFreeMemory = libxlNodeGetFreeMemory, /* 0.9.0 */
>      .nodeGetCellsFreeMemory = libxlNodeGetCellsFreeMemory, /* 1.1.1 */
> +    .domainBlockStats = libxlDomainBlockStats, /* 1.2.20 */
> +    .domainBlockStatsFlags = libxlDomainBlockStatsFlags, /* 1.2.20 */
>      .domainInterfaceStats = libxlDomainInterfaceStats, /* 1.2.20 */
>      .domainMemoryStats = libxlDomainMemoryStats, /* 1.2.20 */
>      .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.2.20 */
>   

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]