Re: [libvirt PATCHv2 04/10] conf: qemu: add virtiofs fsdriver type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 @@
>      &lt;target dir='/import/from/host'/&gt;
>      &lt;readonly/&gt;
>    &lt;/filesystem&gt;
> +  &lt;filesystem type='mount' accessmode='passthrough'&gt;
> +      &lt;driver type='virtiofs'/&gt;
> +      &lt;source dir='/path'/&gt;
> +      &lt;target dir='mount_tag'&gt;
> +  &lt;/filesystem&gt;
>    ...
>  &lt;/devices&gt;
>  ...</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.





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux