On Thu, Jan 23, 2020 at 18:46:05 +0100, Ján Tomko wrote:
> Introduce a new 'virtiofs' driver type for filesystem.
>
> <filesystem type='mount' accessmode='passthrough'>
> <driver type='virtiofs'/>
> <source dir='/path'/>
> <target dir='mount_tag'>
> <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
> </filesystem>
>
> Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 7 ++
> docs/schemas/domaincommon.rng | 16 ++++
> src/conf/domain_conf.c | 1 +
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 4 +
> src/qemu/qemu_domain.c | 4 +
> src/qemu/qemu_domain_address.c | 4 +
> .../vhost-user-fs-fd-memory.xml | 38 ++++++++++
> .../vhost-user-fs-hugepages.xml | 73 +++++++++++++++++++
> .../vhost-user-fs-fd-memory.x86_64-latest.xml | 1 +
> .../vhost-user-fs-hugepages.x86_64-latest.xml | 1 +
> tests/qemuxml2xmltest.c | 3 +
> 12 files changed, 153 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml
> create mode 100644 tests/qemuxml2argvdata/vhost-user-fs-hugepages.xml
> create mode 120000 tests/qemuxml2xmloutdata/vhost-user-fs-fd-memory.x86_64-latest.xml
> create mode 120000 tests/qemuxml2xmloutdata/vhost-user-fs-hugepages.x86_64-latest.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 4db9c292b7..21f2a92ed6 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3921,6 +3921,11 @@
> <target dir='/import/from/host'/>
> <readonly/>
> </filesystem>
> + <filesystem type='mount' accessmode='passthrough'>
> + <driver type='virtiofs'/>
> + <source dir='/path'/>
> + <target dir='mount_tag'>
> + </filesystem>
> ...
> </devices>
> ...</pre>
> @@ -3949,6 +3954,8 @@
> while the value <code>immediate</code> means that a host writeback
> is immediately triggered for all pages touched during a guest file
> write operation <span class="since">(since 0.9.10)</span>.
> + <span class="since">Since 6.1.0</span>, <code>type='virtiofs'</code>
> + is also supported.
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.
> </dd>
> <dt><code>template</code></dt>
> <dd>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 04854bf816..66757d3b63 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2625,6 +2625,22 @@
> </optional>
> <ref name='virtioOptions'/>
> </group>
> + <group>
> + <attribute name="type">
> + <value>virtiofs</value>
> + </attribute>
> + <optional>
> + <attribute name="format">
> + <ref name="storageFormat"/>
> + </attribute>
The format attribute isn't documented and is rejected by the validator.
Why bother adding it to the schema?
> + </optional>
> + <optional>
> + <attribute name="wrpolicy">
> + <value>immediate</value>
> + </attribute>
This doesn't seem to be documented either.
> + </optional>
> + <ref name='virtioOptions'/>
> + </group>
> <empty/>
> </choice>
> </element>
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c66b60fd21..974c2b79af 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2687,6 +2687,10 @@ qemuBuildFilesystemCommandLine(virCommandPtr cmd,
> return -1;
> break;
>
> + case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
> + /* TODO: vhost-user-fs-pci */
> + break;
IMO in these cases we should still fall through to error reporting
rather than format nothing but you don't have to change it.
> +
> case VIR_DOMAIN_FS_DRIVER_TYPE_LOOP:
> case VIR_DOMAIN_FS_DRIVER_TYPE_NBD:
> case VIR_DOMAIN_FS_DRIVER_TYPE_PLOOP:
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 38addc7b61..eb8e82b545 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8275,6 +8275,10 @@ qemuDomainDeviceDefValidateFS(virDomainFSDefPtr fs,
> _("Filesystem driver type not supported"));
> return -1;
>
> + case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
> + /* TODO: vhost-user-fs-pci */
> + return 0;
> +
> case VIR_DOMAIN_FS_DRIVER_TYPE_LAST:
> default:
> virReportEnumRangeError(virDomainFSDriverType, fs->fsdriver);
> diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
> index 9e3bcc434d..3c6ac62ff5 100644
> --- a/src/qemu/qemu_domain_address.c
> +++ b/src/qemu/qemu_domain_address.c
> @@ -690,6 +690,10 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
> }
> break;
>
> + case VIR_DOMAIN_FS_DRIVER_TYPE_VIRTIOFS:
> + /* vhost-user-fs-pci */
> + return virtioFlags;
> +
> case VIR_DOMAIN_FS_DRIVER_TYPE_LOOP:
> case VIR_DOMAIN_FS_DRIVER_TYPE_NBD:
> case VIR_DOMAIN_FS_DRIVER_TYPE_PLOOP:
> diff --git a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml
> new file mode 100644
> index 0000000000..b02eb5cb2b
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml
> @@ -0,0 +1,38 @@
> +<domain type='kvm'>
> + <name>guest</name>
> + <uuid>126f2720-6f8e-45ab-a886-ec9277079a67</uuid>
> + <memory unit='KiB'>14680064</memory>
> + <currentMemory unit='KiB'>14680064</currentMemory>
> + <memoryBacking>
> + <source type='file'/>
> + <access mode='shared'/>
> + </memoryBacking>
> + <vcpu placement='static'>2</vcpu>
> + <os>
> + <type arch='x86_64' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <cpu>
> + <numa>
> + <cell id='0' cpus='0-1' memory='14680064' unit='KiB' memAccess='shared'/>
> + </numa>
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.
