On 09/23/2015 11:24 PM, Jim Fehlig wrote:
> 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.
>
Indeed, I should ignore the return error and just do VIR_FREE(val).
>> +
>> + 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.
>
OK. Two other issues that I already have fixed for v2: 1) libxlDiskPathParse is
renamed to libxlDiskPathToID which is based on virDiskNameParse (as suggested by
Daniel) resulting in a much simpler function; 2) Added validation devno;
>> + 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)'.
>
OK.
>> +
>> + if (!virFileExists(path)) {
>> + virReportError(VIR_ERR_OPERATION_INVALID,
>> + "%s", _("cannot open bus path"));
>>
>
> I think the error should be VIR_ERR_OPERATION_FAILED.
>
OK.
>> + 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?
>
OK.
>> + 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.
>
The macro will always free 'name' and 'val' on successfull (prior?) invocations,
thus only the last one needs to be taken care of isn't it?
>> +
>> +#undef LIBXL_SET_VBDSTAT
>>
>
> Another case of failing 'make syntax-check' with cppi installed.
>
Sorry about this, I didn't notice the cppi warnings. Same issue will appear also
on the next patch, which are fixed already for V2.
Thanks!
Joao
> 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