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

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(a)redhat.com&gt; > Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; > Reviewed-by: Masayoshi Mizuma <m.mizuma(a)jp.fujitsu.com&gt; > --- > 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 @@ > &lt;readonly/&gt; > &lt;/filesystem&gt; > &lt;filesystem type='mount' accessmode='passthrough'&gt; > - &lt;driver type='virtiofs'/&gt; > + &lt;driver type='virtiofs queue='1024'/&gt; > + &lt;binary path='/usr/libexec/virtiofsd' xattr='on'&gt; > + &lt;cache mode='always'/&gt; > + &lt;lock posix='on' flock='on'/&gt; > + &lt;/binary&gt; > &lt;source dir='/path'/&gt; > &lt;target dir='mount_tag'/&gt; > &lt;/filesystem&gt; > + Spurious whitespace?

> ... > &lt;/devices&gt; > ...</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.

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

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

> > switch (def->type) { > case VIR_DOMAIN_FS_TYPE_MOUNT: Reviewed-by: Peter Krempa <pkrempa(a)redhat.com&gt;