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 <qemu_oss@xxxxxxxxxxxxx>
> >---
> >
> > 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 @@
> >
> > <source name='my-vm-template'/>
> > <target dir='/'/>
> >
> > </filesystem>
> >
> >- <filesystem type='mount' accessmode='passthrough'>
> >+ <filesystem type='mount' accessmode='passthrough'
> >multidevs='remap'>>
> > <driver type='path' wrpolicy='immediate'/>
> > <source dir='/export/to/guest'/>
> > <target dir='/import/from/host'/>
> >
> >@@ -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
