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 @@
> <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?
> ...
> </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.
> + 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
[...]
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d78ea92ead..8e7400294b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
[...]
> @@ -11311,6 +11320,64 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
> }
> }
>
> + if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) {
> + g_autofree char *queue_size = virXPathString("string(./driver/@queue)", ctxt);
> + g_autofree char *binary = virXPathString("string(./binary/@path)", ctxt);
> + g_autofree char *xattr = virXPathString("string(./binary/@xattr)", ctxt);
> + g_autofree char *cache = virXPathString("string(./binary/cache/@mode)", ctxt);
> + g_autofree char *posix_lock = virXPathString("string(./binary/lock/@posix)", ctxt);
> + g_autofree char *flock = virXPathString("string(./binary/lock/@flock)", ctxt);
> + int val;
> +
> +
Too many newlines.
> + if (queue_size && virStrToLong_ull(queue_size, NULL, 10, &def->queue_size) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("cannot parse queue size '%s' for virtiofs"),
> + queue_size);
> + goto error;
[...]
> @@ -25142,6 +25209,9 @@ virDomainFSDefFormat(virBufferPtr buf,
> const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy);
> const char *src = def->src->path;
> g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER;
> + g_auto(virBuffer) driverBuf = VIR_BUFFER_INIT_CHILD(buf);
> + g_auto(virBuffer) binaryAttrBuf = VIR_BUFFER_INITIALIZER;
> + g_auto(virBuffer) binaryBuf = VIR_BUFFER_INIT_CHILD(buf);
>
> if (!type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -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);
> +
> + }
> +
> + if (def->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS) {
> + g_auto(virBuffer) lockAttrBuf = VIR_BUFFER_INITIALIZER;
> + virBufferEscapeString(&binaryAttrBuf, " path='%s'", def->binary);
> +
> + if (def->xattr != VIR_TRISTATE_SWITCH_ABSENT) {
> + virBufferAsprintf(&binaryAttrBuf, " xattr='%s'",
> + virTristateSwitchTypeToString(def->xattr));
> + }
> +
> + if (def->cache != VIR_DOMAIN_FS_CACHE_MODE_DEFAULT) {
> + virBufferAsprintf(&binaryBuf, "<cache mode='%s'/>\n",
> + virDomainFSCacheModeTypeToString(def->cache));
> + }
> +
> + if (def->posix_lock != VIR_TRISTATE_SWITCH_ABSENT) {
> + virBufferAsprintf(&lockAttrBuf, " posix='%s'",
> + virTristateSwitchTypeToString(def->posix_lock));
> + }
> +
> + if (def->flock != VIR_TRISTATE_SWITCH_ABSENT) {
> + virBufferAsprintf(&lockAttrBuf, " flock='%s'",
> + virTristateSwitchTypeToString(def->flock));
> + }
> +
> + virXMLFormatElement(&binaryBuf, "lock", &lockAttrBuf, NULL);
> }
>
> +
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@xxxxxxxxxx>
