Re: [libvirt] [PATCH v5] libxl: implement virDomainBlockStats

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(a)oracle.com&gt; > --- > 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.

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;