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

[Ján Tomko <jtomko@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > Reviewed-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> > Reviewed-by: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> > ---
> >  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?

Yes.

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

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@xxxxxxxxxx>

Attachment: signature.asc
Description: PGP signature