On Fri, Jan 16, 2009 at 03:46:01PM -0500, john cooper wrote:
We have found certain application scenarios where overriding of the default qemu host cache mode provides a substantial improvement in guest performance. In particular, disabling host caching of the file/dev backing a guest drive. A summary of performance metrics may be found below.
Attached is a simple patch which adds general support for a "cache=*" attribute to be passed to qemu from libvirt as part of a xml <driver> element. It is parsed in a domain's xml definition as well as being generated during output of the same. For example:
<disk type='file' device='disk'> <source file='/guest_roots/fc8.root'/> <target dev='hda'/> <driver name='qemu' type='qwerty' cache='none'/> </disk>
where both the existing "type=*" and added "cache=*" attributes are optional and independent.
That scheme looks good. FYI, I have a patch ready which supports the existing driver name & type attributes for QEMU/KVM, as per existing usage in Xen driver
domain_conf.c | 32 +++++++++++++++++++++++++------- domain_conf.h | 15 +++++++++++++++ qemu_conf.c | 32 ++++++++++++++++++++++++++++----
A couple of extra things needed - Addition to tests/qemuxml2argvtest.c to validate the XML to struct to QEMU ARGV conversion. - Addition to tests/qemuxml2xmltest.c to validate XML to struct to XML round-trip conversions. - Addition to the docs/libvirt.rng XML schema.
3 files changed, 68 insertions(+), 11 deletions(-) ================================================================= --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -80,6 +80,20 @@ enum virDomainDiskBus { VIR_DOMAIN_DISK_BUS_LAST };
+/* summary of all possible host open file cache modes + */ +typedef enum { + VIR_DOMAIN_DISK_CACHE_DEFAULT = 0, + VIR_DOMAIN_DISK_CACHE_DISABLE, + VIR_DOMAIN_DISK_CACHE_WRITEBACK, + VIR_DOMAIN_DISK_CACHE_WRITETHRU, + } host_cachemode;
Can you rename this typedef to virDomainDiskCache, and add a sentinal value VIR_DOMAIN_DISK_CACHE_LAST
+ +typedef struct { + char *tag; + host_cachemode idx; + } cache_map_t;
This 2nd struct is not required.
/* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -91,6 +105,7 @@ struct _virDomainDiskDef { char *dst; char *driverName; char *driverType; + host_cachemode cachemode;
Our code policy is to use 'int' when enums are used, since enums have no fixed size.
--- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -616,6 +616,26 @@ qemudNetworkIfaceConnect(virConnectPtr c return NULL; }
+/* map from internal cache mode to qemu cache arg text + */ +static cache_map_t qemu_cache_map[] = { + {"none", VIR_DOMAIN_DISK_CACHE_DISABLE}, + {"writeback", VIR_DOMAIN_DISK_CACHE_WRITEBACK}, + {"writethrough", VIR_DOMAIN_DISK_CACHE_WRITETHRU}, + {NULL}}; + +/* return qemu arg text * corresponding to idx if found, NULL otherwise + */ +static inline char *qemu_cache_mode(host_cachemode idx) +{ + int i; + + for (i = 0; qemu_cache_map[i].tag; ++i) + if (qemu_cache_map[i].idx == idx) + return (qemu_cache_map[i].tag); + return (NULL); +}
This static map & function are not required. Instead the int to char * conversion (and its reverse) are automatically generated through use of our built-in enum support macros In the domain_conf.h header file add VIR_ENUM_DECL(virDomainDiskCache) And then in the domain_conf.c file the impl is provided by VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST, "default", "none", "writeback", "writethrough"); This guarentees (with a compile check) that there is the correct number of constant strings, to match the enum values. You have two generated functions that can be used in the XML parsing/formatting calls int virDomainDiskCacheTypeFromString(const char *type); const char *virDomainDiskCacheTypeFromString(int type);
static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) @@ -946,6 +966,8 @@ int qemudBuildCommandLine(virConnectPtr virDomainDiskDefPtr disk = vm->def->disks[i]; int idx = virDiskNameToIndex(disk->dst); const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); + int nc; + char *cachemode;
if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { @@ -980,15 +1002,17 @@ int qemudBuildCommandLine(virConnectPtr break; }
- snprintf(opt, PATH_MAX, "file=%s,if=%s,%sindex=%d%s%s", + nc = snprintf(opt, PATH_MAX, "file=%s,if=%s,%sindex=%d%s", disk->src ? disk->src : "", bus, media ? media : "", idx, bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK - ? ",boot=on" : "", - disk->shared && ! disk->readonly - ? ",cache=off" : ""); + ? ",boot=on" : ""); + cachemode = qemu_cache_mode(disk->cachemode); + if (cachemode || disk->shared && !disk->readonly) + snprintf(opt + nc, PATH_MAX - nc, ",cache=%s", + cachemode ? cachemode : "off");
There have been two differnet syntaxes supported in QEMU for caching, so we need to detect this and switch between them. Originally there was just cache=on & cache=off (writethrough and no caching) Now it supports cache=writeback, cache=writethrough and cache=none|off So, if we detect the earlier syntax we need to raise an error if the user requested writeback (or translate it to 'cache=off' for safety sake).
ADD_ARG_LIT("-drive"); ADD_ARG_LIT(opt); ================================================================= --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -528,6 +528,16 @@ int virDomainDiskCompare(virDomainDiskDe
#ifndef PROXY + +/* map from xml cache tag to internal cache mode + */ +static cache_map_t cache_map[] = { + {"none", VIR_DOMAIN_DISK_CACHE_DISABLE}, + {"off", VIR_DOMAIN_DISK_CACHE_DISABLE}, + {"writeback", VIR_DOMAIN_DISK_CACHE_WRITEBACK}, + {"writethrough", VIR_DOMAIN_DISK_CACHE_WRITETHRU}, + {NULL}}; +
This chunk is not required, because the VIR_ENUM code takes care of this.
/* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -543,6 +553,8 @@ virDomainDiskDefParseXML(virConnectPtr c char *source = NULL; char *target = NULL; char *bus = NULL; + char *cachetag = NULL; + int i;
if (VIR_ALLOC(def) < 0) { virDomainReportError(conn, VIR_ERR_NO_MEMORY, NULL); @@ -592,6 +604,14 @@ virDomainDiskDefParseXML(virConnectPtr c (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); driverType = virXMLPropString(cur, "type"); + if (cachetag = virXMLPropString(cur, "cache")) { + for (i = 0; cache_map[i].tag; ++i) + if (!strcmp(cache_map[i].tag, cachetag)) { + def->cachemode = cache_map[i].idx; + break; + } + VIR_FREE(cachetag); + }
Indentation got a little mixed up here . The for() loop is can be replaced by a call to virDomainDiskCacheTypeFromString.
} else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -2620,14 +2640,12 @@ virDomainDiskDefFormat(virConnectPtr con type, device);
if (def->driverName) { + virBufferVSprintf(buf, " <driver name='%s'", def->driverName); if (def->driverType) - virBufferVSprintf(buf, - " <driver name='%s' type='%s'/>\n", - def->driverName, def->driverType); - else - virBufferVSprintf(buf, - " <driver name='%s'/>\n", - def->driverName); + virBufferVSprintf(buf, " type='%s'", def->driverType); + if (def->cachemode) + virBufferVSprintf(buf, " cache='%s'", cache_map[def->cachemode].tag);
The virDomainDiskCacheTypeToString() method should be used for this instead Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|