On 07/25/2016 09:35 PM, Jim Fehlig wrote:
On 07/23/2016 05:34 AM, Joao Martins wrote:
On 07/22/2016 10:50 PM, Jim Fehlig wrote:
On 07/20/2016 04:48 PM, 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 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. Too bad libxl or libxlutil doesn't provide such a function. Yeah :( Unfortunately the function is private to libxl.
But we could get devno indirectly through libxl_device_disk_getinfo provided by libxl_diskinfo->devid field. But at the same time that function does much more than that i.e. 5 other xenstore reads being the reason why I didn't pursued this way initially. On the other hand while it's incurs more overhead for just calculating an ID, it's better towards unlikely adjustments on this area. I say unlikely because this hasn't changed for the past 6 years (Xen 4.0?), see xen commit e9703a9 ("libxl: Properly parse vbd name"). What do you think?
I think copying the pertinent code to libxlDiskPathToID is the best approach in this case. OK.
xend, whose functionality was replaced by the libvirt libxl driver, had a similar function
http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/python/xen/util/blki...
Yep. [snip]
static int +libxlDiskPathToID(const char *virtpath) +{ + static char const* drive_prefix[] = {"xvd", "hd", "sd"}; + int disk, partition, chrused; + int fmt, id; + char *r;[snip + 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; The purpose of the default case is not clear to me. Can you explain that a bit? IIUC, virtpath would contain values such as xvda, sdc, or /path/to/image. virtpath would only contain device names (no image paths). This falltrough case is to have device number directly from unsigned long instead of parsing device name. See (xen) libxl commit 8ca0ed2cd ("libxl: fail to parse disk vpath if a disk+part number needed but unavailable") as docs/misc/vbd-interface.txt isn't exactly clear on this. Regardless probably doesn't make a lot of sense for libvirt since we can only have device names. I will probably just set "id = -1" or depending on your preference rely on libxl_device_disk_getinfo(..) returned devid.
Yeah, I don't think it makes much sense for libvirt. virtpath is the 'disk' parameter in virDomainBlockStats*, which is documented as
/nods
"The @disk parameter is either the device target shorthand (the <target dev='...'/> sub-element, such as "vda"), or (since 0.9.8) an unambiguous source name of the block device (the <source file='...'/> sub-element, such as "/path/to/image"). Valid names can be found by calling virDomainGetXMLDesc() and inspecting elements within //domain/devices/disk. Some drivers might also accept the empty string for the @disk parameter, and then yield summary stats for the entire domain."
My apologies, I was misinformed that the path could actually be a file and wasn't handling that case in this patch. I've fixed that in v5 - and since I fetch anyways the DiskDef by path I can just use disk->dst (aka disk/target/@dev). Note that I don't validate disk->dst existence which AFAICT disk definition will always have this dst field not NULL.
Setting 'id = -1' is the right thing to do IMO. OK.
+ } + 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; +} + +static int +libxlDomainBlockStatsVBD(virDomainObjPtr vm, + const char *dev, + libxlBlockStatsPtr stats) The stats parameter is unused when __linux__ is not defined. Otherwise, the patch looks fine. Ah yes, and size will get declared but not used too. Would you prefer resorting to this syntax instead:
#ifdef __linux__ static int libxlDomainBlockStatsVBD(virDomainObjPtr vm, const char *dev, libxlBlockStatsPtr stats) { ... } #else static int libxlDomainBlockStatsVBD(virDomainObjPtr vm, const char *dev ATTRIBUTE_UNUSED, libxlBlockStatsPtr stats ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("platform unsupported")); return -1; } #endif
Or in alternative moving __linux__ conditional above devno declaration and always set ATTRIBUTE_UNUSED on dev and stats params?
grepping through the sources shows both patterns are used. I find the first one more readable and future-proof. And having ATTRIBUTE_UNUSED on a used param feels odd to me :-).
Hehe - I also lean towards option 1, much more readable. Just sent v5 here: https://www.redhat.com/archives/libvir-list/2016-July/msg01029.html Regards, Joao