[libvirt] [PATCH v3] Add new attribute wrpolicy to <driver> element
Eric Blake
eblake at redhat.com
Wed Jan 11 22:19:04 UTC 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> <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 at redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120111/f8d1de91/attachment-0001.sig>
More information about the libvir-list
mailing list