Re: [PATCH 1/1] conf: qemu 9pfs: add 'multidevs' option

[Christian Schoenebeck]

On Donnerstag, 19. März 2020 14:10:26 CET Ján Tomko wrote:
> On a Tuesday in 2020, Christian Schoenebeck wrote:
> >Introduce new 'multidevs' option for filesystem.
> >
> >  <filesystem type='mount' accessmode='mapped' multidevs='remap'>
> 
> I don't like the 'multidevs' name, but cannot think of anything
> beter.
> 
> 'collisions' maybe?

Not sure if 'collisions' is better, e.g. collisions='remap' sounds scary. :)
And which collision would that be? At least IMO 'multidevs' is less ambigious.
I have no problem though to change it to whatever name you might come up with. 
Just keep the resulting key-value pair set in mind:

multidevs='default'
multidevs='remap'
multidevs='forbid'
multidevs='warn'

vs.

collisions='default'
collisions='remap' <- probably misleading what 'remap' means in this case
collisions='forbid'
collisions='warn' <- wrong, it warns about multiple devices, not about file ID 
collisions.

So different key-name might also require different value-names.

Another option would be the long form 'multi-devices=...'

> >    <source dir='/path'/>
> >    <target dir='mount_tag'>
> >  
> >  </filesystem>
> >
> >This option prevents misbheaviours on guest if a 9pfs export
> >contains multiple devices, due to the potential file ID collisions
> >this otherwise may cause.
> >
> >Signed-off-by: Christian Schoenebeck <address@hidden>
> >---
> >
> > docs/formatdomain.html.in     | 47 ++++++++++++++++++++++++++++++++++-
> > docs/schemas/domaincommon.rng | 10 ++++++++
> > src/conf/domain_conf.c        | 30 ++++++++++++++++++++++
> > src/conf/domain_conf.h        | 13 ++++++++++
> > src/qemu/qemu_command.c       |  7 ++++++
> > 5 files changed, 106 insertions(+), 1 deletion(-)
> 
> Please split the XML changes from the qemu driver changes.

Ok

> Also missing:
> * qemu_capabilities addition

AFAICS the common procedure is to add new capabilities always to the end of 
the enum list. So I guess I will do that as well.

> * qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
> reject this setting for virtiofs

Good to know where that check is supposed to go to, thanks!

> * qemuxml2xmltest addition
> * qemuxml2argvtest addition

Ok, I have to read up how those tests work. AFAICS I need to add xml files to 
their data subdirs.

Separate patches required for those 2 tests?

> (no changes required for virschematest - it checks all the XML files in
> the directories used by the above tests against the schema)
> 
> >diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> >index 594146009d..13c506988b 100644
> >--- a/docs/formatdomain.html.in
> >+++ b/docs/formatdomain.html.in
> >@@ -3967,7 +3967,7 @@
> >
> >     &lt;source name='my-vm-template'/&gt;
> >     &lt;target dir='/'/&gt;
> >   
> >   &lt;/filesystem&gt;
> >
> >-  &lt;filesystem type='mount' accessmode='passthrough'&gt;
> >+  &lt;filesystem type='mount' accessmode='passthrough'
> >multidevs='remap'&gt;>
> >     &lt;driver type='path' wrpolicy='immediate'/&gt;
> >     &lt;source dir='/export/to/guest'/&gt;
> >     &lt;target dir='/import/from/host'/&gt;
> >
> >@@ -4084,13 +4084,58 @@
> >
> >         </dd>
> >         </dl>
> >
> >+      <p>
> >
> >       <span class="since">Since 5.2.0</span>, the filesystem element
> >       has an optional attribute <code>model</code> with supported values
> >       "virtio-transitional", "virtio-non-transitional", or "virtio".
> >       See <a href="#elementsVirtioTransitional">Virtio transitional
> >       devices</a>
> >       for more details.
> >
> >+      </p>
> >+
> 
> Unrelated change that can be split out.

Ok, I'll make that the 1st preparatory patch then including ...

> >+      <p>
> >+      The filesystem element has an optional attribute
> ><code>multidevs</code> +      which specifies how to deal with a
> >filesystem export containing more than +      one device, in order to
> >avoid file ID collisions on guest when using 9pfs +      (<span
> >class="since">since 6.2.0, requires QEMU 4.2</span>). +      This
> >attribute is not available for virtiofs. The possible values are: +     
> ></p>
> >+
> >+        <dl>
> >+        <dt><code>default</code></dt>
> >+        <dd>
> >+        Use QEMU's default setting (which currently is <code>warn</code>).
> >+        </dd>
> >+        <dt><code>remap</code></dt>
> >+        <dd>
> >+        This setting allows guest to access multiple devices per export
> >without +        encountering misbehaviours. Inode numbers from host are
> >automatically +        remapped on guest to actively prevent file ID
> >collisions if guest +        accesses one export containing multiple
> >devices.
> >+        </dd>
> >+        <dt><code>forbid</code></dt>
> >+        <dd>
> >+        Only allow to access one device per export by guest. Attempts to
> >access +        additional devices on the same export will cause the
> >individual +        filesystem access by guest to fail with an error and
> >being logged (once) +        as error on host side.
> >+        </dd>
> >+        <dt><code>warn</code></dt>
> >+        <dd>
> >+        This setting resembles the behaviour of 9pfs prior to QEMU 4.2,
> >that is +        no action is performed to prevent any potential file ID
> >collisions if an +        export contains multiple devices, with the only
> >exception: a warning is +        logged (once) on host side now. This
> >setting may lead to misbehaviours +        on guest side if more than one
> >device is exported per export, due to the +        potential file ID
> >collisions this may cause on guest side in that case. +        </dd>
> >+        </dl>
> >+
> >
> >       </dd>
> >
> >+      <p>
> >+      The <code>filesystem</code> element may contain the following
> >subelements: +      </p>
> >+
> 
> And so can this one.

... this one.

> >       <dt><code>driver</code></dt>
> >       <dd>
> >       
> >         The optional driver element allows specifying further details
> >
> >@@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf,
> >
> >         virBufferAsprintf(buf, " model='%s'",
> >         
> >                           virDomainFSModelTypeToString(def->model));
> >     
> >     }
> >
> >+    if (def->multidevs) {
> >+        virBufferAsprintf(buf, " multidevs='%s'", multidevs);
> >+    }
> 
> make syntax-check complains here:
> Curly brackets around single-line body:
> ../src/conf/domain_conf.c:25452-25454:
>      if (def->multidevs) {
>          virBufferAsprintf(buf, " multidevs='%s'", multidevs);
>      }
> 
> Jano

Sorry for that, I assumed same code style as qemu. I'll do the automated 
syntax checks from now on.

Best regards,
Christian Schoenebeck