6:58 a.m.
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 :|