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

On 12/22/2011 12:55 PM, Osier Yang wrote: > On 2011年12月22日 14:54, 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> >> >> Currently this only works with type='mount' for the QEMU/KVM driver. >> >> Signed-off-by: Deepak C Shetty&lt;deepakcs(a)linux.vnet.ibm.com&gt; >> --- >> >> docs/formatdomain.html.in | 8 +++++++- >> docs/schemas/domaincommon.rng | 5 +++++ >> src/conf/domain_conf.c | 29 +++++++++++++++++++++++++++-- >> src/conf/domain_conf.h | 10 ++++++++++ >> src/qemu/qemu_command.c | 9 +++++++++ >> 5 files changed, 58 insertions(+), 3 deletions(-) >> >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index c57b7b3..803db8e 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -1303,7 +1303,7 @@ >> &lt;source name='my-vm-template'/&gt; >> &lt;target dir='/'/&gt; >> &lt;/filesystem&gt; >> -&lt;filesystem type='mount' accessmode='passthrough'&gt; >> +&lt;filesystem type='mount' accessmode='passthrough' >> writeout='immediate'&gt; >> &lt;driver type='path'/&gt; >> &lt;source dir='/export/to/guest'/&gt; >> &lt;target dir='/import/from/host'/&gt; >> @@ -1379,6 +1379,12 @@ >> </dd> >> </dl> >> >> + The filesystem block has an optional >> attribute<code>writeout='immediate'</code> > > <code>write</code>, and it's better to clarify it only supports > "immediate" currently. So you are proposing to change the attribute name to write instead of writeout ?

I wanted to keep it as writeout, since it matches with the qemu option also, will be easier to locate/debug, no ? Will make the changes in v2 and submit. > >> + to help 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 27ec1da..2298994 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 2c91f82..80b8eab 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -256,6 +256,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", >> @@ -3258,6 +3261,7 @@ virDomainFSDefParseXML(xmlNodePtr node, >> char *source = NULL; >> char *target = NULL; >> char *accessmode = NULL; >> + char *writeout = NULL; >> >> if (VIR_ALLOC(def)< 0) { >> virReportOOMError(); >> @@ -3286,6 +3290,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_INTERNAL_ERROR, > > IMHO VIR_ERR_CONFIG_UNSUPPORTED is more proper here. IIUC, i should return VIR_ERR_CONFIG_UNSUPPORTED if the xml has 'writeout' but QEMU_CAPS does not support writeout, correct ?

Will make the changes in v2 and submit. > >> + _("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) { >> @@ -3348,6 +3363,7 @@ cleanup: >> VIR_FREE(target); >> VIR_FREE(source); >> VIR_FREE(accessmode); >> + VIR_FREE(writeout); >> >> return def; >> >> @@ -10006,6 +10022,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, >> @@ -10022,12 +10039,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); >> } >> + >> + /* Dont generate anything if writeout is set to default */ > > Dont -> Don't thanks :) Will make the changes in v2 and submit. > >> + 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 3229a6f..bf4d79b 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h >> @@ -450,12 +450,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; >> @@ -1973,6 +1982,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_command.c b/src/qemu/qemu_command.c >> index 1f70eb1..ccfd092 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) >> @@ -1990,6 +1994,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", >> @@ -2014,6 +2019,10 @@ char *qemuBuildFSStr(virDomainFSDefPtr fs, >> virBufferAddLit(&opt, ",security_model=none"); >> } >> } >> + >> + if (fs->writeout != VIR_DOMAIN_FS_WRITEOUT_DEFAULT) { > > Since the "writeout" option is introduced far later than the the > "-fsdev". i.e. "writeout" can be not supported by qemu even if > "-fsdev" is supported. So we might want to detect the capability > first and report error earlier than starting the qemu process. > Like QEMU_CAPS_FSDEV_READONLY in commit a1a83c5. > So basically this would help in maintain correct error reporting when xml has writeout but user has older qemu which does not support writeout yet, correct ?

> Others looks good, (I just planned to add this support, you step > one more than me. :-) > 'guess what, we both probably are stepping on each other, I had earlier planned to do readonly fsdev support, saw ur post and moved to writeout support.

> Regards, > Osier > >