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

2020-03-19 Thread Ján Tomko 

On a Thursday in 2020, Christian Schoenebeck wrote:

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.
>
>  

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=...'


Okay, let's leave it at 'multidevs'.




>
>
>
>  
>
>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 
>---
>
> 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?


Usually xml2xmltest is in the same test as the XML parser/formatter
and xml2argvtest is a part of the qemu driver patch.

Jano


signature.asc
Description: PGP signature

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

2020-03-19 Thread Daniel P . Berrangé 
On Thu, Mar 19, 2020 at 04:57:41PM +0100, Christian Schoenebeck wrote:
> 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.
> > >
> > >  
> > 
> > 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=...'

I tried to come up with names when this was posted to QEMU, but didn't
think of much better than multidevs, so I think that's acceptable for
libvirt usage.

"collisions" isn't better enough to justify different naming from QEMU


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

2020-03-19 Thread 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.
> >
> >  
> 
> 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=...'

> >
> >
> >  
> >  
> >
> >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 
> >---
> >
> > 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 @@
> >
> > 
> > 
> >
> >+  
> >
> >   Since 5.2.0, the filesystem element
> >   has an optional attribute model with supported values
> >   "virtio-transitional", "virtio-non-transitional", or "virtio".
> >   See Virtio transitional
> >   devices
> >   for more details.
> >
> >+  
> >+
> 
> Unrelated change that can be split out.

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

> >+  
> >+  The filesystem element has an optional attribute
> >multidevs +  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 +  ( >class="since">since 6.2.0, requires QEMU 4.2). +  This
> >attribute is not available for virtiofs. The possible values are: + 
> >
> >+
> >+
> >+default
> >+
> >+Use QEMU's default setting (which currently is warn).
> >+
> >+remap
> >+
> >+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.
> >+
> >+forbid
> >+
> >+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.
> >+
> >+warn
> >+
> >+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

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

2020-03-19 Thread Ján Tomko 

On a Tuesday in 2020, Christian Schoenebeck wrote:

Introduce new 'multidevs' option for filesystem.

 


I don't like the 'multidevs' name, but cannot think of anything
beter.

'collisions' maybe?


   
   
 

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 
---
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.

Also missing:
* qemu_capabilities addition
* qemuDomainDeviceDefValidateFS in qemu_domain.c - check for the capability,
  reject this setting for virtiofs
* qemuxml2xmltest addition
* qemuxml2argvtest addition

(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 @@



+  
  Since 5.2.0, the filesystem element
  has an optional attribute model with supported values
  "virtio-transitional", "virtio-non-transitional", or "virtio".
  See Virtio transitional devices
  for more details.
+  
+


Unrelated change that can be split out.


+  
+  The filesystem element has an optional attribute multidevs
+  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
+  (since 6.2.0, requires QEMU 4.2).
+  This attribute is not available for virtiofs. The possible values are:
+  
+
+
+default
+
+Use QEMU's default setting (which currently is warn).
+
+remap
+
+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.
+
+forbid
+
+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.
+
+warn
+
+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.
+
+
+
  




+  
+  The filesystem element may contain the following 
subelements:
+  
+


And so can this one.


  driver
  
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


signature.asc
Description: PGP signature

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

2020-03-17 Thread Christian Schoenebeck 
Introduce new 'multidevs' option for 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 
---
 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(-)

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 @@
 
 
 
+  
   Since 5.2.0, the filesystem element
   has an optional attribute model with supported values
   "virtio-transitional", "virtio-non-transitional", or "virtio".
   See Virtio transitional devices
   for more details.
+  
+
+  
+  The filesystem element has an optional attribute multidevs
+  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
+  (since 6.2.0, requires QEMU 4.2).
+  This attribute is not available for virtiofs. The possible values are:
+  
+
+
+default
+
+Use QEMU's default setting (which currently is warn).
+
+remap
+
+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.
+
+forbid
+
+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.
+
+warn
+
+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.
+
+
+
   
 
+  
+  The filesystem element may contain the following 
subelements:
+  
+
   driver
   
 The optional driver element allows specifying further details
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 6805420451..9b37740e30 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2676,6 +2676,16 @@
 
   
 
+
+  
+
+  default
+  remap
+  forbid
+  warn
+
+  
+
 
   
 
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 71535f53f5..b96f87063a 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(virDomainFSMultidevs,
+  VIR_DOMAIN_FS_MULTIDEVS_LAST,
+  "default",
+  "remap",
+  "forbid",
+  "warn",
+);
+
 VIR_ENUM_IMPL(virDomainFSCacheMode,
   VIR_DOMAIN_FS_CACHE_MODE_LAST,
   "default",
@@ -11376,6 +11384,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
 g_autofree char *usage = NULL;
 g_autofree char *units = NULL;
 g_autofree char *model = NULL;
+g_autofree char *multidevs = NULL;
 
 ctxt->node = node;
 
@@ -11414,6 +11423,17 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
 }
 }
 
+multidevs = virXMLPropString(node, "multidevs");
+if (multidevs) {
+if ((def->multidevs = virDomainFSMultidevsTypeFromString(multidevs)) < 
0) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   _("unknown multidevs '%s'"), multidevs);
+goto error;
+}
+} else {
+def->multidevs = VIR_DOMAIN_FS_MULTIDEVS_DEFAULT;
+}
+
 if