[libvirt] [PATCH v3] Add new attribute wrpolicy to <driver> element
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. Signed-off-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com> --- v2: - added writeout as a qemu cap - cosmetic changes in comments - moved to using VIR_ERR_CONFIG_UNSUPPORTED - corrected doc v3: - changed attr name from writeout to wrpolicy - moved attr to be part of driver xml element docs/formatdomain.html.in | 12 ++++++++++-- docs/schemas/domaincommon.rng | 7 +++++++ src/conf/domain_conf.c | 27 ++++++++++++++++++++++++++- src/conf/domain_conf.h | 10 ++++++++++ src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 5 +++-- src/qemu/qemu_command.c | 14 ++++++++++++++ 7 files changed, 73 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 18b7e22..fb2bcbc 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1310,7 +1310,7 @@ <target dir='/'/> </filesystem> <filesystem type='mount' accessmode='passthrough'> - <driver type='path'/> + <driver type='path' wrpolicy='immediate'/> <source dir='/export/to/guest'/> <target dir='/import/from/host'/> <readonly/> @@ -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>. </dd> <dt><code>type='template'</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 353faea..810b634 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1142,6 +1142,13 @@ <value>handle</value> </choice> </attribute> + <optional> + <attribute name="wrpolicy"> + <choice> + <value>immediate</value> + </choice> + </attribute> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0190a81..98e5866 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -257,6 +257,9 @@ VIR_ENUM_IMPL(virDomainFSAccessMode, VIR_DOMAIN_FS_ACCESSMODE_LAST, "mapped", "squash") +VIR_ENUM_IMPL(virDomainFSWrpolicy, VIR_DOMAIN_FS_WRPOLICY_LAST, + "default", + "immediate") VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "user", @@ -3468,6 +3471,7 @@ virDomainFSDefParseXML(xmlNodePtr node, char *source = NULL; char *target = NULL; char *accessmode = NULL; + char *wrpolicy = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -3517,6 +3521,7 @@ virDomainFSDefParseXML(xmlNodePtr node, def->readonly = 1; } else if ((fsdriver == NULL) && (xmlStrEqual(cur->name, BAD_CAST "driver"))) { fsdriver = virXMLPropString(cur, "type"); + wrpolicy = virXMLPropString(cur, "wrpolicy"); } } cur = cur->next; @@ -3530,6 +3535,16 @@ virDomainFSDefParseXML(xmlNodePtr node, } } + if (wrpolicy) { + if ((def->wrpolicy = virDomainFSWrpolicyTypeFromString(wrpolicy)) < 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown filesystem write policy '%s'"), wrpolicy); + goto error; + } + } else { + def->wrpolicy = VIR_DOMAIN_FS_WRPOLICY_DEFAULT; + } + if (source == NULL) { virDomainReportError(VIR_ERR_NO_SOURCE, target ? "%s" : NULL, target); @@ -3556,6 +3571,7 @@ cleanup: VIR_FREE(target); VIR_FREE(source); VIR_FREE(accessmode); + VIR_FREE(wrpolicy); return def; @@ -10163,6 +10179,7 @@ virDomainFSDefFormat(virBufferPtr buf, const char *type = virDomainFSTypeToString(def->type); const char *accessmode = virDomainFSAccessModeTypeToString(def->accessmode); const char *fsdriver = virDomainFSDriverTypeTypeToString(def->fsdriver); + const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy); if (!type) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -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"); + } if (def->src) { diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 03aa5b6..7541fbc 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -479,12 +479,21 @@ enum virDomainFSAccessMode { VIR_DOMAIN_FS_ACCESSMODE_LAST }; +/* 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; char *src; char *dst; unsigned int readonly : 1; @@ -1974,6 +1983,7 @@ VIR_ENUM_DECL(virDomainControllerModelUSB) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainFSDriverType) VIR_ENUM_DECL(virDomainFSAccessMode) +VIR_ENUM_DECL(virDomainFSWrpolicy) VIR_ENUM_DECL(virDomainNet) VIR_ENUM_DECL(virDomainNetBackend) VIR_ENUM_DECL(virDomainNetVirtioTxMode) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 43c7578..2af16f3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -144,6 +144,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "ich9-ahci", "no-acpi", "fsdev-readonly", + "fsdev-writeout", ); struct qemu_feature_flags { @@ -1083,6 +1084,8 @@ qemuCapsComputeCmdFlags(const char *help, qemuCapsSet(flags, QEMU_CAPS_FSDEV); if (strstr(fsdev, "readonly")) qemuCapsSet(flags, QEMU_CAPS_FSDEV_READONLY); + if (strstr(fsdev, "writeout")) + qemuCapsSet(flags, QEMU_CAPS_FSDEV_WRITEOUT); } if (strstr(help, "-smbios type")) qemuCapsSet(flags, QEMU_CAPS_SMBIOS_TYPE); diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index c759baf..5b2f932 100644 --- a/src/qemu/qemu_capabilities.h +++ 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 */ - 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"); static void uname_normalize (struct utsname *ut) @@ -2086,6 +2090,7 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); + const char *wrpolicy = qemuDomainFSWrpolicyTypeToString(fs->wrpolicy); if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2119,6 +2124,15 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, } } + if (fs->wrpolicy != VIR_DOMAIN_FS_WRPOLICY_DEFAULT) { + if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV_WRITEOUT)) { + virBufferAsprintf(&opt, ",writeout=%s", wrpolicy); + } else { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("filesystem writeout not supported")); + } + } + virBufferAsprintf(&opt, ",id=%s%s", QEMU_FSDEV_HOST_PREFIX, fs->info.alias); virBufferAsprintf(&opt, ",path=%s", fs->src);
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
On 01/11/2012 03:19 PM, Eric Blake wrote:
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.
Also, I meant to add:
docs/formatdomain.html.in | 12 ++++++++++-- docs/schemas/domaincommon.rng | 7 +++++++ src/conf/domain_conf.c | 27 ++++++++++++++++++++++++++- src/conf/domain_conf.h | 10 ++++++++++ src/qemu/qemu_capabilities.c | 3 +++ src/qemu/qemu_capabilities.h | 5 +++-- src/qemu/qemu_command.c | 14 ++++++++++++++ 7 files changed, 73 insertions(+), 5 deletions(-)
Please add a new pair of test files to tests/qemuxml2argvdata (both .xml and .args file) to validate the RNG, as well as listing that file name in tests/qemuxml2argvtest.c to validate the qemu command line generation (I find it easiest to start from an existing similar .xml file, and just add in the new attribute). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Please add a new pair of test files to tests/qemuxml2argvdata (both .xml and .args file) to validate the RNG, as well as listing that file name in tests/qemuxml2argvtest.c to validate the qemu command line generation (I find it easiest to start from an existing similar .xml file, and just add in the new attribute).
I added wrpolicy testcase to tests/qemuxml2argvdata/qemuxml2argv-fs9p.args and tests/qemuxml2argvdata/qemuxml2argv-fs9p.xml but i see failure when running make check for the qemuxml2argv test case... how do i get more info on what is failing ? I assume that i need to have the libvirtd running on the system having the wrpolicy changes, correct ? Inside the .args file, it says /usr/bin/qemu, but my latest qemu is in my custom home folder, do i need to change .args file to pass the test ? thanx, deepak
Hello Eric, Posted v4 of this patch with your comments implemented at ... https://www.redhat.com/archives/libvir-list/2012-January/msg00633.html thanx, deepak
On 01/12/2012 03:49 AM, Eric Blake wrote:
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).
Yes, thats correct, thats how qemu works today for writeout.
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>.
Agreed.
+++ 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.
Agreed. will remove the <choice> pair
@@ -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).
Agreed.
+ 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.
I think leaving it there would make the code more understandable, i prefer to leave it there.
@@ -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 ','.
Will do.
+/* 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 */
Will do.
+++ 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.
Will undo the unrelated changes.
- 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).
Will do.
@@ -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.
Will do.
+ 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.
Thanks, will post v4 in some time.
participants (2)
-
Deepak C Shetty -
Eric Blake