Re: [libvirt] [PATCH v3] Add new attribute wrpolicy to <driver> element

11 Jan 2012

On 01/10/2012 07:29 AM, Deepak C Shetty wrote:
...
This introduces new attribute wrpolicy with only supported
value as immediate. This will be an optional
attribute with no defaults. This helps specify whether
to skip the host page cache.
When wrpolicy is specified, meaning when wrpolicy=immediate
a writeback is explicitly initiated for the dirty pages in
the host page cache as part of the guest file write operation.
Usage:
<filesystem type='mount' accessmode='passthrough'>
 <driver type='path' wrpolicy='immediate'/>
 <source dir='/export/to/guest'/>
 <target dir='mount_tag'/>
</filesystem>
Currently this only works with type='mount' for the QEMU/KVM driver.

...
@@ -1337,7 +1337,15 @@
 sub-element <code>driver</code>, with an
 attribute <code>type='path'</code>
 or <code>type='handle'</code> (since
- 0.9.7).
+ 0.9.7). The driver block has an optional attribute
+ <code>wrpolicy</code> with the only supported value
+ as <code>immediate</code>. It helps specify whether to skip 
+ the host page cache. When wrpolicy is specified, meaning when 
+ wrpolicy=immediate a writeback is explicitly initiated for the
+ dirty pages in the host page cache as part of the guest file 
+ write operation. When this attribute is not specified, there 
+ are no defaults, meaning explicit writeback won't be initiated
+ .
This sentence should not be part of the ; rather, you should have
just a at the end stating that wrpolicy is since 0.9.10.

I'm still trying to understand the qemu semantics: your choices are an
explicit writeout=immediate (with explicit requests to the host) or
nothing (host writes may be delayed). So, you are saying the a missing
attribute lets qemu choose, and an explicit attribute maps to the
explicit qemu values (where right now qemu only has one documented
explicit value). I guess that works, but I think this is a better
wording for those semantics:

[handle since] 0.9.7). The driver block has an optional attribute
<code>wrpolicy</code> that further controls interaction with the host
page cache; omitting the attribute gives default behavior, while the
value <code>immediate</code> means that a host writeback is immediately
triggered for all pages touched during a guest file write operation
(since 0.9.10).

Then, supposing we add a new policy 'foo' in the future, we could change
things to say:

The driver block has an optional attribute
<code>wrpolicy</code> that further controls interaction with the host
page cache; omitting the attribute gives default behavior, the value
<code>immediate</code> means that a host writeback is immediately
triggered for all pages touched during a guest file write operation, and
the value <code>foo</code> does xxyzy (since 0.9.10,
wrpolicy='foo' since 1.0).
...
+++ b/docs/schemas/domaincommon.rng
@@ -1142,6 +1142,13 @@
 <value>handle</value>
 </choice>
 </attribute>
+ <optional>
+ <attribute name="wrpolicy">
+ <choice>
+ <value>immediate</value>
+ </choice>
No need for a <choice> when there's only one possible value; then again,
leaving the <choice> in now means we won't have to reindent if a new
value is added later. Up to you if you want to shave two lines of the
patch.
...
@@ -3530,6 +3535,16 @@ virDomainFSDefParseXML(xmlNodePtr node,
 }
 }
