[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.



Daniel P. Berrange wrote:
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.
Ok. Let's address the above when we're satisfied with
the state of the patch as we have at least one open
issue (as below).

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
Yes you mentioned this in your earlier (off list) reply
to me. I did produce a patch version using the
VIR_ENUM_DECL/VIR_ENUM_IMPL macros but thought they may
not be the most natural fit for the task since there
is no way to multiply define a particular enum value
(eg: cache "none" || "off"). Also a given hypervisor will
likely support only a subset of the internally enumerated
option space forcing dummy-out of unused cache modes. But
that is speculative and I don't have a strong bias either
way. So I've reverted to use of VIR_ENUM_DECL/VIR_ENUM_IMPL
here.


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).
I was trying to address part of this as above. But in general
I don't see how we can force an error in advance of launching
qemu without some information beforehand of whether we're
dealing with an old vs. new syntax qemu. One way to do so is
to test launch "qemu --help" and let it tell us what it accepts.
Alternatively we can just let qemu error exit in the case an
old/new qemu version doesn't recognize the alternate syntax.

Other than the above I believe I've incorporated all of
your remaining suggestions. Attached is an updated patch.

-john


--
john cooper redhat com

 domain_conf.c |   31 ++++++++++++++++++++++++-------
 domain_conf.h |   16 ++++++++++++++++
 qemu_conf.c   |   27 +++++++++++++++++++++++----
 3 files changed, 63 insertions(+), 11 deletions(-)
=================================================================
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -80,6 +80,21 @@ enum virDomainDiskBus {
     VIR_DOMAIN_DISK_BUS_LAST
 };
 
+/* summary of all possible host open file cache modes
+ */
+typedef enum {
+    VIR_DOMAIN_DISK_CACHE_UNSPECIFIED,	/* reserved */
+    VIR_DOMAIN_DISK_CACHE_OFF,
+    VIR_DOMAIN_DISK_CACHE_ON,
+    VIR_DOMAIN_DISK_CACHE_DISABLE,
+    VIR_DOMAIN_DISK_CACHE_WRITEBACK,
+    VIR_DOMAIN_DISK_CACHE_WRITETHRU,
+
+    VIR_DOMAIN_DISK_CACHE_LAST
+    } virDomainDiskCache;
+
+VIR_ENUM_DECL(virDomainDiskCache)
+
 /* Stores the virtual disk configuration */
 typedef struct _virDomainDiskDef virDomainDiskDef;
 typedef virDomainDiskDef *virDomainDiskDefPtr;
@@ -91,6 +106,7 @@ struct _virDomainDiskDef {
     char *dst;
     char *driverName;
     char *driverType;
+    int cachemode;
     unsigned int readonly : 1;
     unsigned int shared : 1;
     int slotnum; /* pci slot number for unattach */
=================================================================
--- a/src/qemu_conf.c
+++ b/src/qemu_conf.c
@@ -616,6 +616,20 @@ qemudNetworkIfaceConnect(virConnectPtr c
     return NULL;
 }
 
+/* map from internal cache mode to qemu cache arg text
+ *
+ * Note: currently this replicates virDomainDiskCache, but will need to
+ * error flag potential new entries in virDomainDiskCache which are
+ * not supported by qemu (raising exceptions as appropriate).
+ */ 
+VIR_ENUM_IMPL(qemu_cache_map, VIR_DOMAIN_DISK_CACHE_LAST,
+    "",         /* reserved -- mode not specified */
+    "off",      /* depreciated by "none" */
+    "on",       /* obsolete by "writethrough" */
+    "none",
+    "writeback",
+    "writethrough")
+
 static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev,
                                           char *buf,
                                           int buflen)
@@ -946,6 +960,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 +996,18 @@ 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 = disk->cachemode ?
+                qemu_cache_mapTypeToString(disk->cachemode): NULL;
+            if (cachemode || disk->shared && !disk->readonly)
+                snprintf(opt + nc, PATH_MAX - nc, ",cache=%s",
+                    cachemode ? cachemode : "off");
 
             ADD_ARG_LIT("-drive");
             ADD_ARG_LIT(opt);
=================================================================
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -528,6 +528,17 @@ int virDomainDiskCompare(virDomainDiskDe
 
 
 #ifndef PROXY
+
+/* map from xml cache tag to internal cache mode
+ */
+VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST,
+    "",         /* reserved -- mode not specified */
+    "off",
+    "on",
+    "none",
+    "writeback",
+    "writethrough");
+
 /* Parse the XML definition for a disk
  * @param node XML nodeset to parse for disk definition
  */
@@ -543,6 +554,8 @@ virDomainDiskDefParseXML(virConnectPtr c
     char *source = NULL;
     char *target = NULL;
     char *bus = NULL;
+    char *cachetag = NULL;
+    int cm;
 
     if (VIR_ALLOC(def) < 0) {
         virDomainReportError(conn, VIR_ERR_NO_MEMORY, NULL);
@@ -592,6 +605,11 @@ virDomainDiskDefParseXML(virConnectPtr c
                        (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
                 driverName = virXMLPropString(cur, "name");
                 driverType = virXMLPropString(cur, "type");
+                if (cachetag = virXMLPropString(cur, "cache")) {
+                    if ((cm = virDomainDiskCacheTypeFromString(cachetag)) != -1)
+                        def->cachemode = cm;
+                    VIR_FREE(cachetag);
+                }
             } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
                 def->readonly = 1;
             } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
@@ -2620,14 +2638,13 @@ 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'",
+                virDomainDiskCacheTypeToString(def->cachemode));
+        virBufferVSprintf(buf, "/>\n");
     }
 
     if (def->src) {

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