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

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



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> <span class="since">(since
> -        0.9.7)</span>.
> +        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
> +        </span>.

This sentence should not be part of the <span>; rather, you should have
just a <span> 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)</span>. 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
<span class="since">(since 0.9.10)</span>.

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 <span class="since">(since 0.9.10,
wrpolicy='foo' since 1.0)</span>.

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

Attachment: signature.asc
Description: OpenPGP digital signature


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