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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list