This is the counter part patch against latest libvirt git master head to
support the 'vii' feature of patch 5, which introduces the XML config
XML tag "important" on libvirt side.

To stick with the previous example mentioned with patch 5, likewise
libvirt XML configuration might then look like this:

  <domain type='kvm'>
    ...
    <devices>
      ...
      <filesystem type='mount' accessmode='mapped'>
        <source dir='/vm/fs'/>
        <target dir='root'/>
        <important path='/var/shares'/>
        <important path='/tmp'/>
      </filesystem>
    </devices>
  </domain>

Like with the vii qemu virtfs command line argument, the order of the
"important" tag defines which one gets the highest inode namespace
(smallest generated suffix) on guest side.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 docs/schemas/domaincommon.rng |  6 ++++++
 src/conf/domain_conf.c        | 30 ++++++++++++++++++++++++++++++
 src/conf/domain_conf.h        |  3 +++
 src/qemu/qemu_command.c       | 10 ++++++++++
 4 files changed, 49 insertions(+)

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 111b85c36f..c75edfc4d3 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2515,6 +2515,12 @@
             </choice>
           </attribute>
         </optional>
+        <zeroOrMore>
+          <element name='important'>
+            <attribute name="path"/>
+            <empty/>
+          </element>
+        </zeroOrMore>
         <optional>
           <element name='readonly'>
             <empty/>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index b4fb6cf981..cc75c6a7dd 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2294,6 +2294,8 @@ virDomainFSDefNew(void)
 
 void virDomainFSDefFree(virDomainFSDefPtr def)
 {
+    size_t i;
+
     if (!def)
         return;
 
@@ -2302,6 +2304,13 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
     virDomainDeviceInfoClear(&def->info);
     VIR_FREE(def->virtio);
 
+    if (def->important) {
+        for (i = 0; i < def->nimportant; i++)
+            if (def->important[i])
+                VIR_FREE(def->important[i]);
+    }
+    VIR_FREE(def->important);
+
     VIR_FREE(def);
 }
 
@@ -10953,6 +10962,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
     VIR_AUTOFREE(char *) usage = NULL;
     VIR_AUTOFREE(char *) units = NULL;
     VIR_AUTOFREE(char *) model = NULL;
+    long n;
 
     ctxt->node = node;
 
@@ -11001,6 +11011,12 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
                                   1, ULLONG_MAX, false) < 0)
         goto error;
 
+    n = virXMLChildElementCount(node);
+    if (n > 0) {
+        if (VIR_ALLOC_N(def->important, n) < 0)
+            goto error;
+    }
+
     cur = node->children;
     while (cur != NULL) {
         if (cur->type == XML_ELEMENT_NODE) {
@@ -11039,6 +11055,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
 
                 if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
                     goto error;
+            } else if (virXMLNodeNameEqual(cur, "important")) {
+                def->important[def->nimportant++] = virXMLPropString(cur, "path");
             }
         }
         cur = cur->next;
@@ -11107,6 +11125,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
         goto error;
 
  cleanup:
+    if (def && def->important && !def->nimportant)
+        VIR_FREE(def->important);
     return def;
 
  error:
