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

Deepak C Shetty deepakcs at linux.vnet.ibm.com
Thu Dec 22 08:01:10 UTC 2011


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




More information about the libvir-list mailing list