[libvirt PATCHv4 07/15] conf: add virtiofs-related elements and attributes

[Ján Tomko]

On Wed, Feb 26, 2020 at 08:23:43AM +0100, Peter Krempa wrote:
>On Thu, Feb 20, 2020 at 15:32:44 +0100, Ján Tomko wrote:
>> Add more elements for tuning the virtiofsd daemon
>> and the vhost-user-fs device:
>>
>>   <driver type='virtiofs' queue='1024' xattr='on'>
>>     <binary path='/usr/libexec/virtiofsd'>
>>       <cache mode='always'/>
>>       <lock posix='off' flock='off'/>
>>     </binary>
>>   </driver>
>>
>> Signed-off-by: Ján Tomko <jtomko at redhat.com>
>> Reviewed-by: Daniel P. Berrangé <berrange at redhat.com>
>> Reviewed-by: Masayoshi Mizuma <m.mizuma at jp.fujitsu.com>
>> ---
>>  docs/formatdomain.html.in                     |  25 +++-
>>  docs/schemas/domaincommon.rng                 |  48 ++++++++
>>  src/conf/domain_conf.c                        | 107 +++++++++++++++++-
>>  src/conf/domain_conf.h                        |  15 +++
>>  src/libvirt_private.syms                      |   1 +
>>  .../vhost-user-fs-fd-memory.xml               |   6 +-
>>  .../vhost-user-fs-hugepages.xml               |   1 +
>>  7 files changed, 200 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index dab8fb8f6b..7c4153c7ce 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -3936,10 +3936,15 @@
>>      <readonly/>
>>    </filesystem>
>>    <filesystem type='mount' accessmode='passthrough'>
>> -      <driver type='virtiofs'/>
>> +      <driver type='virtiofs queue='1024'/>
>> +      <binary path='/usr/libexec/virtiofsd' xattr='on'>
>> +         <cache mode='always'/>
>> +         <lock posix='on' flock='on'/>
>> +      </binary>
>>        <source dir='/path'/>
>>        <target dir='mount_tag'/>
>>    </filesystem>
>> +
>
>Spurious whitespace?
>

Yes.

>>    ...
>>  </devices>
>>  ...</pre>
>> @@ -4063,9 +4068,27 @@
>>            <a href="#elementsVirtio">Virtio-specific options</a> can also be
>>            set. (<span class="since">Since 3.5.0</span>)
>>            </li>
>> +          <li>
>> +            For <code>virtiofs</code>, the <code>queue</code> attribute can be used
>> +            to specify the queue size (i.e. how many requests can the queue fit).
>> +            (<span class="since">Since 6.1.0</span>)
>
>Version.
>
>> +          </li>
>>          </ul>
>>        </dd>
>>
>> +      <dt><code>binary</code></dt>
>> +      <dd>
>> +        The optional <code>binary</code> element can tune the options for virtiofsd.
>
>I think you should state that any of the following attributes/elements
>are optional, so that it's obvious that e.g. 'path' can be omitted if
>the user wishes to configure locking.
>

Done.

>> +        The attribute <code>path</code> can be used to override the path to the daemon.
>> +        Attribute <code>xattr</code> enables the use of filesystem extended attributes.
>> +        Caching can be tuned via the <code>cache</code> element, possible <code>mode</code>
>> +        values being <code>none</code> and <code>always</code>.
>> +        Locking can be controlled via the <code>lock</code>
>> +        element - attributes <code>posix</code> and <code>flock</code> both accepting
>> +        values <code>yes</code> or <code>no</code>.
>> +        (<span class="since">Since 6.1.0</span>)
>
>Version.
>
>> +      </dd>
>> +
>>        <dt><code>source</code></dt>
>>        <dd>
>>          The resource on the host that is being accessed in the guest. The
>> @@ -25165,6 +25235,8 @@ virDomainFSDefFormat(virBufferPtr buf,
>>      virBufferAddLit(buf, ">\n");
>>
>>      virBufferAdjustIndent(buf, 2);
>> +    virBufferAdjustIndent(&driverBuf, 2);
>> +    virBufferAdjustIndent(&binaryBuf, 2);
>
>Eww. Something is wrong if you need to tweak the indentation after using
>VIR_BUFFER_INIT_CHILD.
>

Yes, matches the pre-existing BufferAdjustIndent. I had to fight the
urge to clean up everything in this epopee.

>>      if (def->fsdriver) {
>>          virBufferAsprintf(&driverAttrBuf, " type='%s'", fsdriver);
>>
>> @@ -25176,11 +25248,44 @@ virDomainFSDefFormat(virBufferPtr buf,
>>          if (def->wrpolicy)
>>              virBufferAsprintf(&driverAttrBuf, " wrpolicy='%s'", wrpolicy);
>>
>> +        if (def->queue_size)
>> +            virBufferAsprintf(&driverAttrBuf, " queue='%llu'", def->queue_size);
>> +
>> +    }
>> +

[...]

>Surious newline.
>
>>      virDomainVirtioOptionsFormat(&driverAttrBuf, def->virtio);
>>
>> -    virXMLFormatElement(buf, "driver", &driverAttrBuf, NULL);
>> +    virXMLFormatElement(buf, "driver", &driverAttrBuf, &driverBuf);
>> +    virXMLFormatElement(buf, "binary", &binaryAttrBuf, &binaryBuf);
>> +    virXMLFormatElement(buf, "driver", &driverAttrBuf, &driverBuf);

>'driver' would be formatted twice if the first call of
>virXMLFormatElement wouldn't clear the arguments. Remove the latter one.
>

Probably a rebase artifact.

Jano

>>
>>      switch (def->type) {
>>      case VIR_DOMAIN_FS_TYPE_MOUNT:
>
>Reviewed-by: Peter Krempa <pkrempa at redhat.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200226/3b13ad5a/attachment-0001.sig>