[libvirt] [PATCH v2] Add new attribute writeout to <filesystem> element

This introduces new attribute writeout 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 writeout is specified, meaning when writeout=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' writeout='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 docs/formatdomain.html.in | 9 ++++++++- docs/schemas/domaincommon.rng | 5 +++++ src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++-- 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, 70 insertions(+), 5 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 9cf0f12..93f754a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1303,7 +1303,7 @@ <source name='my-vm-template'/> <target dir='/'/> </filesystem> - <filesystem type='mount' accessmode='passthrough'> + <filesystem type='mount' accessmode='passthrough' writeout='immediate'> <driver type='path'/> <source dir='/export/to/guest'/> <target dir='/import/from/host'/> @@ -1379,6 +1379,13 @@ </dd> </dl> + The filesystem block has an optional attribute <code>writeout</code> with the only + supported value as <code>immediate</code>. It helps specify whether to skip the host page cache. + When writeout is specified, meaning when writeout=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. + </dd> <dt><code>source</code></dt> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 553a6f0..0b37f05 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1106,6 +1106,11 @@ <value>squash</value> </choice> </attribute> + <attribute name="writeout"> + <choice> + <value>immediate</value> + </choice> + </attribute> </optional> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 495ed33..a548b90 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(virDomainFSWriteout, VIR_DOMAIN_FS_WRITEOUT_LAST, + "default", + "immediate") VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "user", @@ -3297,6 +3300,7 @@ virDomainFSDefParseXML(xmlNodePtr node, char *source = NULL; char *target = NULL; char *accessmode = NULL; + char *writeout = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(); @@ -3325,6 +3329,17 @@ virDomainFSDefParseXML(xmlNodePtr node, def->accessmode = VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH; } + writeout = virXMLPropString(node, "writeout"); + if (writeout) { + if ((def->writeout = virDomainFSWriteoutTypeFromString(writeout)) < 0) { + virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown filesystem writeout '%s'"), writeout); + goto error; + } + } else { + def->writeout = VIR_DOMAIN_FS_WRITEOUT_DEFAULT; + } + cur = node->children; while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE) { @@ -3387,6 +3402,7 @@ cleanup: VIR_FREE(target); VIR_FREE(source); VIR_FREE(accessmode); + VIR_FREE(writeout); return def; @@ -10046,6 +10062,7 @@ virDomainFSDefFormat(virBufferPtr buf, const char *type = virDomainFSTypeToString(def->type); const char *accessmode = virDomainFSAccessModeTypeToString(def->accessmode); const char *fsdriver = virDomainFSDriverTypeTypeToString(def->fsdriver); + const char *writeout = virDomainFSWriteoutTypeToString(def->writeout); if (!type) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -10062,12 +10079,20 @@ virDomainFSDefFormat(virBufferPtr buf, if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_PATH || def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_DEFAULT) { virBufferAsprintf(buf, - " <filesystem type='%s' accessmode='%s'>\n", + " <filesystem type='%s' accessmode='%s'", type, accessmode); } else { virBufferAsprintf(buf, - " <filesystem type='%s'>\n", type); + " <filesystem type='%s'", type); } + + /* Don't generate anything if writeout is set to default */ + if (def->writeout) { + virBufferAsprintf(buf, " writeout='%s'", writeout); + } + + /* close the filesystem element */ + virBufferAddLit(buf,">\n"); if (def->fsdriver) { virBufferAsprintf(buf, " <driver type='%s'/>\n", fsdriver); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1f6e442..6f65b4a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -459,12 +459,21 @@ enum virDomainFSAccessMode { VIR_DOMAIN_FS_ACCESSMODE_LAST }; +/* Filesystem Writeout */ +enum virDomainFSWriteout { + VIR_DOMAIN_FS_WRITEOUT_DEFAULT = 0, + VIR_DOMAIN_FS_WRITEOUT_IMMEDIATE, + + VIR_DOMAIN_FS_WRITEOUT_LAST +}; + typedef struct _virDomainFSDef virDomainFSDef; typedef virDomainFSDef *virDomainFSDefPtr; struct _virDomainFSDef { int type; int fsdriver; int accessmode; + int writeout; 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(virDomainFSWriteout) 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 76f3632..adfc738 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(qemuDomainFSWriteout) +VIR_ENUM_IMPL(qemuDomainFSWriteout, VIR_DOMAIN_FS_WRITEOUT_LAST, + "default", + "immediate"); static void uname_normalize (struct utsname *ut) @@ -2084,6 +2088,7 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, { virBuffer opt = VIR_BUFFER_INITIALIZER; const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver); + const char *writeout = qemuDomainFSWriteoutTypeToString(fs->writeout); if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2108,6 +2113,15 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, virBufferAddLit(&opt, ",security_model=none"); } } + + if (fs->writeout != VIR_DOMAIN_FS_WRITEOUT_DEFAULT) { + if (qemuCapsGet(qemuCaps, QEMU_CAPS_FSDEV_WRITEOUT)) { + virBufferAsprintf(&opt, ",writeout=%s", writeout); + } 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 12/22/2011 03:10 AM, Deepak C Shetty wrote:
This introduces new attribute writeout 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 writeout is specified, meaning when writeout=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' writeout='immediate'> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem>
I still don't like the proposed XML. We need to get consensus on where to put it, with my proposal being: <filesystem type='mount'> <driver type='path' write='immediate'/> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/22/2011 03:20 PM, Eric Blake wrote:
Usage: <filesystem type='mount' accessmode='passthrough' writeout='immediate'> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem> I still don't like the proposed XML. We need to get consensus on where to put it, with my proposal being:
<filesystem type='mount'> <driver type='path' write='immediate'/> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem>
Right, other types of drivers might include a userspace NFS client, or some kind of "artificial" filesystem. In addition, instead of write='immediate' I suggest cache='writeback'/cache='writethrough'. Paolo

On Thu, Dec 22, 2011 at 05:25:49PM +0100, Paolo Bonzini wrote:
On 12/22/2011 03:20 PM, Eric Blake wrote:
Usage: <filesystem type='mount' accessmode='passthrough' writeout='immediate'> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem> I still don't like the proposed XML. We need to get consensus on where to put it, with my proposal being:
<filesystem type='mount'> <driver type='path' write='immediate'/> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem>
Right, other types of drivers might include a userspace NFS client, or some kind of "artificial" filesystem.
In addition, instead of write='immediate' I suggest cache='writeback'/cache='writethrough'.
Yeah I agree - we should follow the syntax we already use for caching with the <disk> element instead of inventing new syntax Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 12/22/2011 11:07 PM, Daniel P. Berrange wrote:
On Thu, Dec 22, 2011 at 05:25:49PM +0100, Paolo Bonzini wrote:
On 12/22/2011 03:20 PM, Eric Blake wrote:
Usage: <filesystem type='mount' accessmode='passthrough' writeout='immediate'> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem> I still don't like the proposed XML. We need to get consensus on where to put it, with my proposal being:
<filesystem type='mount'> <driver type='path' write='immediate'/> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem> Right, other types of drivers might include a userspace NFS client, or some kind of "artificial" filesystem.
In addition, instead of write='immediate' I suggest cache='writeback'/cache='writethrough'. Yeah I agree - we should follow the syntax we already use for caching with the<disk> element instead of inventing new syntax
Daniel Hi Dan and Paolo, There had been a discussion earlier on the qemu-devel mailing list on whether cache=writethrough is appropriate for 9pfs Vs writeout as the attribute name, and it was decided to use writeout as the writeout behaviour is not exactly what cache=writethrough means. Ccing Aneesh here who was part of the original qemu-devel discussion.
http://lists.gnu.org/archive/html/qemu-devel/2011-10/msg01274.html Aneesh, 2 things being discussed here from 9pfs perspective 1) Whether writeout should be an attribute at the <filesystem...> level or <driver..> level in the XML schema. 2) Have cache=writethrough in the XML instead of writeout=immediate. Comments pls.

Hi All, I posted v3 of this patch (considering the inputs from this thread) at ... https://www.redhat.com/archives/libvir-list/2012-January/msg00330.html Comments welcome. thanx, deepak

On 12/22/2011 07:50 PM, Eric Blake wrote:
On 12/22/2011 03:10 AM, Deepak C Shetty wrote:
I still don't like the proposed XML. We need to get consensus on where to put it, with my proposal being:
<filesystem type='mount'> <driver type='path' write='immediate'/> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem>
Currrently, the way its implemented in qemu's hw/9pfs is that the writeout is not driver specific. Its a global flag at the v9fs level i.e. V9FS_IMMEDIATE_WRITEOUT. 9pfs maps to 'mount' filesystem type in libvirt, hence i put it as part of the filesystem attribute.

On 12/22/2011 10:52 PM, Deepak C Shetty wrote:
On 12/22/2011 07:50 PM, Eric Blake wrote:
On 12/22/2011 03:10 AM, Deepak C Shetty wrote:
I still don't like the proposed XML. We need to get consensus on where to put it, with my proposal being:
<filesystem type='mount'> <driver type='path' write='immediate'/> <source dir='/export/to/guest'/> <target dir='mount_tag'/> </filesystem>
Currrently, the way its implemented in qemu's hw/9pfs is that the writeout is not driver specific. Its a global flag at the v9fs level i.e. V9FS_IMMEDIATE_WRITEOUT. 9pfs maps to 'mount' filesystem type in libvirt, hence i put it as part of the filesystem attribute.
Hi Eric, I saw your response to my v1 patch (should have seen before) and i think I am convinced that it makes more sense to have this as part of the <driver ... element. Regarding the attribute name, will it be more appropriate to have writeback='immediate' ? Once we have consensus, i will post v3 patch. Thanks - Deepak
participants (4)
-
Daniel P. Berrange
-
Deepak C Shetty
-
Eric Blake
-
Paolo Bonzini