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

Re: [Xen-devel] [PATCH v3 5/8] libxl: implement virDomainBlockStats




On 11/18/2015 07:01 PM, Jim Fehlig wrote:
> On 11/13/2015 06:14 AM, 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 devno libxlDiskPathToID is introduced.
>> 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>
>> ---
>> 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
>> ---
>>  configure.ac             |   2 +-
>>  src/libxl/libxl_driver.c | 375 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 376 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index f481c50..10c56e5 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 9a5a74c..ba1d67b 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,
>> @@ -4644,6 +4665,358 @@ libxlDomainIsUpdated(virDomainPtr dom)
>>  }
>>  
>>  static int
>> +libxlDiskPathToID(const char *virtpath)
>> +{
>> +    static char const* drive_prefix[] = {"xvd", "hd", "sd"};
>> +    int disk, partition, chrused;
>> +    int fmt, id;
>> +    char *r;
>> +    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:
>> +        errno = virStrToLong_i(virtpath, &r, 0, &id);
>> +        if (errno || *r || id > INT_MAX)
>> +            id = -1;
>> +        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) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s", _("cannot read sector size"));
> 
> Probably no need to report an error here since LIBXL_VBD_SECTOR_SIZE is 
> returned
> and used by the caller. Maybe a VIR_WARN is more appropriate.
> 
OK.

>> +        return ret;
>> +    }
>> +
>> +    if (virAsprintf(&path, "/local/domain/%d/device/vbd/%d/backend",
>> +                    domid, devno) < 0)
>> +        goto close;
> 
> Perhaps a bit cleaner to initialize path and val to NULL then have a single
> 'cleanup' label where they are freed. Then you would only need to VIR_FREE 
> path
> and val before re-purposing them.
>
Good point. I will rework it that way.

>> +
>> +    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 = LIBXL_VBD_SECTOR_SIZE;
>> +
>> +    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 = libxlDiskPathToID(dev);
>> +    int size = libxlDiskSectorSize(vm->def->id, devno);
>> +#ifdef __linux__
>> +    char *path, *name, *val;
>> +    unsigned long long stat;
>> +
>> +    path = name = val = NULL;
>> +    if (devno < 0) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>> +                       "%s", _("cannot find device number"));
>> +        return ret;
>> +    }
>> +    if (VIR_STRDUP(stats->backend, "vbd") < 0)
>> +        return ret;
>> +
>> +    if (virAsprintf(&path, 
>> "/sys/bus/xen-backend/devices/vbd-%d-%d/statistics",
>> +                    vm->def->id, devno) < 0)
>> +        return ret;
>> +
>> +    if (!virFileExists(path)) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED,
>> +                       "%s", _("cannot open bus path"));
>> +        goto cleanup;
>> +    }
>> +
>> +# define LIBXL_SET_VBDSTAT(FIELD, VAR, MUL)           \
>> +    if ((virAsprintf(&name, "%s/"FIELD, path) < 0) || \
>> +        (virFileReadAll(name, 256, &val) < 0) ||      \
>> +        (sscanf(val, "%llu", &stat) != 1)) {          \
>> +        virReportError(VIR_ERR_OPERATION_FAILED,     \
>> +                       _("cannot read %s"), name);    \
>> +        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);
>> +
>> +# undef LIBXL_SET_VBDSTAT
>> +
>> +#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;
> 
> Indentation looks off.
> 
OK, and also remove this unnecessary return there.

>> +    }
>> +    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;
>> +    }
> 
> Currently, libxlDomainObjEndJob returns false when the virDomainObj refcnt has
> reached 0, in which case it will be disposed and doesn't need unlocked. Notice
> the pattern in all the other calls to libxlDomainObjEndJob. (I say 'currently'
> because I'm working on a patch to change the ref counting and job handling
> similar to commit 540c339a for the QEMU driver.)
> 
You mean the followup of this one right?
(https://www.redhat.com/archives/libvir-list/2015-June/msg00711.html). It
appears I do the same mistake on InterfaceStats and ConnectGetAllDomainStats, so
I will fix that too.

>> +
>> + 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;
>> +    }
> 
> Same comment here.
> 
OK.

> Regards,
> Jim
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.