+ if (wrpolicy) {
+ if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) < 0) {
This must be <= 0, not < 0 (that is, your RNG didn't allow
wrpolicy='default', so neither should the code).
...
+ virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown filesystem write policy '%s'"), wrpolicy);
+ goto error;
+ }
+ } else {
+ def->wrpolicy = VIR_DOMAIN_FS_WRPOLICY_DEFAULT;
This else clause is redundant: VIR_DOMAIN_FS_WRPOLICY_DEFAULT is 0, but
def was 0-initialized. But I guess it doesn't hurt to leave it in.
...
@@ -10182,7 +10199,15 @@ virDomainFSDefFormat(virBufferPtr buf,
 type, accessmode);
if (def->fsdriver) {
- virBufferAsprintf(buf, " <driver type='%s'/>\n", fsdriver);
+ virBufferAsprintf(buf, " <driver type='%s'", fsdriver);
+
+ /* Don't generate anything if wrpolicy is set to default */
+ if (def->wrpolicy) {
+ virBufferAsprintf(buf, " wrpolicy='%s'", wrpolicy);
+ }
+
+ virBufferAddLit(buf,"/>\n");
Style - space after ','.
...
+/* Filesystem Write policy */
+enum virDomainFSWrpolicy {
+ VIR_DOMAIN_FS_WRPOLICY_DEFAULT = 0,
+ VIR_DOMAIN_FS_WRPOLICY_IMMEDIATE,
+
+ VIR_DOMAIN_FS_WRPOLICY_LAST
+};
+
 typedef struct _virDomainFSDef virDomainFSDef;
 typedef virDomainFSDef *virDomainFSDefPtr;
 struct _virDomainFSDef {
 int type;
 int fsdriver;
 int accessmode;
+ int wrpolicy;
Lately, I've been annotating what values _should_ go in an int field
member, as in:

int wrpolicy; /* enum virDomainFSWrpolicy */
...
+++ b/src/qemu/qemu_capabilities.h
@@ -113,10 +113,11 @@ enum qemuCapsFlags {
 QEMU_CAPS_NO_SHUTDOWN = 74, /* usable -no-shutdown */
QEMU_CAPS_DRIVE_CACHE_UNSAFE = 75, /* Is cache=unsafe supported? */
- QEMU_CAPS_PCI_ROMBAR = 76, /* -device rombar=0|1 */
+ QEMU_CAPS_PCI_ROMBAR = 76, /* -device rombar=0|1 */
 QEMU_CAPS_ICH9_AHCI = 77, /* -device ich9-ahci */
 QEMU_CAPS_NO_ACPI		= 78, /* -no-acpi */
Why did you change spacing on QEMU_CAPS_PCI_ROMBAR, but not on
QEMU_CAPS_NO_ACPI? If you're going to make an unrelated change, at
least make all the code in that neighborhood consistent. It's almost
worth doing the whitespace changes in a separate patch, where we can
make the entire enum consistent.
...
- QEMU_CAPS_FSDEV_READONLY =79, /* -fsdev readonly supported */
+ QEMU_CAPS_FSDEV_READONLY = 79, /* -fsdev readonly supported */
+ QEMU_CAPS_FSDEV_WRITEOUT = 80, /* -fsdev writeout supported */
QEMU_CAPS_LAST, /* this must always be the last item */
 };

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 73f673f..ba0353e 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -108,6 +108,10 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST,
               "local",
               "handle");
+VIR_ENUM_DECL(qemuDomainFSWrpolicy)
+VIR_ENUM_IMPL(qemuDomainFSWrpolicy, VIR_DOMAIN_FS_WRPOLICY_LAST,
+              "default",
+              "immediate");
No need for this; rather, make sure libvirt_private.syms exports
virDomainFSWrpolicyTypeToString, and reuse that function.  (A
qemu-specific version is only needed if the mapping of the XML names to
the qemu attributes differs, but right now, our mapping of the single
element "immediate" is the same).
...
@@ -2119,6 +2124,15 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs,
         }
     }
+    if (fs->wrpolicy != VIR_DOMAIN_FS_WRPOLICY_DEFAULT) {
This can be simplified to:

if (fs->wrpolicy) {

since we already have the convention that VIR_DOMAIN_FS_WRPOLICY_DEFAULT
was explicitly chosen as 0.
...
+       if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV_WRITEOUT)) {
+           virBufferAsprintf(&opt, ",writeout=%s", wrpolicy);
+       } else {
+           qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                          _("filesystem writeout not supported"));
+       }
Missing a goto error.

I think your choice of <driver wrpolicy='immediate'/> is reasonable; you
may want to wait just a bit longer to see if any other opinions on
naming choices are presented, or you can go ahead and post a v4 that
incorporates fixes for my findings.  At any rate, I think we'll get some
form of this patch in for 0.9.10.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

    