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



Jim Meyering wrote:

Hi John,

I tried to apply that, but failed miserably,
since all of the following was recently redone to
use virBufferVSprintf rather than snprintf.
Yea I suspected the code was likely seeing some
motion. Thanks for bringing it forward.
And it's
a good thing, because with the changes below, there was
potential buffer overrun: nc could end up larger than PATH_MAX.
You are correct. I mistook the return value of
snprintf() in the case of truncation.

One remaining bit that I haven't done:
add tests for this, exercising e.g.,
  - cachemode
  - !cachemode && (disk->shared && !disk->readonly)
  - !cachemode && (!disk->shared || disk->readonly)
That logic should be correct as it exists in the
patch, namely we'll generate a "cache=*" flag in
the case cachemode was specified explicitly in
the xml definition or for the preexisting case of
(shared && !readonly). Here if both happen to be
true the explicit xml cache tag takes precedence.

But with the existing code we need to remove the
clause of:

@@ -1007,18 +1025,34 @@ int qemudBuildCommandLine(virConnectPtr
if (bootable &&
disk->device == VIR_DOMAIN_DISK_DEVICE_DISK)
virBufferAddLit(&opt, ",boot=on");
- if (disk->shared && !disk->readonly)
- virBufferAddLit(&opt, ",cache=off");
if (disk->driverType)
virBufferVSprintf(&opt, ",fmt=%s", disk->driverType);

As this results in generation of a "cache=*"
twice on the qemu cmdline, and possibly of
different dispositions.

With this change I think it is good to go.
The attached patch incorporates the above
modification.

-john

--
john cooper redhat com

 domain_conf.c |   34 ++++++++++++++++++++++++++--------
 domain_conf.h |   16 ++++++++++++++++
 qemu_conf.c   |   43 ++++++++++++++++++++++++++++++++++++++-----
 qemu_conf.h   |    3 ++-
 4 files changed, 82 insertions(+), 14 deletions(-)
=================================================================
--- a/src/domain_conf.h
+++ b/src/domain_conf.h
@@ -81,6 +81,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;
@@ -92,6 +107,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
@@ -1,7 +1,7 @@
 /*
  * config.c: VM configuration management
  *
- * Copyright (C) 2006, 2007, 2008 Red Hat, Inc.
+ * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -399,7 +399,9 @@ int qemudExtractVersionInfo(const char *
     if (strstr(help, "-domid"))
         flags |= QEMUD_CMD_FLAG_DOMID;
     if (strstr(help, "-drive"))
-        flags |= QEMUD_CMD_FLAG_DRIVE;
+        flags |= QEMUD_CMD_FLAG_DRIVE |
+            (strstr(help, "cache=writethrough|writeback|none") ?
+            QEMUD_CMD_FLAG_DRIVE_CACHEMODE : 0);
     if (strstr(help, "boot=on"))
         flags |= QEMUD_CMD_FLAG_DRIVE_BOOT;
     if (version >= 9000)
@@ -591,6 +593,22 @@ qemudNetworkIfaceConnect(virConnectPtr c
     return NULL;
 }
 
+VIR_ENUM_DECL(qemu_cache_map)
+
+/* 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",      /* deprecated; use "none" */
+    "on",       /* obsolete;   use "writethrough" */
+    "none",
+    "writeback",
+    "writethrough")
+
 static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev,
                                           char *buf,
                                           int buflen)
@@ -1007,11 +1025,27 @@ int qemudBuildCommandLine(virConnectPtr 
             if (bootable &&
                 disk->device == VIR_DOMAIN_DISK_DEVICE_DISK)
                 virBufferAddLit(&opt, ",boot=on");
-            if (disk->shared && !disk->readonly)
-                virBufferAddLit(&opt, ",cache=off");
             if (disk->driverType)
                 virBufferVSprintf(&opt, ",fmt=%s", disk->driverType);
 
+            unsigned int qf;
+            int cachemode = disk->cachemode;
+            if (cachemode) {
+                if (qemudExtractVersionInfo(vm->def->emulator, NULL, &qf) < 0)
+                    ;    /* error reported */
+                else if (!(qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE)
+                         && cachemode == VIR_DOMAIN_DISK_CACHE_WRITETHRU)
+                    cachemode = VIR_DOMAIN_DISK_CACHE_ON;
+                else if (qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE
+                         && cachemode == VIR_DOMAIN_DISK_CACHE_ON)
+                    cachemode = VIR_DOMAIN_DISK_CACHE_WRITETHRU;
+            }
+            if (cachemode || (disk->shared && !disk->readonly))
+                virBufferVSprintf(&opt, ",cache=%s",
+                                  (cachemode
+                                   ? qemu_cache_mapTypeToString(cachemode)
+                                   : "off"));
+
             if (virBufferError(&opt)) {
                 virReportOOMError(conn);
                 goto error;
@@ -1577,4 +1611,3 @@ cleanup:
     VIR_FREE(xml);
     return ret;
 }
-
=================================================================
--- a/src/qemu_conf.h
+++ b/src/qemu_conf.h
@@ -1,7 +1,7 @@
 /*
  * config.h: VM configuration management
  *
- * Copyright (C) 2006, 2007 Red Hat, Inc.
+ * Copyright (C) 2006, 2007, 2009 Red Hat, Inc.
  * Copyright (C) 2006 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -53,6 +53,7 @@ enum qemud_cmd_flags {
                                                    * since it had a design bug blocking the entire monitor console */
     QEMUD_CMD_FLAG_MIGRATE_QEMU_TCP  = (1 << 10), /* New migration syntax after merge to QEMU with TCP transport */
     QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC = (1 << 11), /* New migration syntax after merge to QEMU with EXEC transport */
+    QEMUD_CMD_FLAG_DRIVE_CACHEMODE = (1 << 12), /* Is the -cache option available */
 };
 
 /* Main driver state */
=================================================================
--- a/src/domain_conf.c
+++ b/src/domain_conf.c
@@ -1,7 +1,7 @@
 /*
  * domain_conf.c: domain XML processing
  *
- * Copyright (C) 2006-2008 Red Hat, Inc.
+ * Copyright (C) 2006-2009 Red Hat, Inc.
  * Copyright (C) 2006-2008 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -551,6 +551,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
  */
@@ -567,6 +578,8 @@ virDomainDiskDefParseXML(virConnectPtr c
     char *source = NULL;
     char *target = NULL;
     char *bus = NULL;
+    char *cachetag = NULL;
+    int cm;
 
     if (VIR_ALLOC(def) < 0) {
         virReportOOMError(conn);
@@ -616,6 +629,12 @@ virDomainDiskDefParseXML(virConnectPtr c
                        (xmlStrEqual(cur->name, BAD_CAST "driver"))) {
                 driverName = virXMLPropString(cur, "name");
                 driverType = virXMLPropString(cur, "type");
+                cachetag = virXMLPropString(cur, "cache");
+                if (cachetag) {
+                    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")) {
@@ -2770,14 +2789,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]