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

Thu Dec 22 08:36:54 UTC 2011

On 2011年12月22日 16:01, Deepak C Shetty wrote:
> 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<deepakcs at linux.vnet.ibm.com>
>>> ---
>>>
>>> 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 @@
>>> <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,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 ?

Sorry, I meant <code>writeout</code>, it shouldn't mix attribute
and its value together when saying "an optional attribute".

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

Yes

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

Yes.

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

lol. :-)

>
>> Regards,
>> Osier
>>
>>
>