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

Re: [libvirt] [PATCH 2/2] qemu: add separate rerror_policy for disk errors



On 10/04/2011 12:35 PM, Laine Stump wrote:
libvirt's XML only had a single attribute, error_policy to control
both read and write error policy, but qemu has separate settings for
these. In one case (enospc) a policy is allowed for write errors but
not read errors.

This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
---
  docs/formatdomain.html.in |   16 +++++++++++++---

Missing the patch to docs/schemas/domaincommon.rng

Also, I'd like to see at least one addition to tests/qemuxml2argvdata that exposes the new command line.

-<span class="since">Since 0.8.0</span>
+            how the hypervisor will behave on a disk read or write
+            error, possible values are "stop", "ignore", and
+            "enospace".<span class="since">Since 0.8.0</span>  There is
+            also an optional<code>rerror_policy</code>  that controls
+            behavior for read errors only.<span class="since">Since
+            0.9.7</space>. If no rerror_policy is given, error_policy
+            is used for both read and write errors. If rerror_policy
+            is given, it overrides the<code>error_policy</code>  for
+            read errors. Also note that "enospace" is not a valid
+            policy for read errors, so if<code>error_policy</code>  is
+            set to "enospace" and no<code>rerror_policy</code>  is
+            given, the read error policy will be automatically set to
+            "ignore".

I think we also need to document that "default" can be used for either (or both) [r]error_policy as a way to rely on the hypervisor defaults. More on this below.

@@ -2560,6 +2562,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
          goto error;
      }

+    if (rerror_policy&&
+        (((def->rerror_policy
+           = virDomainDiskErrorPolicyTypeFromString(error_policy))<  0) ||

virDomainDiskErrorPolicyTypeFromString converts "default" to 0, which means this is an undocumented value that we parse. In many existing cases, we use vir...TypeFromString() <= 0 to reject parsing "default". But here, you copied-and-pasted from error_policy, which also was using < 0 (and silently accepting "default"). Now, as to _why_ it makes sense to allow default, consider the case where I want my qemu command line to have one, but not both, policies. How do I specify that in the XML? By doing:

rerror_policy='stop'

I get my desired:

rerror=stop

But what about the converse, for just werror on the command line?  Using

error_policy='stop'

produces

rerror=stop,werror=stop

which isn't quite what I wanted.  So I need:

rerror_policy='default' error_policy='stop'

to get exactly:

werror=stop

on the command line, which argues that "default" should be part of the documented XML for rerror_policy. And symmetry argues that we might as well then document "default" for error_policy.

Hence, I think the following rng change covers things:

diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index cffaac2..fffee9a 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -853,13 +853,25 @@
     </attribute>
   </define>
   <define name="driverErrorPolicy">
-    <attribute name="error_policy">
-      <choice>
-        <value>stop</value>
-        <value>ignore</value>
-        <value>enospace</value>
-      </choice>
-    </attribute>
+    <optional>
+      <attribute name="error_policy">
+        <choice>
+          <value>default</value>
+          <value>stop</value>
+          <value>ignore</value>
+          <value>enospace</value>
+        </choice>
+      </attribute>
+    </optional>
+    <optional>
+      <attribute name="rerror_policy">
+        <choice>
+          <value>default</value>
+          <value>stop</value>
+          <value>ignore</value>
+        </choice>
+      </attribute>
+    </optional>
   </define>
   <define name="driverIO">
     <attribute name="io">

@@ -9177,6 +9191,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
              virBufferAsprintf(buf, " cache='%s'", cachemode);
          if (def->error_policy)
              virBufferAsprintf(buf, " error_policy='%s'", error_policy);
+        if (def->rerror_policy)
+            virBufferAsprintf(buf, " rerror_policy='%s'", rerror_policy);

Hmm, given my earlier arguments that documenting "default" makes sense, we would have to change the logic here to be that we always print both attributes, even if one of them is "default", if at least one of them is something besides default:

if (def->error_policy || def->rerror_policy) {
  virBufferAsprintf(buf, " error_policy='%s' rerror_policy='%s'",
                    error_policy, rerror_policy
}

+++ b/src/qemu/qemu_command.c
@@ -1696,6 +1696,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,

          if (disk->error_policy)
              wpolicy = virDomainDiskErrorPolicyTypeToString(disk->error_policy);
+        if (disk->rerror_policy)
+            rpolicy = virDomainDiskErrorPolicyTypeToString(disk->rerror_policy);
          if (!rpolicy)
              rpolicy = wpolicy;

@@ -1704,7 +1706,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
               * and it's only valid for werror, not for rerror.
               */
              wpolicy="enospc";
-            rpolicy="ignore";
+            if (!disk->rerror_policy)
+               rpolicy="ignore";

And if you take my suggestion for an explicit "default", then we can't quite key off of !disk->rerror_policy. Hmm, maybe I'm making this all more complicated than it has to be. ;(

An alternative would be adding "none" as a valid (non-zero) policy for both [r]error_policy, fix the parser to use <= 0 to reject "default", and fix the qemu_command to convert the enum value of NONE to omitting the string.

At any rate, I'll let you think about it a bit more for a v2 - we definitely need the rng improved and tests added, before we commit to anything, even if what we end up committing to doesn't expose "default" as an option to the user.

--
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


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