[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] patch: allow disk cache mode to be specified in a domain's xml definition.



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]