On Thu, Jan 23, 2020 at 18:46:05 +0100, Ján Tomko wrote: I'd expect some more description on the topic of virtiofs. Not even the knowledge-base article is crosslinked from here. Specifically what does 'mount_tag' mean and perhaps how to use it in the guest. The validator then forces accessmode to passthrough and that is not mentioned here either. The format attribute isn't documented and is rejected by the validator. Why bother adding it to the schema? This doesn't seem to be documented either. [...] IMO in these cases we should still fall through to error reporting rather than format nothing but you don't have to change it. This is possibly worth mentioning too. Especially if you don't cross-link to the knowledge-base in patch 10. The rest looks good. I'm willing to review just the improved documentation if you reply with the changes.

Add more elements for tuning the virtiofsd daemon and the vhost-user-fs device: <driver type='virtiofs' queue='1024' xattr='on'> <binary>/usr/libexec/virtiofsd</binary> <cache mode='always' size='2097152' unit='KiB'/> <lock posix='off' flock='off'/> </driver> Signed-off-by: Ján Tomko <jtomko(a)redhat.com&gt; --- docs/formatdomain.html.in | 18 ++- docs/schemas/domaincommon.rng | 51 ++++++ src/conf/domain_conf.c | 146 +++++++++++++++++- src/conf/domain_conf.h | 16 ++ src/libvirt_private.syms | 1 + .../vhost-user-fs-fd-memory.xml | 6 +- .../vhost-user-fs-hugepages.xml | 4 +- 7 files changed, 238 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 21f2a92ed6..cfdb1a7a9a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3922,10 +3922,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' xattr='on''&gt; + &lt;binary&gt;/usr/libexec/virtiofsd&lt;/binary&gt; + &lt;cache mode='always' size='2097152' unit='KiB'/&gt; + &lt;lock posix='off' flock='off'/&gt; + &lt;/driver&gt; &lt;source dir='/path'/&gt; &lt;target dir='mount_tag'&gt; &lt;/filesystem&gt; + ... &lt;/devices&gt; ...</pre> @@ -4046,6 +4051,17 @@ <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, <code>xattr</code> enables the use of filesystem + extended attributes. The <code>binary</code> sub-element contains the path + to the virtiofs daemon. + 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>) + </li> </ul> </dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 66757d3b63..a67c56d1c7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2629,6 +2629,57 @@ <attribute name="type"> <value>virtiofs</value> </attribute> + <optional> + <attribute name="queue"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="xattr"> + <ref name="virOnOff"/> + </attribute> + </optional> + <optional> + <element name="binary"> + <ref name="absFilePath"/> + </element> + </optional> + <optional> + <element name="cache"> + <optional> + <attribute name="size"> + <ref name="unsignedLong"/> + </attribute> + <optional> + <attribute name="unit"> + <ref name="unit"/> + </attribute> + </optional> + </optional> + <optional> + <attribute name="mode"> + <choice> + <value>none</value> + <value>always</value> + </choice> + </attribute> + </optional> + </element> + </optional> + <optional> + <element name="lock"> + <optional> + <attribute name="posix"> + <ref name="virOnOff"/> + </attribute> + </optional> + <optional> + <attribute name="flock"> + <ref name="virOnOff"/> + </attribute> + </optional> + </element> + </optional> <optional> <attribute name="format"> <ref name="storageFormat"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 94e1fd64b5..e4f20bb0bf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -501,6 +501,14 @@ VIR_ENUM_IMPL(virDomainFSModel, "virtio-non-transitional", ); +VIR_ENUM_IMPL(virDomainFSCacheMode, + VIR_DOMAIN_FS_CACHE_MODE_LAST, + "default", + "none", + "always", +); + + VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST, "user", @@ -2314,6 +2322,7 @@ void virDomainFSDefFree(virDomainFSDefPtr def) VIR_FREE(def->dst); virDomainDeviceInfoClear(&def->info); VIR_FREE(def->virtio); + VIR_FREE(def->binary); VIR_FREE(def); } @@ -11116,6 +11125,14 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, g_autofree char *usage = NULL; g_autofree char *units = NULL; g_autofree char *model = NULL; + g_autofree char *binary = NULL; + g_autofree char *queue_size = NULL; + g_autofree char *xattr = NULL; + g_autofree char *cache = NULL; + g_autofree char *cache_size = NULL; + g_autofree char *cache_unit = NULL; + g_autofree char *posix_lock = NULL; + g_autofree char *flock = NULL; ctxt->node = node; @@ -11193,12 +11210,39 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, } else if (virXMLNodeNameEqual(cur, "readonly")) { def->readonly = true; } else if (virXMLNodeNameEqual(cur, "driver")) { + xmlNodePtr child; + if (!fsdriver) fsdriver = virXMLPropString(cur, "type"); if (!wrpolicy) wrpolicy = virXMLPropString(cur, "wrpolicy"); if (!format) format = virXMLPropString(cur, "format"); + if (!queue_size) + queue_size = virXMLPropString(cur, "queue"); + if (!xattr) + xattr = virXMLPropString(cur, "xattr"); + + child = cur->children; + while (child != NULL) { + if (virXMLNodeNameEqual(child, "cache")) { + if (!cache) + cache = virXMLPropString(child, "mode"); + if (!cache_size) + cache_size = virXMLPropString(child, "size"); + if (!cache_unit) + cache_unit = virXMLPropString(child, "unit"); + } else if (virXMLNodeNameEqual(child, "lock")) { + if (!posix_lock) + posix_lock = virXMLPropString(child, "posix"); + if (!flock) + flock = virXMLPropString(child, "flock"); + } else if (virXMLNodeNameEqual(child, "binary")) { + if (!binary) + binary = virXMLNodeContentString(child); + } + child = child->next; + } if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0) goto error; @@ -11208,11 +11252,72 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt, } if (fsdriver) { + int val; + if ((def->fsdriver = virDomainFSDriverTypeFromString(fsdriver)) <= 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unknown fs driver type '%s'"), fsdriver); goto error; } + + if (binary) + def->binary = virFileSanitizePath(binary); + + 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"), + cache_size); + goto error; + } + + if (xattr) { + if ((val = virTristateSwitchTypeFromString(xattr)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown xattr value '%s'"), xattr); + goto error; + } + def->xattr = val; + } + + if (cache) { + if ((val = virDomainFSCacheModeTypeFromString(cache)) <= 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse cache mode '%s' for virtiofs"), + cache); + goto error; + } + def->cache = val; + } + + if (cache_size && virStrToLong_ull(cache_size, NULL, 10, &def->cache_size) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("cannot parse cache size '%s' for virtiofs"), + cache_size); + goto error; + } + + if (virScaleInteger(&def->cache_size, cache_unit, + 1024, ULLONG_MAX) < 0) + goto error; + def->cache_size = VIR_DIV_UP(def->cache_size, 1024); + + if (posix_lock) { + if ((val = virTristateSwitchTypeFromString(posix_lock)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown posix lock value '%s'"), posix_lock); + goto error; + } + def->posix_lock = val; + } + + if (flock) { + if ((val = virTristateSwitchTypeFromString(flock)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown flock value '%s'"), flock); + goto error; + } + def->flock = val; + } } if (format) { @@ -24998,6 +25103,7 @@ 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); if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -25021,7 +25127,11 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); + virBufferAdjustIndent(&driverBuf, 2); if (def->fsdriver) { + g_auto(virBuffer) lockAttrBuf = VIR_BUFFER_INITIALIZER; + g_auto(virBuffer) cacheAttrBuf = VIR_BUFFER_INITIALIZER; + virBufferAsprintf(&driverAttrBuf, " type='%s'", fsdriver); if (def->format) @@ -25032,11 +25142,45 @@ virDomainFSDefFormat(virBufferPtr buf, if (def->wrpolicy) virBufferAsprintf(&driverAttrBuf, " wrpolicy='%s'", wrpolicy); + if (def->queue_size) + virBufferAsprintf(&driverAttrBuf, " queue='%llu'", def->queue_size); + + if (def->xattr) { + virBufferAsprintf(&driverAttrBuf, " xattr='%s'", + virTristateSwitchTypeToString(def->xattr)); + } + + virBufferEscapeString(&driverBuf, "<binary>%s</binary>\n", def->binary); + + if (def->cache) { + virBufferAsprintf(&cacheAttrBuf, " mode='%s'", + virDomainFSCacheModeTypeToString(def->cache)); + } + + if (def->cache_size) { + virBufferAsprintf(&cacheAttrBuf, " size='%llu' unit='KiB'", + def->cache_size); + } + + virXMLFormatElement(&driverBuf, "cache", &cacheAttrBuf, NULL); + + if (def->posix_lock) { + virBufferAsprintf(&lockAttrBuf, " posix='%s'", + virTristateSwitchTypeToString(def->posix_lock)); + } + + if (def->flock) { + virBufferAsprintf(&lockAttrBuf, " flock='%s'", + virTristateSwitchTypeToString(def->flock)); + } + + virXMLFormatElement(&driverBuf, "lock", &lockAttrBuf, NULL); } + virDomainVirtioOptionsFormat(&driverAttrBuf, def->virtio); - virXMLFormatElement(buf, "driver", &driverAttrBuf, NULL); + virXMLFormatElement(buf, "driver", &driverAttrBuf, &driverBuf); switch (def->type) { case VIR_DOMAIN_FS_TYPE_MOUNT: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6ee95ac653..b90ccbdba8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -802,6 +802,14 @@ typedef enum { VIR_DOMAIN_FS_MODEL_LAST } virDomainFSModel; +typedef enum { + VIR_DOMAIN_FS_CACHE_MODE_DEFAULT = 0, + VIR_DOMAIN_FS_CACHE_MODE_NONE, + VIR_DOMAIN_FS_CACHE_MODE_ALWAYS, + + VIR_DOMAIN_FS_CACHE_MODE_LAST +} virDomainFSCacheMode; + struct _virDomainFSDef { int type; int fsdriver; /* enum virDomainFSDriverType */ @@ -817,6 +825,13 @@ struct _virDomainFSDef { unsigned long long space_hard_limit; /* in bytes */ unsigned long long space_soft_limit; /* in bytes */ bool symlinksResolved; + char *binary; + unsigned long long queue_size; + virTristateSwitch xattr; + virDomainFSCacheMode cache; + unsigned long long cache_size; + virTristateSwitch posix_lock; + virTristateSwitch flock; virDomainVirtioOptionsPtr virtio; virObjectPtr privateData; }; @@ -3425,6 +3440,7 @@ VIR_ENUM_DECL(virDomainFSDriver); VIR_ENUM_DECL(virDomainFSAccessMode); VIR_ENUM_DECL(virDomainFSWrpolicy); VIR_ENUM_DECL(virDomainFSModel); +VIR_ENUM_DECL(virDomainFSCacheMode); VIR_ENUM_DECL(virDomainNet); VIR_ENUM_DECL(virDomainNetBackend); VIR_ENUM_DECL(virDomainNetVirtioTxMode); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 907640b684..75f145a9de 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -389,6 +389,7 @@ virDomainDiskSourceFormat; virDomainDiskTranslateSourcePool; virDomainFeatureTypeFromString; virDomainFeatureTypeToString; +virDomainFSCacheModeTypeToString; virDomainFSDefFree; virDomainFSDefNew; virDomainFSDriverTypeToString; diff --git a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml index b02eb5cb2b..3a578ae126 100644 --- a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml +++ b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml @@ -26,7 +26,11 @@ <controller type='usb' index='0' model='none'/> <controller type='pci' index='0' model='pci-root'/> <filesystem type='mount' accessmode='passthrough'> - <driver type='virtiofs'/> + <driver type='virtiofs' queue='1024' xattr='on'> + <binary>/usr/libexec/virtiofsd</binary> + <cache mode='always' size='2097152' unit='KiB'/> + <lock posix='off' flock='off'/> + </driver> <source dir='/path'/> <target dir='mount_tag'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> diff --git a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml index 8a4b1c7c0b..fb9c4cc6c3 100644 --- a/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml +++ b/tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml @@ -61,7 +61,9 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x3'/> </controller> <filesystem type='mount' accessmode='passthrough'> - <driver type='virtiofs'/> + <driver type='virtiofs'> + <binary>/usr/libexec/virtiofsd</binary> + </driver> <source dir='/path'/> <target dir='mount_tag'/> <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> -- 2.21.0

On Thu, Jan 23, 2020 at 18:46:06 +0100, Ján Tomko wrote: So while looking at the rest of the series, I've noticed that the locking options are actually for the daemon and not for the device, while caching is passed to the device. This means that we mix semantically different options here and additionally with a possible rewrite of the daemon the daemon-based options may become obsolete. I'd suggest you add them into a container along with a tag of which version or name of virtiofsd is used.

On Thu, Jan 23, 2020 at 18:46:07 +0100, Ján Tomko wrote: IMO the option should be "virtiofsd_debug" so that it's really clear that it's meant for the daemon. In case of gluster which you put it close to the option is actually for the client. I'd make it more obvious that this actually enables debugging of the daemon.

On Thu, Jan 23, 2020 at 18:46:08 +0100, Ján Tomko wrote: "missing virtiofs binary path" perhaps? Also the schema documents that the path is optional and the documentation never mentions that the path must be provided. I also didn't find any filling of a default, thus one of these is wrong. The schema should also reject this. Except for the issue with the path to the virtiofs binary this patch looks okay.

On Thu, Jan 23, 2020 at 18:46:09 +0100, Ján Tomko wrote: [...] 'obj' is no longer unused. [...] [...] Not required but I'd be grateful if you get rid of the pointless 'ret' variable in this function. Context in this patch made me question how that actually works. Okay, hot(un)plug of VIR_DOMAIN_DEVICE_FS devices is not supported. [...] I'm not entirely sure how the virtiofsd logs, but logging into the syslog seems to be suboptimal. I think we should log into a fd/file same way as qemu does. Specifically I'm worried about the possibility to identify/match the daemon to the corresponding VM. The binary itself does not have any identifying data of the VM name and such so it might be actually hard to match up which daemon caused which log entry. Specifically since pid numbers are considered internal implementation in libvirt. No ternaries please. Also properly expand the conditions as I've commented in the XML patch. Is it possible to use virCommandSetErrorBuffer instead? That would solve all of the reading and pipes. nit: exitstatus might not been updated thus reporting 0 might be confusing. Please heap-allocate 'errbuf' here. Is there any caching on the daemon side which could be thrown away on an unclean shutdown? [...] I don't think you should do this for non-virtiofs cases.

On Thu, Jan 23, 2020 at 18:46:10 +0100, Ján Tomko wrote: Rather than posting this bugzilla link to a not very descriptive bug it would be worthwhile to describe what the device actually does and how it integrates with the daemon. I don't like to see another alias generated in the command line generator and then thrown away. Unfortunately it's still a common practice. fs->dst is user-provided and not validated. It must be escaped on the command line. Hmm, so only cache size and queue is relevant for the device itself. I think the options in the XML should reflect that. I'll mention this in the XML schema patch.

On Thu, Jan 23, 2020 at 18:46:11 +0100, Ján Tomko wrote: The XML documentation added to formatdomain.html is really lacking. At the very least this document should be linked from there. In that case it should perhaps also have a different attribute.