| Message ID | E1jEFpL-00028n-Qj@lizzy.crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] conf: qemu 9pfs: add 'multidevs' option | expand |
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? > <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@crudebyte.com> >--- > 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 @@ > </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. >+ <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. > <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
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@crudebyte.com> > >--- > > > > 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
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. > > > > > > <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=...' 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
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. >> > >> > <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=...' Okay, let's leave it at 'multidevs'. > >> > <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@crudebyte.com> >> >--- >> > >> > 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
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> + + <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> + <dt><code>driver</code></dt> <dd> 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 @@ </choice> </attribute> </optional> + <optional> + <attribute name="multidevs"> + <choice> + <value>default</value> + <value>remap</value> + <value>forbid</value> + <value>warn</value> + </choice> + </attribute> + </optional> <optional> <element name='readonly'> <empty/> 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 (virDomainParseScaledValue("./space_hard_limit[1]", NULL, ctxt, &def->space_hard_limit, 1, ULLONG_MAX, false) < 0) @@ -25397,6 +25417,7 @@ virDomainFSDefFormat(virBufferPtr buf, const char *accessmode = virDomainFSAccessModeTypeToString(def->accessmode); const char *fsdriver = virDomainFSDriverTypeToString(def->fsdriver); const char *wrpolicy = virDomainFSWrpolicyTypeToString(def->wrpolicy); + const char *multidevs = virDomainFSMultidevsTypeToString(def->multidevs); const char *src = def->src->path; g_auto(virBuffer) driverAttrBuf = VIR_BUFFER_INITIALIZER; g_auto(virBuffer) driverBuf = VIR_BUFFER_INIT_CHILD(buf); @@ -25415,6 +25436,12 @@ virDomainFSDefFormat(virBufferPtr buf, return -1; } + if (!multidevs) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected multidevs %d"), def->multidevs); + return -1; + } + virBufferAsprintf(buf, "<filesystem type='%s' accessmode='%s'", type, accessmode); @@ -25422,6 +25449,9 @@ virDomainFSDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " model='%s'", virDomainFSModelTypeToString(def->model)); } + if (def->multidevs) { + virBufferAsprintf(buf, " multidevs='%s'", multidevs); + } virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 91b776c28a..23b7924781 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -796,6 +796,18 @@ typedef enum { VIR_DOMAIN_FS_WRPOLICY_LAST } virDomainFSWrpolicy; +/* How to handle exports containing multiple devices. */ +typedef enum { + VIR_DOMAIN_FS_MULTIDEVS_DEFAULT = 0, /* Use QEMU's default setting */ + VIR_DOMAIN_FS_MULTIDEVS_REMAP, /* Remap inodes from host to guest */ + VIR_DOMAIN_FS_MULTIDEVS_FORBID, /* Prohibit more than one device */ + VIR_DOMAIN_FS_MULTIDEVS_WARN, /* Just log a warning if multiple devices */ + + VIR_DOMAIN_FS_MULTIDEVS_LAST +} virDomainFSMultidevs; + +VIR_ENUM_DECL(virDomainFSMultidevs); + typedef enum { VIR_DOMAIN_FS_MODEL_DEFAULT = 0, VIR_DOMAIN_FS_MODEL_VIRTIO, @@ -820,6 +832,7 @@ struct _virDomainFSDef { int wrpolicy; /* enum virDomainFSWrpolicy */ int format; /* virStorageFileFormat */ int model; /* virDomainFSModel */ + int multidevs; /* virDomainFSMultidevs */ unsigned long long usage; /* in bytes */ virStorageSourcePtr src; char *dst; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9790c92cf8..7020e5448c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2632,6 +2632,13 @@ qemuBuildFSStr(virDomainFSDefPtr fs) } else if (fs->accessmode == VIR_DOMAIN_FS_ACCESSMODE_SQUASH) { virBufferAddLit(&opt, ",security_model=none"); } + if (fs->multidevs == VIR_DOMAIN_FS_MULTIDEVS_REMAP) { + virBufferAddLit(&opt, ",multidevs=remap"); + } else if (fs->multidevs == VIR_DOMAIN_FS_MULTIDEVS_FORBID) { + virBufferAddLit(&opt, ",multidevs=forbid"); + } else if (fs->multidevs == VIR_DOMAIN_FS_MULTIDEVS_WARN) { + virBufferAddLit(&opt, ",multidevs=warn"); + } } else if (fs->fsdriver == VIR_DOMAIN_FS_DRIVER_TYPE_HANDLE) { /* removed since qemu 4.0.0 see v3.1.0-29-g93aee84f57 */ virBufferAddLit(&opt, "handle");
Introduce new 'multidevs' option for filesystem. <filesystem type='mount' accessmode='mapped' multidevs='remap'> <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@crudebyte.com> --- 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(-)