@@ -24601,6 +24621,7 @@ virDomainFSDefFormat(virBufferPtr buf,
     const char *src = def->src->path;
     VIR_AUTOCLEAN(virBuffer) driverBuf = VIR_BUFFER_INITIALIZER;
     int ret = -1;
+    size_t i;
 
     if (!type) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -24689,6 +24710,15 @@ virDomainFSDefFormat(virBufferPtr buf,
     if (def->readonly)
         virBufferAddLit(buf, "<readonly/>\n");
 
+    if (def->important) {
+        for (i = 0; i < def->nimportant; ++i) {
+            if (!def->important[i]) continue;
+            virBufferAddLit(buf, "<important");
+            virBufferEscapeString(buf, " path='%s'", def->important[i]);
+            virBufferAddLit(buf, "/>\n");
+        }
+    }
+
     if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
         goto cleanup;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 01c22d8cc3..9bbd66d932 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -818,6 +818,9 @@ struct _virDomainFSDef {
     unsigned long long space_soft_limit; /* in bytes */
     bool symlinksResolved;
     virDomainVirtioOptionsPtr virtio;
+
+    size_t nimportant;
+    char **important;
 };
 
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 50b4205267..2005ccadf8 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2732,6 +2732,7 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
     virBuffer opt = VIR_BUFFER_INITIALIZER;
     const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
     const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
+    size_t i;
 
     if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -2775,6 +2776,15 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
     if (fs->readonly)
         virBufferAddLit(&opt, ",readonly");
 
+    if (fs->important) {
+        for (i = 0; i < fs->nimportant; ++i) {
+            if (i == 0)
+                virBufferAsprintf(&opt, ",vii=%s", fs->important[i]);
+            else
+                virBufferAsprintf(&opt, ":%s", fs->important[i]);
+        }
+    }
+
     if (virBufferCheckError(&opt) < 0)
         goto error;
 
-- 
2.11.0

On Mon, May 06, 2019 at 07:58:28PM +0200, Christian Schoenebeck via Qemu-devel wrote:
> This is the counter part patch against latest libvirt git master head to
> support the 'vii' feature of patch 5, which introduces the XML config
> XML tag "important" on libvirt side.
> 
> To stick with the previous example mentioned with patch 5, likewise
> libvirt XML configuration might then look like this:
> 
>   <domain type='kvm'>
>     ...
>     <devices>
>       ...
>       <filesystem type='mount' accessmode='mapped'>
>         <source dir='/vm/fs'/>
>         <target dir='root'/>
>         <important path='/var/shares'/>
>         <important path='/tmp'/>
>       </filesystem>
>     </devices>
>   </domain>
> 
> Like with the vii qemu virtfs command line argument, the order of the
> "important" tag defines which one gets the highest inode namespace
> (smallest generated suffix) on guest side.

Do we think anyone is likely to use this feature in the real world ?

I'm not really a fan of the representation, because this is affecting
guest ABI via a side effect of the ordering which is the kind of thing
that has got us in trouble before.  If we need control over the IDs
used for each mount point, then I tend to think we need to represent
it explicitly such that the mgmt app sets the exact ID used.

     <pathid dir="/var/shares" id="0x1"/>
     <pathid dir="/tmp" id="0x3"/>

this ensures that the IDs are still stable when adding or removing
paths


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

On Dienstag, 7. Mai 2019 13:57:56 CEST Daniel P. Berrangé wrote:
> >       ...
> >       <filesystem type='mount' accessmode='mapped'>
> >       
> >         <source dir='/vm/fs'/>
> >         <target dir='root'/>
> >         <important path='/var/shares'/>
> >         <important path='/tmp'/>
> >       
> >       </filesystem>
> >     
> >     </devices>
> >   
> >   </domain>
> > 
> > Like with the vii qemu virtfs command line argument, the order of the
> > "important" tag defines which one gets the highest inode namespace
> > (smallest generated suffix) on guest side.
> 
> Do we think anyone is likely to use this feature in the real world ?

I don't know if other people need it, that's one of the reasons why I am 
asking for a coarse high level feedback of the current v3 patch set before 
getting into the details.

The only thing I can say right now is that I must use this feature when 
running Samba to avoid all kinds of serious problems. And I could imagine 
inode namespace control to become more of an issue once nested virtualization 
becomes more popular.

> I'm not really a fan of the representation, because this is affecting
> guest ABI via a side effect of the ordering which is the kind of thing
> that has got us in trouble before.  If we need control over the IDs
> used for each mount point, then I tend to think we need to represent
> it explicitly such that the mgmt app sets the exact ID used.
> 
>      <pathid dir="/var/shares" id="0x1"/>
>      <pathid dir="/tmp" id="0x3"/>
> 
> this ensures that the IDs are still stable when adding or removing
> paths

Well, that would lead to the exact opposite of what you asked for. Because 
allowing admins to configure an exact ID (which I think you mean should be used 
as exact inode suffix by 9p then) would expose implementation details inside 
9pfs to config space, which are subject to change, might collide with 
implementation details, and requires implementation knowledge and extreme care 
by admins so they would pick appropriate IDs with "suffix-free" property which 
are guaranteed to create unique numbers in all cases:

https://en.wikipedia.org/wiki/Prefix_code

Also keep in mind that one fs device might end up having multiple suffixes.

Hence my suggestion was to only expose the bare minimum to config space 
regarding this issue: Asking (if required at all) admins which ones are the 
most critical pathes regarding inode namespace for their use cases, and 9p 
would then automatically generate appropriate suffixes for those mentioned by 
admin to achieve the highest inode namespace appropriately and in a safe way.

Plus for the "important path=" semantics I suggested you don't have have to 
use mount points BTW. You can use subdirs and even individual files and 9pfs 
would then automatically resolve the appropriate fs device of the given path. 
So e.g. when using nested virtualization, an admin inside a lower level guest 
does not even need to know the exact mount points on a higher level / host.

Best regards,
Christian Schoenebeck

On Mon, 06 May 2019 19:58:28 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This is the counter part patch against latest libvirt git master head to

Hmm... shouldn't this be Cc'd to libvir-list@redhat.com as well then ?

> support the 'vii' feature of patch 5, which introduces the XML config

What is patch 5 ?!? What is 'vii' ? I am a bit lost here...

> XML tag "important" on libvirt side.
> 
> To stick with the previous example mentioned with patch 5, likewise
> libvirt XML configuration might then look like this:
> 
>   <domain type='kvm'>
>     ...
>     <devices>
>       ...
>       <filesystem type='mount' accessmode='mapped'>
>         <source dir='/vm/fs'/>
>         <target dir='root'/>
>         <important path='/var/shares'/>
>         <important path='/tmp'/>
>       </filesystem>
>     </devices>
>   </domain>
> 
> Like with the vii qemu virtfs command line argument, the order of the
> "important" tag defines which one gets the highest inode namespace
> (smallest generated suffix) on guest side.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  docs/schemas/domaincommon.rng |  6 ++++++
>  src/conf/domain_conf.c        | 30 ++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h        |  3 +++
>  src/qemu/qemu_command.c       | 10 ++++++++++
>  4 files changed, 49 insertions(+)
> 
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 111b85c36f..c75edfc4d3 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2515,6 +2515,12 @@
>              </choice>
>            </attribute>
>          </optional>
> +        <zeroOrMore>
> +          <element name='important'>
> +            <attribute name="path"/>
> +            <empty/>
> +          </element>
> +        </zeroOrMore>
>          <optional>
>            <element name='readonly'>
>              <empty/>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b4fb6cf981..cc75c6a7dd 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2294,6 +2294,8 @@ virDomainFSDefNew(void)
>  
>  void virDomainFSDefFree(virDomainFSDefPtr def)
>  {
> +    size_t i;
> +
>      if (!def)
>          return;
>  
> @@ -2302,6 +2304,13 @@ void virDomainFSDefFree(virDomainFSDefPtr def)
>      virDomainDeviceInfoClear(&def->info);
>      VIR_FREE(def->virtio);
>  
> +    if (def->important) {
> +        for (i = 0; i < def->nimportant; i++)
> +            if (def->important[i])
> +                VIR_FREE(def->important[i]);
> +    }
> +    VIR_FREE(def->important);
> +
>      VIR_FREE(def);
>  }
>  
> @@ -10953,6 +10962,7 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>      VIR_AUTOFREE(char *) usage = NULL;
>      VIR_AUTOFREE(char *) units = NULL;
>      VIR_AUTOFREE(char *) model = NULL;
> +    long n;
>  
>      ctxt->node = node;
>  
> @@ -11001,6 +11011,12 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>                                    1, ULLONG_MAX, false) < 0)
>          goto error;
>  
> +    n = virXMLChildElementCount(node);
> +    if (n > 0) {
> +        if (VIR_ALLOC_N(def->important, n) < 0)
> +            goto error;
> +    }
> +
>      cur = node->children;
>      while (cur != NULL) {
>          if (cur->type == XML_ELEMENT_NODE) {
> @@ -11039,6 +11055,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>  
>                  if (virDomainVirtioOptionsParseXML(cur, &def->virtio) < 0)
>                      goto error;
> +            } else if (virXMLNodeNameEqual(cur, "important")) {
> +                def->important[def->nimportant++] = virXMLPropString(cur, "path");
>              }
>          }
>          cur = cur->next;
> @@ -11107,6 +11125,8 @@ virDomainFSDefParseXML(virDomainXMLOptionPtr xmlopt,
>          goto error;
>  
>   cleanup:
> +    if (def && def->important && !def->nimportant)
> +        VIR_FREE(def->important);
>      return def;
>  
>   error:
> @@ -24601,6 +24621,7 @@ virDomainFSDefFormat(virBufferPtr buf,
>      const char *src = def->src->path;
>      VIR_AUTOCLEAN(virBuffer) driverBuf = VIR_BUFFER_INITIALIZER;
>      int ret = -1;
> +    size_t i;
>  
>      if (!type) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -24689,6 +24710,15 @@ virDomainFSDefFormat(virBufferPtr buf,
>      if (def->readonly)
>          virBufferAddLit(buf, "<readonly/>\n");
>  
> +    if (def->important) {
> +        for (i = 0; i < def->nimportant; ++i) {
> +            if (!def->important[i]) continue;
> +            virBufferAddLit(buf, "<important");
> +            virBufferEscapeString(buf, " path='%s'", def->important[i]);
> +            virBufferAddLit(buf, "/>\n");
> +        }
> +    }
> +
>      if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0)
>          goto cleanup;
>  
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 01c22d8cc3..9bbd66d932 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -818,6 +818,9 @@ struct _virDomainFSDef {
>      unsigned long long space_soft_limit; /* in bytes */
>      bool symlinksResolved;
>      virDomainVirtioOptionsPtr virtio;
> +
> +    size_t nimportant;
> +    char **important;
>  };
>  
>  
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 50b4205267..2005ccadf8 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2732,6 +2732,7 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
>      virBuffer opt = VIR_BUFFER_INITIALIZER;
>      const char *driver = qemuDomainFSDriverTypeToString(fs->fsdriver);
>      const char *wrpolicy = virDomainFSWrpolicyTypeToString(fs->wrpolicy);
> +    size_t i;
>  
>      if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
>          virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> @@ -2775,6 +2776,15 @@ qemuBuildFSStr(virDomainFSDefPtr fs)
>      if (fs->readonly)
>          virBufferAddLit(&opt, ",readonly");
>  
> +    if (fs->important) {
> +        for (i = 0; i < fs->nimportant; ++i) {
> +            if (i == 0)
> +                virBufferAsprintf(&opt, ",vii=%s", fs->important[i]);
> +            else
> +                virBufferAsprintf(&opt, ":%s", fs->important[i]);
> +        }
> +    }
> +
>      if (virBufferCheckError(&opt) < 0)
>          goto error;
>  

On Dienstag, 7. Mai 2019 11:55:56 CEST Greg Kurz wrote:
> > support the 'vii' feature of patch 5, which introduces the XML config
> 
> What is patch 5 ?!? What is 'vii' ? I am a bit lost here...

Hi Greg,

Sorry that I caused a bit of confusion, You were actually commenting mostly on 
v2 of the patch set, where my email client replaced the message IDs and hence 
screwed threading.

This is v3 that I sent yesterday and which has correct threading:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html

Please just have a glimpse on that v3 thread, and before I address the details 
that you requested (I have reviewed them all already and will address them), I 
would like you to ask you for a coarse feedback on design/features first. 
Because there are some things where I am unresolved on design level yet:

1. Should I drop the "persistency" feature of patch 3 (same inode numbers 
after reboots/suspends)  completely from the patch set? It is disabled at 
compile time by default for now after entire v3 patch set is applied. Or 
should that persistency feature probably become a qemu command line option 
instead?

2. If persistency feature should be preserved, shall I probably move out all 
the inode remapping code into a separate C unit to avoid 9p.c getting bloated 
too much (the amount of code for saving/loading the qp*_table hash tables is 
quite large). If yes, any suggestion for an appropriate unit name?

3. Are you fine with the suggested variable length suffixes (patch 4) becoming 
the default behaviour (instead of the fixed length 16 bit prefix solution by 
Antonios)?

4. Do you have a better idea for a name instead of the suggested "vii" (patch 
5) virtfs qemu command line option? And are you fine with the idea of that 
"vii" feature anyway?

> > This is the counter part patch against latest libvirt git master head to
> 
> Hmm... shouldn't this be Cc'd to libvir-list@redhat.com as well then ?

Well, for now I just provided that libvirt patch to give you an idea about how 
imagined this "vii" feature to be used. Does it make sense to CC them already 
even though this suggested "vii" command line option does not exist on qemu 
side yet?

I know I piled up quite a bit of code on this patch set, so to speed up things 
simply raise questions instead of spending too much time in reviewing 
everything in detail already.

Thanks Greg!

Best regards,
Christian Schoenebeck

On Tue, 07 May 2019 14:23:11 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 7. Mai 2019 11:55:56 CEST Greg Kurz wrote:
> > > support the 'vii' feature of patch 5, which introduces the XML config  
> > 
> > What is patch 5 ?!? What is 'vii' ? I am a bit lost here...  
> 
> Hi Greg,
> 
> Sorry that I caused a bit of confusion, You were actually commenting mostly on 
> v2 of the patch set, where my email client replaced the message IDs and hence 
> screwed threading.
> 
> This is v3 that I sent yesterday and which has correct threading:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
> 

For a reason yet to be investigated, I haven't received it yet...

> Please just have a glimpse on that v3 thread, and before I address the details 
> that you requested (I have reviewed them all already and will address them), I 
> would like you to ask you for a coarse feedback on design/features first. 
> Because there are some things where I am unresolved on design level yet:
> 

I'll try but probably not before next week.

> 1. Should I drop the "persistency" feature of patch 3 (same inode numbers 
> after reboots/suspends)  completely from the patch set? It is disabled at 
> compile time by default for now after entire v3 patch set is applied. Or 
> should that persistency feature probably become a qemu command line option 
> instead?
> 
> 2. If persistency feature should be preserved, shall I probably move out all 
> the inode remapping code into a separate C unit to avoid 9p.c getting bloated 
> too much (the amount of code for saving/loading the qp*_table hash tables is 
> quite large). If yes, any suggestion for an appropriate unit name?
> 
> 3. Are you fine with the suggested variable length suffixes (patch 4) becoming 
> the default behaviour (instead of the fixed length 16 bit prefix solution by 
> Antonios)?
> 
> 4. Do you have a better idea for a name instead of the suggested "vii" (patch 
> 5) virtfs qemu command line option? And are you fine with the idea of that 
> "vii" feature anyway?
> 
> > > This is the counter part patch against latest libvirt git master head to  
> > 
> > Hmm... shouldn't this be Cc'd to libvir-list@redhat.com as well then ?  
> 
> Well, for now I just provided that libvirt patch to give you an idea about how 
> imagined this "vii" feature to be used. Does it make sense to CC them already 
> even though this suggested "vii" command line option does not exist on qemu 
> side yet?
> 
> I know I piled up quite a bit of code on this patch set, so to speed up things 
> simply raise questions instead of spending too much time in reviewing 
> everything in detail already.
> 
> Thanks Greg!
> 
> Best regards,
> Christian Schoenebeck

On Dienstag, 7. Mai 2019 17:42:39 CEST Greg Kurz wrote:
> > Sorry that I caused a bit of confusion, You were actually commenting
> > mostly on v2 of the patch set, where my email client replaced the message
> > IDs and hence screwed threading.
> > 
> > This is v3 that I sent yesterday and which has correct threading:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
> 
> For a reason yet to be investigated, I haven't received it yet...

Here are the archive links for latest v3 patch set [5(+1) patches total]:

[PATCH v3 0/5] 9p: Fix file ID collisions:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html

[PATCH v3 1/5] 9p: mitigates most QID path collisions:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01142.html

[PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01140.html

[PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01144.html

[PATCH v3 4/5] 9p: use variable length suffixes for inode mapping:
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01141.html

[PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01138.html

And the optional libvirt patch:

[libvirt patch] qemu: adds support for virtfs 9p argument 'vii':
https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01223.html

> > Please just have a glimpse on that v3 thread, and before I address the
> > details that you requested (I have reviewed them all already and will
> > address them), I would like you to ask you for a coarse feedback on
> > design/features first.
> > Because there are some things where I am unresolved on design level yet:
> I'll try but probably not before next week.

No problem, take your time!

Best regards,
Christian Schoenebeck

On Dienstag, 7. Mai 2019 18:16:08 CEST Christian Schoenebeck wrote:
> Here are the archive links for latest v3 patch set [5(+1) patches total]:
> 
> [PATCH v3 0/5] 9p: Fix file ID collisions:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
> 
> [PATCH v3 1/5] 9p: mitigates most QID path collisions:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01142.html
> 
> [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01140.html
> 
> [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01144.html
> 
> [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping:
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01141.html
> 
> [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01138.html
> 
> And the optional libvirt patch:
> 
> [libvirt patch] qemu: adds support for virtfs 9p argument 'vii':
> https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01223.html
> 
> > > Please just have a glimpse on that v3 thread, and before I address the
> > > details that you requested (I have reviewed them all already and will
> > > address them), I would like you to ask you for a coarse feedback on
> > > design/features first.
> > 
> > > Because there are some things where I am unresolved on design level yet:
> > 
> > I'll try but probably not before next week.

Hi Greg, you have not forgotten about me, did you? ;-)

Or should I go ahead and provide a v4 next week addressing the issues 
discussed so far?

Best regards,
Christian Schoenebeck

On Fri, 17 May 2019 10:40:48 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 7. Mai 2019 18:16:08 CEST Christian Schoenebeck wrote:
> > Here are the archive links for latest v3 patch set [5(+1) patches total]:
> > 
> > [PATCH v3 0/5] 9p: Fix file ID collisions:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01143.html
> > 
> > [PATCH v3 1/5] 9p: mitigates most QID path collisions:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01142.html
> > 
> > [PATCH v3 2/5] 9P: trivial cleanup of QID path collision mitigation:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01140.html
> > 
> > [PATCH v3 3/5] 9p: persistency of QID path beyond reboots / suspensions:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01144.html
> > 
> > [PATCH v3 4/5] 9p: use variable length suffixes for inode mapping:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01141.html
> > 
> > [PATCH v3 5/5] 9p: adds virtfs 'vii' device parameter
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01138.html
> > 
> > And the optional libvirt patch:
> > 
> > [libvirt patch] qemu: adds support for virtfs 9p argument 'vii':
> > https://lists.gnu.org/archive/html/qemu-devel/2019-05/msg01223.html
> >   
> > > > Please just have a glimpse on that v3 thread, and before I address the
> > > > details that you requested (I have reviewed them all already and will
> > > > address them), I would like you to ask you for a coarse feedback on
> > > > design/features first.  
> > >   
> > > > Because there are some things where I am unresolved on design level yet:  
> > > 
> > > I'll try but probably not before next week.  
> 
> Hi Greg, you have not forgotten about me, did you? ;-)
> 

Hi Christian,

I have certainly not forgotten but I had (and still have) some more urgent
work to do and I couldn't find time for this... Sorry :)

> Or should I go ahead and provide a v4 next week addressing the issues 
> discussed so far?
> 

Thinking again about the initial issue raised by Antonios, I agree we
should at least detect collisions in the existing 9pfs code and
emit an error rather than silently returning duplicate QIDs to the
client. This would be patch 2 from Antonios's series: only allow
a single host device for a given fs device.

Then, we come to the bulk problem: how to handle the case where we
have multiple devices involved in a directory we want to share ?
Antonios's proposal is a clever way to address the collisions, but
your work proves it isn't enough... Before going forward, I'd like
to consider another approach.

What about:

1) de-compose the shared directory on a per-device basis,
   ie. identify all mount points under the shared directory

2) expose found mount points separately, each with its onw 9p device

3) re-compose the directory tree within the guest using the same topology
   as the host

ie. if you want to share /vm/fs and

/vm/fs on device A
/vm/fs/shares on device B
/vm/fs/tmp on device C

you would start QEMU with

-fsdev local,path=/vm/fs,id=fsdev0... \
-device virtio-9p,fsdev=fsdev0,mount_tag=tag0 \
-fsdev local,path=/vm/fs,id=fsdev1... \
-device virtio-9p,fsdev=fsdev1,mount_tag=tag1 \
-fsdev local,path=/vm/fs,id=fsdev2... \
-device virtio-9p,fsdev=fsdev2,mount_tag=tag2

and /etc/fstab in the guest:

tag0    /       9p      nofail,trans=virtio,version=9p2000.L   0 0
tag1    /shares 9p      nofail,trans=virtio,version=9p2000.L   0 0
tag2    /tmp    9p      nofail,trans=virtio,version=9p2000.L   0 0

This involves some more work for the user but it doesn't require
any changes in QEMU.

Would this approach solve the issues you've been hitting with Samba ?

Cheers,

--
Greg

On Freitag, 17. Mai 2019 14:30:29 CEST Greg Kurz wrote:
> Then, we come to the bulk problem: how to handle the case where we
> have multiple devices involved in a directory we want to share ?
> Antonios's proposal is a clever way to address the collisions, but
> your work proves it isn't enough...

With the patch set I have right now, things finally bahave smooth.

> Before going forward, I'd like
> to consider another approach.
> 
> What about:
> 
> 1) de-compose the shared directory on a per-device basis,
>    ie. identify all mount points under the shared directory
> 
> 2) expose found mount points separately, each with its onw 9p device
> 
> 3) re-compose the directory tree within the guest using the same topology
>    as the host
> 
> ie. if you want to share /vm/fs and
> 
> /vm/fs on device A
> /vm/fs/shares on device B
> /vm/fs/tmp on device C
> 
> you would start QEMU with
> 
> -fsdev local,path=/vm/fs,id=fsdev0... \
> -device virtio-9p,fsdev=fsdev0,mount_tag=tag0 \
> -fsdev local,path=/vm/fs,id=fsdev1... \
> -device virtio-9p,fsdev=fsdev1,mount_tag=tag1 \
> -fsdev local,path=/vm/fs,id=fsdev2... \
> -device virtio-9p,fsdev=fsdev2,mount_tag=tag2
> 
> and /etc/fstab in the guest:
> 
> tag0    /       9p      nofail,trans=virtio,version=9p2000.L   0 0
> tag1    /shares 9p      nofail,trans=virtio,version=9p2000.L   0 0
> tag2    /tmp    9p      nofail,trans=virtio,version=9p2000.L   0 0
> 
> This involves some more work for the user but it doesn't require
> any changes in QEMU.

So your suggestion is actually: don't fix it.

"Some" more work for the user is a quantity of how many guests you are 
running, multiplied by the nested virtualization levels you might have = 
potentially a lot of work for admins.

> Would this approach solve the issues you've been hitting with Samba ?

No, because that completely neglects runtime changes on a higher level (host), 
plus it completely destroys the fundamental idea about 9p, which is about 
transparency of the higher level(s).

May I ask, do you have concrete reasons why you want to abondon the entire 
patch set? Because that's what it sounds to me.

Best regards,
Christian Schoenebeck

On Fri, 17 May 2019 15:23:01 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 17. Mai 2019 14:30:29 CEST Greg Kurz wrote:
> > Then, we come to the bulk problem: how to handle the case where we
> > have multiple devices involved in a directory we want to share ?
> > Antonios's proposal is a clever way to address the collisions, but
> > your work proves it isn't enough...  
> 
> With the patch set I have right now, things finally bahave smooth.
> 
> > Before going forward, I'd like
> > to consider another approach.
> > 
> > What about:
> > 
> > 1) de-compose the shared directory on a per-device basis,
> >    ie. identify all mount points under the shared directory
> > 
> > 2) expose found mount points separately, each with its onw 9p device
> > 
> > 3) re-compose the directory tree within the guest using the same topology
> >    as the host
> > 
> > ie. if you want to share /vm/fs and
> > 
> > /vm/fs on device A
> > /vm/fs/shares on device B
> > /vm/fs/tmp on device C
> > 
> > you would start QEMU with
> > 
> > -fsdev local,path=/vm/fs,id=fsdev0... \
> > -device virtio-9p,fsdev=fsdev0,mount_tag=tag0 \
> > -fsdev local,path=/vm/fs,id=fsdev1... \
> > -device virtio-9p,fsdev=fsdev1,mount_tag=tag1 \
> > -fsdev local,path=/vm/fs,id=fsdev2... \
> > -device virtio-9p,fsdev=fsdev2,mount_tag=tag2
> > 
> > and /etc/fstab in the guest:
> > 
> > tag0    /       9p      nofail,trans=virtio,version=9p2000.L   0 0
> > tag1    /shares 9p      nofail,trans=virtio,version=9p2000.L   0 0
> > tag2    /tmp    9p      nofail,trans=virtio,version=9p2000.L   0 0
> > 
> > This involves some more work for the user but it doesn't require
> > any changes in QEMU.  
> 
> So your suggestion is actually: don't fix it.
> 

Potentially yes if another approach is satisfying enough, as I wouldn't
want to over-engineer too much around this 9p imposed limitation. The
right thing to do would be to come up with a new version of the protocol
with variable sized QIDs and call it a day.

> "Some" more work for the user is a quantity of how many guests you are 
> running, multiplied by the nested virtualization levels you might have = 
> potentially a lot of work for admins.
> 

Maybe, I don't have enough feedback on 9p use cases to have a clear
picture...

> > Would this approach solve the issues you've been hitting with Samba ?  
> 
> No, because that completely neglects runtime changes on a higher level (host), 

Unless I'm missing something, runtime changes would be neglected as well
with the "vii" property since it is static for the device lifetime.

> plus it completely destroys the fundamental idea about 9p, which is about 
> transparency of the higher level(s).
> 

That's a point indeed, even if again I'm not sure if this is a frequent
case to share a directory tree spanning over multiple devices.

> May I ask, do you have concrete reasons why you want to abondon the entire 
> patch set? Because that's what it sounds to me.
> 

I don't have that much time to spend on 9p maintainership, for both
reviewing and fixing bugs (CVEs most of the time). So, yes it may
sound like I want to drop the patchset, but it's just I need to be
convinced I won't regret having merged a huge change like this...
when I'll have to support it alone later ;-)

For the moment, I'm not convinced by the "vii" solution. It even
motivated my suggestion of having several devices actually, since
the paths you would put in there are known before starting QEMU.

It might take me some more rounds of discussion to decide. I understand
it is frustrating but bear with me :)

> Best regards,
> Christian Schoenebeck

Cheers,

--
Greg

On Freitag, 17. Mai 2019 16:47:46 CEST Greg Kurz wrote:
> Potentially yes if another approach is satisfying enough, as I wouldn't
> want to over-engineer too much around this 9p imposed limitation. The
> right thing to do would be to come up with a new version of the protocol
> with variable sized QIDs and call it a day.

Yes, but that's the long-term solution which will still take a very long 
while. This patch set is already a functional solution until that happens, and 
this 9p issue is IMO quite severe (as minor as data corruption, data loss and 
exists for several years already).

> > plus it completely destroys the fundamental idea about 9p, which is about
> > transparency of the higher level(s).
> 
> That's a point indeed, even if again I'm not sure if this is a frequent
> case to share a directory tree spanning over multiple devices.

When the use case is mass storage then it is very likely that you will have 
several devices. Times have changed. With modern file systems like ZFS it is 
very common to create a large amount of "data sets" for all kinds of 
individual purposes and mount points which is cheap to get. Each fs data set 
is a separate "device" from OS (i.e. Linux) point of view, even if all those 
FS data sets are in the same FS pool and even on the same physical IO device.

Background: The concept of FS "data sets" combines the benefits of classical  
partitions (e.g. logical file space separation, independent fs configurations 
like compression on/off/algorithm, data deduplication on/off, snapshot 
isolation, snapshots on/off) without the disadvantages of classical real 
partitions (physical space is dynamically shared, no space wasted on fixed 
boundaries; physical device pool management is transparent for all data sets, 
configuration options can be inherited from parent data sets).

> I don't have that much time to spend on 9p maintainership, for both
> reviewing and fixing bugs (CVEs most of the time). So, yes it may
> sound like I want to drop the patchset, but it's just I need to be
> convinced I won't regret having merged a huge change like this...
> when I'll have to support it alone later ;-)

Actually I already assumed this to be the actual root cause.

I see that you are currently the only maintainer, and my plan was not to just 
provide a one time shot but eventually hang along helping with maintaining it 
due my use of 9p and its huge potential I see (as universal and efficient root 
file system for all guests, not just for exotically sharing a small tree 
portion between guests). I also have plans for supporting native file forks 
BTW. But if you are seriously suggesting to simply omit a fundamental issue in 
9p, then my plans would obviously no longer make sense. :)

I mean I am completely tolerant to all kinds of religious views on bit level, 
languages, code style, libs, precise implementation details, parameters, 
source structure, etc.; but saying to simply leave a fundamental bug open for 
years to come, that's where I have to drop out.

> For the moment, I'm not convinced by the "vii" solution. It even
> motivated my suggestion of having several devices actually, since
> the paths you would put in there are known before starting QEMU.

Well, that "vii" configuration and the QID persistency are 2 optional patches 
on top of the core fixes. It is a huge difference to suggest dropping these 2 
patches than saying to completely drop the entire patch set and to leave this 
bug open.

The mandatory core fixes that I see (for the short/mid term) are at least 
Antonios' patches plus my variable length prefix patch, the latter significantly 
reduces the inode numbers on guest and significantly increases the inode name 
space, and hence also significanlty reduces the propability that 9p might ever 
need to kick in with any kind of expensive remapping actions (or even 
something worse like stat fatally returning -ENFILE to the user).

About the "vii" configuration, even though you don't like the idea: there is 
also a big difference giving the user the _optional_ possibility to define e.g. 
one path (not device) on guest said to be sensitive regarding high inode 
numbers on guest; and something completely different telling the user that he 
_must_ configure every single device from host that is ever supposed to pop up 
with 9p on guest and forcing the user to update that configuration whenever a 
new device is added or removed on host. The "vii" configuration feature does 
not require any knowledge of how many and which kinds of devices are actually 
ever used on host (nor on any higher level host in case of nested 
virtualization), nor does that "vii" config require any changes ever when host 
device setup changes. So 9p's core transparency feature would not be touched 
at all.

> It might take me some more rounds of discussion to decide. I understand
> it is frustrating but bear with me :)

Let me make a different suggestion: how about putting these fixes into a 
separate C unit for now and making the default behaviour (if you really want) 
to not use any of that code by default at all. So the user would just get an 
error message in the qemu log files by default if he tries to export several 
devices with one 9p device, suggesting him either to map them as separate 9p 
devices (like you suggested) and informing the user about the alternative of 
enabling support for the automatic inode remapping code (from that separate C 
unit) instead by adding one convenient config option if he/she really wants.

That way we would have a fix now, not in years, people can decide to use the 
automatic and hardware transparent solution, instead of being forced to write 
dozens of config lines for each single guest, or they might decide to stick 
with your "solution" ;-).

And once the long term solution for this issue with variable length QIDs is in 
place, the inode remapping code can very simply be dropped from the code base 
completley by just deleting the C unit and about a hand full of lines in 9p.c 
or so, and hence this fix would not bloat the existing 9p units nor cause 
maintenance nightmares of any kind.

Best regards,
Christian Schoenebeck

Hi Christian,

On Fri, 17 May 2019 22:53:41 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Freitag, 17. Mai 2019 16:47:46 CEST Greg Kurz wrote:
> > Potentially yes if another approach is satisfying enough, as I wouldn't
> > want to over-engineer too much around this 9p imposed limitation. The
> > right thing to do would be to come up with a new version of the protocol
> > with variable sized QIDs and call it a day.  
> 
> Yes, but that's the long-term solution which will still take a very long 
> while. This patch set is already a functional solution until that happens, and 
> this 9p issue is IMO quite severe (as minor as data corruption, data loss and 
> exists for several years already).
> 

On the other hand, I'm afraid that having a functional solution won't
motivate people to come up with a new spec... Anyway, I agree that the
data corruption/loss issues must be prevented, ie, the 9p server should
at least return an error to the client rather than returning a colliding
QID. A simple way to avoid that is to enforce a single device, ie. patch
2 in Antonios's series. Of course this may break some setups where
multiple devices are involved, but it is pure luck if they aren't already
broken with the current code base. I assume that this covers most 9p users,
based on the fact that Antonios and now yourself are the only people who
ever reported this issue. Then, I'm okay if we try to cover some more
scenarios, but it must take the maintainance burden into account.

> > > plus it completely destroys the fundamental idea about 9p, which is about
> > > transparency of the higher level(s).  
> > 
> > That's a point indeed, even if again I'm not sure if this is a frequent
> > case to share a directory tree spanning over multiple devices.  
> 
> When the use case is mass storage then it is very likely that you will have 
> several devices. Times have changed. With modern file systems like ZFS it is 
> very common to create a large amount of "data sets" for all kinds of 
> individual purposes and mount points which is cheap to get. Each fs data set 
> is a separate "device" from OS (i.e. Linux) point of view, even if all those 
> FS data sets are in the same FS pool and even on the same physical IO device.
> 
> Background: The concept of FS "data sets" combines the benefits of classical  
> partitions (e.g. logical file space separation, independent fs configurations 
> like compression on/off/algorithm, data deduplication on/off, snapshot 
> isolation, snapshots on/off) without the disadvantages of classical real 
> partitions (physical space is dynamically shared, no space wasted on fixed 
> boundaries; physical device pool management is transparent for all data sets, 
> configuration options can be inherited from parent data sets).
> 

Ok. I must admit my ignorance around ZFS and "data sets"... so IIUC, even with
a single underlying physical device you might end up with lstat() returning
different st_dev values on the associated files, correct ?

I take your word for the likeliness of the issue to popup in such setups. :)

> > I don't have that much time to spend on 9p maintainership, for both
> > reviewing and fixing bugs (CVEs most of the time). So, yes it may
> > sound like I want to drop the patchset, but it's just I need to be
> > convinced I won't regret having merged a huge change like this...
> > when I'll have to support it alone later ;-)  
> 
> Actually I already assumed this to be the actual root cause.
> 
> I see that you are currently the only maintainer, and my plan was not to just 
> provide a one time shot but eventually hang along helping with maintaining it 
> due my use of 9p and its huge potential I see (as universal and efficient root 
> file system for all guests, not just for exotically sharing a small tree 
> portion between guests). I also have plans for supporting native file forks 

What are "native file forks" ?

> BTW. But if you are seriously suggesting to simply omit a fundamental issue in 
> 9p, then my plans would obviously no longer make sense. :)
> 

It would be very much appreciated to have someone else investing time in 9p
of course. :)

> I mean I am completely tolerant to all kinds of religious views on bit level, 
> languages, code style, libs, precise implementation details, parameters, 
> source structure, etc.; but saying to simply leave a fundamental bug open for 
> years to come, that's where I have to drop out.
> 

I guess I was unclear... As said above, I do want to avoid QID collisions,
at least by enforcing a single device per shared path. Then, supporting
multiple devices will depend on the benefice/expense ratio.

> > For the moment, I'm not convinced by the "vii" solution. It even
> > motivated my suggestion of having several devices actually, since
> > the paths you would put in there are known before starting QEMU.  
> 
> Well, that "vii" configuration and the QID persistency are 2 optional patches 
> on top of the core fixes. It is a huge difference to suggest dropping these 2 
> patches than saying to completely drop the entire patch set and to leave this 
> bug open.
> 
> The mandatory core fixes that I see (for the short/mid term) are at least 
> Antonios' patches plus my variable length prefix patch, the latter significantly 
> reduces the inode numbers on guest and significantly increases the inode name 
> space, and hence also significanlty reduces the propability that 9p might ever 
> need to kick in with any kind of expensive remapping actions (or even 
> something worse like stat fatally returning -ENFILE to the user).
> 
> About the "vii" configuration, even though you don't like the idea: there is 

Hmm... I was objecting on the fact that "vii" would cope with runtime
changes on the host while the guest is running.

> also a big difference giving the user the _optional_ possibility to define e.g. 
> one path (not device) on guest said to be sensitive regarding high inode 
> numbers on guest; and something completely different telling the user that he 
> _must_ configure every single device from host that is ever supposed to pop up 
> with 9p on guest and forcing the user to update that configuration whenever a 
> new device is added or removed on host. The "vii" configuration feature does 
> not require any knowledge of how many and which kinds of devices are actually 
> ever used on host (nor on any higher level host in case of nested 
> virtualization), nor does that "vii" config require any changes ever when host 
> device setup changes. So 9p's core transparency feature would not be touched 
> at all.
> 

I guess this all boils down to I finding some time to read/understand more :)

As usual, a series with smaller and simpler patches will be easier to
review, and more likely to be merged.

> > It might take me some more rounds of discussion to decide. I understand
> > it is frustrating but bear with me :)  
> 
> Let me make a different suggestion: how about putting these fixes into a 
> separate C unit for now and making the default behaviour (if you really want) 
> to not use any of that code by default at all. So the user would just get an 
> error message in the qemu log files by default if he tries to export several 
> devices with one 9p device, suggesting him either to map them as separate 9p 
> devices (like you suggested) and informing the user about the alternative of 
> enabling support for the automatic inode remapping code (from that separate C 
> unit) instead by adding one convenient config option if he/she really wants.
> 

It seems that we may be reaching some consensus here :)

I like the approach, provided this is configurable from the command line, off
by default and doesn't duplicate core 9p code. Not sure this needs to sit in
its own C unit though.

> That way we would have a fix now, not in years, people can decide to use the 
> automatic and hardware transparent solution, instead of being forced to write 
> dozens of config lines for each single guest, or they might decide to stick 
> with your "solution" ;-).
> 
> And once the long term solution for this issue with variable length QIDs is in 
> place, the inode remapping code can very simply be dropped from the code base 
> completley by just deleting the C unit and about a hand full of lines in 9p.c 
> or so, and hence this fix would not bloat the existing 9p units nor cause 
> maintenance nightmares of any kind.
> 

The 9p code has a long history of CVEs and limitations that prevented it
to reach full production grade quality. Combined with the poor quality of
the code, this has scared off many experienced QEMU developpers, which
prefer to work on finding an alternative solution. Such alternative is
virtio-fs, which is being actively worked on:

https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02746.html

Note: I'm not telling "stay away from 9p" but maybe worth taking a look,
      because if virtio-fs gets merged, it is likely to become the official
      and better supported way to share files between host and guest.

> Best regards,
> Christian Schoenebeck

Please repost a series, possibly based on some of Antonios's patches that
allows to avoid the QID collision, returns an error to the client instead
and possibly printing out some useful messages in the QEMU log. Then, on
top of that, you can start introducing hashing and variable prefix length.

Cheers,

--
Greg

On Montag, 20. Mai 2019 16:05:09 CEST Greg Kurz wrote:
> Hi Christian,

Hi Greg,

> On the other hand, I'm afraid that having a functional solution won't
> motivate people to come up with a new spec... Anyway, I agree that the
> data corruption/loss issues must be prevented, ie, the 9p server should
> at least return an error to the client rather than returning a colliding
> QID. 

Ok, I will extend Antonios' patch to log that error on host. I thought about 
limiting that error message to once per session (for not flooding the logs), 
but it is probably not worth it, so if you don't veto then I will just log 
that error simply on every file access.

> A simple way to avoid that is to enforce a single device, ie. patch
> 2 in Antonios's series. Of course this may break some setups where
> multiple devices are involved, but it is pure luck if they aren't already
> broken with the current code base. 

Yes, the worst thing you can have is this collision silently being ignored, 
like it is actually right now. Because you end up getting all sorts of 
different misbehaviours on guests, and these are just symptoms that take a 
while to debug and realise that the actual root cause is an inode collision. 
So enforcing a single device is still better than undefined behaviour.

> > Background: The concept of FS "data sets" combines the benefits of
> > classical partitions (e.g. logical file space separation, independent fs
> > configurations like compression on/off/algorithm, data deduplication
> > on/off, snapshot isolation, snapshots on/off) without the disadvantages
> > of classical real partitions (physical space is dynamically shared, no
> > space wasted on fixed boundaries; physical device pool management is
> > transparent for all data sets, configuration options can be inherited
> > from parent data sets).
> 
> Ok. I must admit my ignorance around ZFS and "data sets"... so IIUC, even
> with a single underlying physical device you might end up with lstat()
> returning different st_dev values on the associated files, correct ?
> 
> I take your word for the likeliness of the issue to popup in such setups. :)

Yes, that is correct, you _always_ get a different stat::st_dev value for each 
ZFS data set. Furthermore, each ZFS data set has its own inode number sequence 
generator starting from one. So consider you create two new ZFS data sets, 
then you create one file on each data set, then both files will have inode 
number 1.

That probably makes it clear why you hit this ID collision bug very easily 
when using the combination ZFS & 9p.

> > also a big difference giving the user the _optional_ possibility to define
> > e.g. one path (not device) on guest said to be sensitive regarding high
> > inode numbers on guest; and something completely different telling the
> > user that he _must_ configure every single device from host that is ever
> > supposed to pop up with 9p on guest and forcing the user to update that
> > configuration whenever a new device is added or removed on host. The
> > "vii" configuration feature does not require any knowledge of how many
> > and which kinds of devices are actually ever used on host (nor on any
> > higher level host in case of nested
> > virtualization), nor does that "vii" config require any changes ever when
> > host device setup changes. So 9p's core transparency feature would not be
> > touched at all.
> 
> I guess this all boils down to I finding some time to read/understand more
> :)

Yes, that helps sometimes. :)

> As usual, a series with smaller and simpler patches will be easier to
> review, and more likely to be merged.

Of course.

In the next patch series I will completely drop a) the entire QID persistency 
feature code and b) that disputed "vii" feature. But I will still suggest the 
variable inode suffix length patch as last patch in that new patch series.

That should make the amount of changes manageable small.

> > Let me make a different suggestion: how about putting these fixes into a
> > separate C unit for now and making the default behaviour (if you really
> > want) to not use any of that code by default at all. So the user would
> > just get an error message in the qemu log files by default if he tries to
> > export several devices with one 9p device, suggesting him either to map
> > them as separate 9p devices (like you suggested) and informing the user
> > about the alternative of enabling support for the automatic inode
> > remapping code (from that separate C unit) instead by adding one
> > convenient config option if he/she really wants.
> It seems that we may be reaching some consensus here :)
> 
> I like the approach, provided this is configurable from the command line,
> off by default and doesn't duplicate core 9p code. Not sure this needs to
> sit in its own C unit though.

Any preference for a command line argument name and/or libvirt XML config tag/
attribute for switching the inode remapping code on?

About that separate C unit: I leave that up to you to decide, it does not 
matter to me. I just suggested it since you consider these patches as 
temporary workaround until there are appropriate protocol changes. So clear 
code separation for them might help to get rid of the entire code later on. 
Plus for distribution maintainers it might be easiert to cherry pick them as 
backports.

However since I will drop the persistency and "vii" feature in the next patch 
series, it probably does not make a huge difference anyway. As you prefer.

> The 9p code has a long history of CVEs and limitations that prevented it
> to reach full production grade quality. Combined with the poor quality of
> the code, this has scared off many experienced QEMU developpers, which
> prefer to work on finding an alternative solution. 

And I already wondered about the obvious low activity on this particular qemu 
feature. I mean I don't find it contemporary still running guests to use their 
own file system being emulated on a file ontop of yet another file system and 
loosing essentially all benefits of the host's actual backend file system 
features.

> Such alternative is virtio-fs, which is being actively worked on:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02746.html
> 
> Note: I'm not telling "stay away from 9p" but maybe worth taking a look,
>       because if virtio-fs gets merged, it is likely to become the official
>       and better supported way to share files between host and guest.

Ah, good to know! That's new to me, thanks!

Makes sense to me, especially its performance will certainly be much better.

> Please repost a series, possibly based on some of Antonios's patches that
> allows to avoid the QID collision, returns an error to the client instead
> and possibly printing out some useful messages in the QEMU log. Then, on
> top of that, you can start introducing hashing and variable prefix length.

So you want that as its own patch series first, or can I continue with my 
suggestion to deliver the hash patch and variable suffix length patch as last 
patches within the same series?

Best regards,
Christian Schoenebeck

On Wed, 22 May 2019 18:03:13 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Montag, 20. Mai 2019 16:05:09 CEST Greg Kurz wrote:
> > Hi Christian,  
> 
> Hi Greg,
> 
> > On the other hand, I'm afraid that having a functional solution won't
> > motivate people to come up with a new spec... Anyway, I agree that the
> > data corruption/loss issues must be prevented, ie, the 9p server should
> > at least return an error to the client rather than returning a colliding
> > QID.   
> 
> Ok, I will extend Antonios' patch to log that error on host. I thought about 
> limiting that error message to once per session (for not flooding the logs), 
> but it is probably not worth it, so if you don't veto then I will just log 
> that error simply on every file access.
> 

Please use error_report_once().

> > A simple way to avoid that is to enforce a single device, ie. patch
> > 2 in Antonios's series. Of course this may break some setups where
> > multiple devices are involved, but it is pure luck if they aren't already
> > broken with the current code base.   
> 
> Yes, the worst thing you can have is this collision silently being ignored, 
> like it is actually right now. Because you end up getting all sorts of 
> different misbehaviours on guests, and these are just symptoms that take a 
> while to debug and realise that the actual root cause is an inode collision. 
> So enforcing a single device is still better than undefined behaviour.
> 
> > > Background: The concept of FS "data sets" combines the benefits of
> > > classical partitions (e.g. logical file space separation, independent fs
> > > configurations like compression on/off/algorithm, data deduplication
> > > on/off, snapshot isolation, snapshots on/off) without the disadvantages
> > > of classical real partitions (physical space is dynamically shared, no
> > > space wasted on fixed boundaries; physical device pool management is
> > > transparent for all data sets, configuration options can be inherited
> > > from parent data sets).  
> > 
> > Ok. I must admit my ignorance around ZFS and "data sets"... so IIUC, even
> > with a single underlying physical device you might end up with lstat()
> > returning different st_dev values on the associated files, correct ?
> > 
> > I take your word for the likeliness of the issue to popup in such setups. :)  
> 
> Yes, that is correct, you _always_ get a different stat::st_dev value for each 
> ZFS data set. Furthermore, each ZFS data set has its own inode number sequence 
> generator starting from one. So consider you create two new ZFS data sets, 
> then you create one file on each data set, then both files will have inode 
> number 1.
> 
> That probably makes it clear why you hit this ID collision bug very easily 
> when using the combination ZFS & 9p.
> 
> > > also a big difference giving the user the _optional_ possibility to define
> > > e.g. one path (not device) on guest said to be sensitive regarding high
> > > inode numbers on guest; and something completely different telling the
> > > user that he _must_ configure every single device from host that is ever
> > > supposed to pop up with 9p on guest and forcing the user to update that
> > > configuration whenever a new device is added or removed on host. The
> > > "vii" configuration feature does not require any knowledge of how many
> > > and which kinds of devices are actually ever used on host (nor on any
> > > higher level host in case of nested
> > > virtualization), nor does that "vii" config require any changes ever when
> > > host device setup changes. So 9p's core transparency feature would not be
> > > touched at all.  
> > 
> > I guess this all boils down to I finding some time to read/understand more
> > :)  
> 
> Yes, that helps sometimes. :)
> 
> > As usual, a series with smaller and simpler patches will be easier to
> > review, and more likely to be merged.  
> 
> Of course.
> 
> In the next patch series I will completely drop a) the entire QID persistency 
> feature code and b) that disputed "vii" feature. But I will still suggest the 
> variable inode suffix length patch as last patch in that new patch series.
> 
> That should make the amount of changes manageable small.
> 
> > > Let me make a different suggestion: how about putting these fixes into a
> > > separate C unit for now and making the default behaviour (if you really
> > > want) to not use any of that code by default at all. So the user would
> > > just get an error message in the qemu log files by default if he tries to
> > > export several devices with one 9p device, suggesting him either to map
> > > them as separate 9p devices (like you suggested) and informing the user
> > > about the alternative of enabling support for the automatic inode
> > > remapping code (from that separate C unit) instead by adding one
> > > convenient config option if he/she really wants.  
> > It seems that we may be reaching some consensus here :)
> > 
> > I like the approach, provided this is configurable from the command line,
> > off by default and doesn't duplicate core 9p code. Not sure this needs to
> > sit in its own C unit though.  
> 
> Any preference for a command line argument name and/or libvirt XML config tag/
> attribute for switching the inode remapping code on?
> 
> About that separate C unit: I leave that up to you to decide, it does not 
> matter to me. I just suggested it since you consider these patches as 
> temporary workaround until there are appropriate protocol changes. So clear 
> code separation for them might help to get rid of the entire code later on. 
> Plus for distribution maintainers it might be easiert to cherry pick them as 
> backports.
> 
> However since I will drop the persistency and "vii" feature in the next patch 
> series, it probably does not make a huge difference anyway. As you prefer.
> 
> > The 9p code has a long history of CVEs and limitations that prevented it
> > to reach full production grade quality. Combined with the poor quality of
> > the code, this has scared off many experienced QEMU developpers, which
> > prefer to work on finding an alternative solution.   
> 
> And I already wondered about the obvious low activity on this particular qemu 
> feature. I mean I don't find it contemporary still running guests to use their 
> own file system being emulated on a file ontop of yet another file system and 
> loosing essentially all benefits of the host's actual backend file system 
> features.
> 
> > Such alternative is virtio-fs, which is being actively worked on:
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02746.html
> > 
> > Note: I'm not telling "stay away from 9p" but maybe worth taking a look,
> >       because if virtio-fs gets merged, it is likely to become the official
> >       and better supported way to share files between host and guest.  
> 
> Ah, good to know! That's new to me, thanks!
> 
> Makes sense to me, especially its performance will certainly be much better.
> 
> > Please repost a series, possibly based on some of Antonios's patches that
> > allows to avoid the QID collision, returns an error to the client instead
> > and possibly printing out some useful messages in the QEMU log. Then, on
> > top of that, you can start introducing hashing and variable prefix length.  
> 
> So you want that as its own patch series first, or can I continue with my 
> suggestion to deliver the hash patch and variable suffix length patch as last 
> patches within the same series?
> 

Same series is okay.

> Best regards,
> Christian Schoenebeck

Cheers,

--
Greg

On Montag, 3. Juni 2019 08:57:15 CEST Greg Kurz wrote:
> > Ok, I will extend Antonios' patch to log that error on host. I thought
> > about limiting that error message to once per session (for not flooding
> > the logs), but it is probably not worth it, so if you don't veto then I
> > will just log that error simply on every file access.
> 
> Please use error_report_once().

I will.

> > > Please repost a series, possibly based on some of Antonios's patches
> > > that
> > > allows to avoid the QID collision, returns an error to the client
> > > instead
> > > and possibly printing out some useful messages in the QEMU log. Then, on
> > > top of that, you can start introducing hashing and variable prefix
> > > length.
> > 
> > So you want that as its own patch series first, or can I continue with my
> > suggestion to deliver the hash patch and variable suffix length patch as
> > last patches within the same series?
> 
> Same series is okay.

Ok.

I'm currently busy with other work; I will probably send a new patch set 
approximately next week.

Best regards,
Christian Schoenebeck