[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |