[PATCH 0 of 3] blktap3/libxl: add support for blktap3 in libxl

[PATCH 0 of 3] blktap3/libxl: add support for blktap3 in libxl

[Thanos Makatos]

This patch series implements support for blktap3 in libxl.

Supporting blktap3 requires rather few changes:
1. We introduce a new disk back-end type (TAP3) and a new device kind (VBD3) to
   allow blktap3 to co-exist with blktap3. blktap2 remains the default back-end
   for tap devices. Switching in the future to blktap3 as the default handler
   of tap devices should be trivial.
2. libxl doesn't spawn the tapdisk process any more, as is the case for
   blktap2, since the tapback daemon is responsible for that. Thus, libxl only
   needs to write to XenStore the file/partition/whatever backing the virtual
   disk so that the tapback daemon can pass it to tapdisk.
3. Since there is no block device in dom0 any more, pygrub won't be able to
   boot from VHD files. To get around this problem, pygrub can use the NBD
   functionality (existing in blktap2.5): it can explicitly ask tapback to
   create a NDB in order to access the virtual disk. This functionality will be
   implemented in a future patch series. However, this will only work for Linux
   dom0's. As a generic solution that would work on any dom0, we could
   implement a simple protocol for data exchange between pygrub and tapdisk.

Signed-off-by: Thanos Makatos <thanos.makatos@citrix.com>

[PATCH 1 of 3] blktap3/libxl: add new device kind and disk back-end

[Thanos Makatos]

We use new identifiers so that blktap2 and blktap3 can co-exist.

diff -r 6c1b12c884b4 -r dd920505264c tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl	Tue Feb 05 15:47:41 2013 +0000
+++ b/tools/libxl/libxl_types.idl	Fri Feb 08 17:23:23 2013 +0000
@@ -58,6 +58,7 @@ libxl_disk_backend = Enumeration("disk_b
     (1, "PHY"),
     (2, "TAP"),
     (3, "QDISK"),
+    (4, "TAP3"),
     ])
 
 libxl_nic_type = Enumeration("nic_type", [
diff -r 6c1b12c884b4 -r dd920505264c tools/libxl/libxl_types_internal.idl
--- a/tools/libxl/libxl_types_internal.idl	Tue Feb 05 15:47:41 2013 +0000
+++ b/tools/libxl/libxl_types_internal.idl	Fri Feb 08 17:23:23 2013 +0000
@@ -20,6 +20,7 @@ libxl__device_kind = Enumeration("device
     (6, "VKBD"),
     (7, "CONSOLE"),
     (8, "VTPM"),
+    (9, "VBD3"),
     ])
 
 libxl__console_backend = Enumeration("console_backend", [
diff -r 6c1b12c884b4 -r dd920505264c tools/libxl/libxlu_disk_l.c
--- a/tools/libxl/libxlu_disk_l.c	Tue Feb 05 15:47:41 2013 +0000
+++ b/tools/libxl/libxlu_disk_l.c	Fri Feb 08 17:23:23 2013 +0000
@@ -852,6 +852,7 @@ static void setformat(DiskParseContext *
 static void setbackendtype(DiskParseContext *dpc, const char *str) {
     if (     !strcmp(str,"phy"))   DSET(dpc,backend,BACKEND,str,PHY);
     else if (!strcmp(str,"tap"))   DSET(dpc,backend,BACKEND,str,TAP);
+    else if (!strcmp(str,"tap3"))   DSET(dpc,backend,BACKEND,str,TAP3);
     else if (!strcmp(str,"qdisk")) DSET(dpc,backend,BACKEND,str,QDISK);
     else xlu__disk_err(dpc,str,"unknown value for backendtype");
 }
diff -r 6c1b12c884b4 -r dd920505264c tools/libxl/libxlu_disk_l.l
--- a/tools/libxl/libxlu_disk_l.l	Tue Feb 05 15:47:41 2013 +0000
+++ b/tools/libxl/libxlu_disk_l.l	Fri Feb 08 17:23:23 2013 +0000
@@ -109,6 +109,7 @@ static void setformat(DiskParseContext *
 static void setbackendtype(DiskParseContext *dpc, const char *str) {
     if (     !strcmp(str,"phy"))   DSET(dpc,backend,BACKEND,str,PHY);
     else if (!strcmp(str,"tap"))   DSET(dpc,backend,BACKEND,str,TAP);
+    else if (!strcmp(str,"tap3"))   DSET(dpc,backend,BACKEND,str,TAP3);
     else if (!strcmp(str,"qdisk")) DSET(dpc,backend,BACKEND,str,QDISK);
     else xlu__disk_err(dpc,str,"unknown value for backendtype");
 }

[PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

This patch implements function libxl__blktap3_enabled, the equivalent of the
existing libxl__blktap_enabled for blktap2. The checks performed are rather
simple and should be extended.

diff -r dd920505264c -r dd63f2992e71 tools/libxl/Makefile
--- a/tools/libxl/Makefile	Fri Feb 08 17:23:23 2013 +0000
+++ b/tools/libxl/Makefile	Fri Feb 08 17:23:25 2013 +0000
@@ -37,8 +37,10 @@ LIBXLU_LIBS =
 LIBXL_OBJS-y = osdeps.o libxl_paths.o libxl_bootloader.o flexarray.o
 ifeq ($(LIBXL_BLKTAP),y)
 LIBXL_OBJS-y += libxl_blktap2.o
+LIBXL_OBJS-y += libxl_blktap3.o
 else
 LIBXL_OBJS-y += libxl_noblktap2.o
+LIBXL_OBJS-y += libxl_noblktap3.o
 endif
 LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o
 LIBXL_OBJS-$(CONFIG_IA64) += libxl_nocpuid.o libxl_noarch.o
diff -r dd920505264c -r dd63f2992e71 tools/libxl/libxl_blktap3.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_blktap3.c	Fri Feb 08 17:23:25 2013 +0000
@@ -0,0 +1,60 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+
+/* FIXME get the tapback name from blktap3 instead of hard-coding */
+#define TAPBACK_NAME "tapback"
+#define CMD "pidof " TAPBACK_NAME
+
+/*
+ * Simple sanity checks. Most of these checks are not race-free (e.g. checking
+ * wether the tapdisk binary exists), but at least we get some protection
+ * against spectacularly silly mistakes.
+ *
+ * We don't check whether the tapdisk binary exists as this is done by the
+ * tapback daemon.
+ */
+int libxl__blktap3_enabled(libxl__gc *gc)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    int err;
+
+    /*
+     * Check whether the tapback daemon is running.
+     */
+    err = system(CMD);
+    if (err == -1) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                "failed to check whether the tapback daemon is running\n");
+        return 0;
+    }
+    err = WEXITSTATUS(err);
+    if (err != 0) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "tapback daemon not running\n");
+        return 0;
+    }
+
+    /*
+     * TODO Check for evtchn, gntdev. How do we do that!?
+     */
+
+    return 1;
+}
diff -r dd920505264c -r dd63f2992e71 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Fri Feb 08 17:23:23 2013 +0000
+++ b/tools/libxl/libxl_internal.h	Fri Feb 08 17:23:25 2013 +0000
@@ -1337,6 +1337,14 @@ struct libxl__cpuid_policy {
 };
 
 /*
+ * blktap3 support
+ */
+/* libxl__blktap_enabled:
+ *    return true if blktap3 support is available.
+ */
+_hidden int libxl__blktap3_enabled(libxl__gc *gc);
+
+/*
  * blktap2 support
  */
 
diff -r dd920505264c -r dd63f2992e71 tools/libxl/libxl_noblktap3.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/tools/libxl/libxl_noblktap3.c	Fri Feb 08 17:23:25 2013 +0000
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 2012      Citrix Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * 
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ * 
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
+ * USA.
+ */
+
+#include "libxl_osdeps.h" /* must come before any other headers */
+#include "libxl_internal.h"
+
+int libxl__blktap3_enabled(libxl__gc *gc)
+{
+    return 0;
+}

[PATCH 3 of 3] blktap3/libxl: Handles blktap3 device in libxl

[Thanos Makatos]

Handling of blktap3 devices is similar to blktap2, except that libxl doesn't
spawn the tapdisk and doesn't create the physical device in dom0.

diff -r dd63f2992e71 -r 6b54db4abe12 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Fri Feb 08 17:23:25 2013 +0000
+++ b/tools/libxl/libxl.c	Fri Feb 08 17:23:26 2013 +0000
@@ -1158,6 +1158,8 @@ static void disk_eject_xswatch_callback(
         disk->backend = LIBXL_DISK_BACKEND_TAP;
     } else if (!strcmp(backend_type, "qdisk")) {
         disk->backend = LIBXL_DISK_BACKEND_QDISK;
+    } else if (!strcmp(backend_type, "tap3")) {
+        disk->backend = LIBXL_DISK_BACKEND_TAP3;
     } else {
         disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
     }
@@ -1998,6 +2000,9 @@ int libxl__device_from_disk(libxl__gc *g
         case LIBXL_DISK_BACKEND_QDISK:
             device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
             break;
+        case LIBXL_DISK_BACKEND_TAP3:
+            device->backend_kind = LIBXL__DEVICE_KIND_VBD3;
+            break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
                        disk->backend);
@@ -2113,6 +2118,12 @@ static void device_disk_add(libxl__egc *
 
                 /* now create a phy device to export the device to the guest */
                 goto do_backend_phy;
+            case LIBXL_DISK_BACKEND_TAP3:
+                flexarray_append(back, "params");
+                flexarray_append(back, libxl__sprintf(gc, "%s:%s",
+                    libxl__device_disk_string_of_format(disk->format),
+                    disk->pdev_path));
+                break;
             case LIBXL_DISK_BACKEND_QDISK:
                 flexarray_append(back, "params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
diff -r dd63f2992e71 -r 6b54db4abe12 tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c	Fri Feb 08 17:23:25 2013 +0000
+++ b/tools/libxl/libxl_device.c	Fri Feb 08 17:23:26 2013 +0000
@@ -190,6 +190,23 @@ static int disk_try_backend(disk_try_bac
         }
         return backend;
 
+    case LIBXL_DISK_BACKEND_TAP3:
+        /* TODO What's that script thing? */
+        if (a->disk->script) goto bad_script;
+
+        if (!libxl__blktap3_enabled(a->gc)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
+                       " unsuitable because blktap3 not available",
+                       a->disk->vdev);
+            return 0;
+        }
+        /* TODO other formats supported by blktap3? */
+        if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
+              a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
+            goto bad_format;
+        }
+        return backend;
+
     case LIBXL_DISK_BACKEND_QDISK:
         if (a->disk->script) goto bad_script;
         return backend;
@@ -258,6 +275,7 @@ int libxl__device_disk_set_backend(libxl
         ok=
             disk_try_backend(&a, LIBXL_DISK_BACKEND_PHY) ?:
             disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP) ?:
+            disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP3) ?:
             disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK);
         if (ok)
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend %s",
@@ -290,6 +308,7 @@ char *libxl__device_disk_string_of_backe
     switch (backend) {
         case LIBXL_DISK_BACKEND_QDISK: return "qdisk";
         case LIBXL_DISK_BACKEND_TAP: return "phy";
+        case LIBXL_DISK_BACKEND_TAP3: return "phy";
         case LIBXL_DISK_BACKEND_PHY: return "phy";
         default: return NULL;
     }

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Ian Jackson]

Thanos Makatos writes ("[Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available"):
> This patch implements function libxl__blktap3_enabled, the equivalent of the
> existing libxl__blktap_enabled for blktap2. The checks performed are rather
> simple and should be extended.
...
> +/* FIXME get the tapback name from blktap3 instead of hard-coding */
> +#define TAPBACK_NAME "tapback"
> +#define CMD "pidof " TAPBACK_NAME

I'm afraid I'm fundamentally unhappy with this approach to detecting
the availability of blktap3.  Searching the process table for process
with particular names is not a good idea.

Can't you try to connect to its control socket ?

Ian.

Re: [PATCH 3 of 3] blktap3/libxl: Handles blktap3 device in libxl

[Ian Jackson]

Thanos Makatos writes ("[Xen-devel] [PATCH 3 of 3] blktap3/libxl: Handles blktap3 device in libxl"):
> Handling of blktap3 devices is similar to blktap2, except that libxl doesn't
> spawn the tapdisk and doesn't create the physical device in dom0.
...
> +            case LIBXL_DISK_BACKEND_TAP3:
> +                flexarray_append(back, "params");
> +                flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> +                    libxl__device_disk_string_of_format(disk->format),
> +                    disk->pdev_path));
> +                break;
>              case LIBXL_DISK_BACKEND_QDISK:
>                  flexarray_append(back, "params");
>                  flexarray_append(back, libxl__sprintf(gc, "%s:%s",

This seems to duplicate much of the entry for
LIBXL_DISK_BACKEND_QDISK.  I think this could be profitably
reorganised to avoid clone-and-hack.

> diff -r dd63f2992e71 -r 6b54db4abe12 tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c	Fri Feb 08 17:23:25 2013 +0000
> +++ b/tools/libxl/libxl_device.c	Fri Feb 08 17:23:26 2013 +0000
> @@ -190,6 +190,23 @@ static int disk_try_backend(disk_try_bac
>          }
>          return backend;
>  
> +    case LIBXL_DISK_BACKEND_TAP3:
> +        /* TODO What's that script thing? */
> +        if (a->disk->script) goto bad_script;

Is this series supposed to be an RFC ?  It still has the odd TODO and
FIXME in it.

A disk script is an external script which arranges to provision a
block device for use by guests (and dom0).  It's an alternative to
TAP3 so I think you can just delete the comment.

Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

> > +/* FIXME get the tapback name from blktap3 instead of hard-coding */
> > +#define TAPBACK_NAME "tapback"
> > +#define CMD "pidof " TAPBACK_NAME
> 
> I'm afraid I'm fundamentally unhappy with this approach to detecting
> the availability of blktap3.  Searching the process table for process
> with particular names is not a good idea.
> 
> Can't you try to connect to its control socket ?

Sure.

Re: [PATCH 3 of 3] blktap3/libxl: Handles blktap3 device in libxl

[Thanos Makatos]

> ...
> > +            case LIBXL_DISK_BACKEND_TAP3:
> > +                flexarray_append(back, "params");
> > +                flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> > +                    libxl__device_disk_string_of_format(disk-
> >format),
> > +                    disk->pdev_path));
> > +                break;
> >              case LIBXL_DISK_BACKEND_QDISK:
> >                  flexarray_append(back, "params");
> >                  flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> 
> This seems to duplicate much of the entry for LIBXL_DISK_BACKEND_QDISK.
> I think this could be profitably reorganised to avoid clone-and-hack.

You're right, though I think I missed something: tapback spawns tapdisk when the front-end starts running the Xenbus protocol, so if it takes too long for tapback to spawn the tapdisk (e.g. the host is over-loaded) the domain creation will time out. To address this, libxl could tell tapback it's going to need a tapdisk on a particular file/disk/whatever (it will also have to explicitly tell tapback that it doesn't need the tapdisk anymore). Does this sound plausible?

> 
> > diff -r dd63f2992e71 -r 6b54db4abe12 tools/libxl/libxl_device.c
> > --- a/tools/libxl/libxl_device.c	Fri Feb 08 17:23:25 2013 +0000
> > +++ b/tools/libxl/libxl_device.c	Fri Feb 08 17:23:26 2013 +0000
> > @@ -190,6 +190,23 @@ static int disk_try_backend(disk_try_bac
> >          }
> >          return backend;
> >
> > +    case LIBXL_DISK_BACKEND_TAP3:
> > +        /* TODO What's that script thing? */
> > +        if (a->disk->script) goto bad_script;
> 
> Is this series supposed to be an RFC ?  It still has the odd TODO and
> FIXME in it.

No it's not an RFC. Some TODOs are there as guides for possible improvements/optimisations.

> 
> A disk script is an external script which arranges to provision a block
> device for use by guests (and dom0).  It's an alternative to
> TAP3 so I think you can just delete the comment.

I'll update the comment.

> 
> Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

> Thanos Makatos writes ("[Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check
> whether blktap3 is available"):
> > This patch implements function libxl__blktap3_enabled, the equivalent
> > of the existing libxl__blktap_enabled for blktap2. The checks
> > performed are rather simple and should be extended.
> ...
> > +/* FIXME get the tapback name from blktap3 instead of hard-coding */
> > +#define TAPBACK_NAME "tapback"
> > +#define CMD "pidof " TAPBACK_NAME
> 
> I'm afraid I'm fundamentally unhappy with this approach to detecting
> the availability of blktap3.  Searching the process table for process
> with particular names is not a good idea.
> 
> Can't you try to connect to its control socket ?
> 
> Ian.

There's no control socket yet. Could we leave this as a future improvement?

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Ian Jackson]

Thanos Makatos writes ("RE: [Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is	available"):
> > Thanos Makatos writes ("[Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check
> > whether blktap3 is available"):
> > > This patch implements function libxl__blktap3_enabled, the equivalent
> > > of the existing libxl__blktap_enabled for blktap2. The checks
> > > performed are rather simple and should be extended.
> > ...
> > > +/* FIXME get the tapback name from blktap3 instead of hard-coding */
> > > +#define TAPBACK_NAME "tapback"
> > > +#define CMD "pidof " TAPBACK_NAME
> > 
> > I'm afraid I'm fundamentally unhappy with this approach to detecting
> > the availability of blktap3.  Searching the process table for process
> > with particular names is not a good idea.
> > 
> > Can't you try to connect to its control socket ?
...
> There's no control socket yet. Could we leave this as a future improvement?

How do you control this thing then ?  Does it listen on xenstore ?

In any case, you need to do this test in some way which is reliable.

The reason why searching the process table for processes with
particular names is not a good idea is that there is no rule which
says that other subsystems aren't allowed to create processes with any
names they like.

Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

> > > Can't you try to connect to its control socket ?
> ...
> > There's no control socket yet. Could we leave this as a future
> improvement?
> 
> How do you control this thing then ?  Does it listen on xenstore ?
> 
> In any case, you need to do this test in some way which is reliable.
> 
> The reason why searching the process table for processes with
> particular names is not a good idea is that there is no rule which says
> that other subsystems aren't allowed to create processes with any names
> they like.
> 
> Ian.

Tapback watches XenStore, so it doesn't need the control socket to function. However, the control socket will be necessary for doing more elaborate stuff (e.g. controlling a tapdisk in a thread-safe manner). I understand the problems of using a processes' name in this case, but since the control socket is not absolutely necessary at this point (it's actually not yet implemented in the tapback daemon), could we leave this as a future improvement?

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Ian Jackson]

Thanos Makatos writes ("RE: [Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is	available"):
> Tapback watches XenStore, so it doesn't need the control socket to
> function.

So if all you're doing is seeing whether it's available, surely you
can have it write something in a fixed location in xenstore on startup ?

> However, the control socket will be necessary for doing
> more elaborate stuff (e.g. controlling a tapdisk in a thread-safe
> manner).

I'm not sure I understand this comment.  What is not thread-safe about
controlling it via xenstore ?

> I understand the problems of using a processes' name in
> this case, but since the control socket is not absolutely necessary
> at this point (it's actually not yet implemented in the tapback
> daemon), could we leave this as a future improvement?

On the whole I think it would be better not to introduce bugs on the
vague promise of fixing them later "as a future improvement" ...

Thanks,
Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

> So if all you're doing is seeing whether it's available, surely you can
> have it write something in a fixed location in xenstore on startup ?
> 
> > However, the control socket will be necessary for doing more
> elaborate
> > stuff (e.g. controlling a tapdisk in a thread-safe manner).
> 
> I'm not sure I understand this comment.  What is not thread-safe about
> controlling it via xenstore ?

There are some operations that need to be performed on running tapdisks that do not involve XenStore, e.g. refreshing a tapdisk as the result of a VHD snapshot operation.

> 
> > I understand the problems of using a processes' name in this case,
> but
> > since the control socket is not absolutely necessary at this point
> > (it's actually not yet implemented in the tapback daemon), could we
> > leave this as a future improvement?
> 
> On the whole I think it would be better not to introduce bugs on the
> vague promise of fixing them later "as a future improvement" ...

Fair enough!

> 
> Thanks,
> Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Ian Jackson]

Thanos Makatos writes ("RE: [Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is	available"):
...
> > I'm not sure I understand this comment.  What is not thread-safe about
> > controlling it via xenstore ?
> 
> There are some operations that need to be performed on running tapdisks that do not involve XenStore, e.g. refreshing a tapdisk as the result of a VHD snapshot operation.

You mean "there are some operations whose control path will be outside
xenstore in the current design", I think ?

How will these control operations be invoked when the tapdisk is
running in a driver/stub/backend domain rather than the same domain as
the main toolstack ?

Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

> > There are some operations that need to be performed on running
> tapdisks that do not involve XenStore, e.g. refreshing a tapdisk as the
> result of a VHD snapshot operation.
> 
> You mean "there are some operations whose control path will be outside
> xenstore in the current design", I think ?

Yes.

> 
> How will these control operations be invoked when the tapdisk is
> running in a driver/stub/backend domain rather than the same domain as
> the main toolstack ?

I haven't given much thought to that, but I believe there are a number of viable options. We could use XenStore, though I think we should avoid doing that for performance reasons. I assume the tapdisk(s) will be running on the same domain the tapback daemon is, so we can make tapback listen on a socket in order that the toolstack running on another domain can talk to it. Does this sound plausible?

> 
> Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Ian Jackson]

Thanos Makatos writes ("RE: [Xen-devel] [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is	available"):
> I haven't given much thought to that, but I believe there are a
> number of viable options. We could use XenStore, though I think we
> should avoid doing that for performance reasons. I assume the
> tapdisk(s) will be running on the same domain the tapback daemon is,
> so we can make tapback listen on a socket in order that the
> toolstack running on another domain can talk to it. Does this sound
> plausible?

Aren't you positing some kind of interdomain socket here ?

Ian.

Re: [PATCH 2 of 3] blktap3/libxl: Check whether blktap3 is available

[Thanos Makatos]

> > I haven't given much thought to that, but I believe there are a
> number
> > of viable options. We could use XenStore, though I think we should
> > avoid doing that for performance reasons. I assume the
> > tapdisk(s) will be running on the same domain the tapback daemon is,
> > so we can make tapback listen on a socket in order that the toolstack
> > running on another domain can talk to it. Does this sound plausible?
> 
> Aren't you positing some kind of interdomain socket here ?

Yes. Have there been any discussions on how service domains can/should communicate?

> 
> Ian.

[PATCH 3 of 3] blktap3/libxl: Handles blktap3 device in libxl

[Thanos Makatos]

Handling of blktap3 devices is similar to blktap2, except that libxl doesn't
spawn the tapdisk and doesn't create the physical device in dom0.

Signed-off-by: Thanos Makatos <thanos.makatos@citrix.com>

---
Changed since v1:
  * Don't duplicate code for writing the type:/path/to/file to XenStore.

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1159,6 +1159,8 @@ static void disk_eject_xswatch_callback(
         disk->backend = LIBXL_DISK_BACKEND_TAP;
     } else if (!strcmp(backend_type, "qdisk")) {
         disk->backend = LIBXL_DISK_BACKEND_QDISK;
+    } else if (!strcmp(backend_type, "tap3")) {
+        disk->backend = LIBXL_DISK_BACKEND_TAP3;
     } else {
         disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
     }
@@ -2012,6 +2014,9 @@ int libxl__device_from_disk(libxl__gc *g
         case LIBXL_DISK_BACKEND_QDISK:
             device->backend_kind = LIBXL__DEVICE_KIND_QDISK;
             break;
+        case LIBXL_DISK_BACKEND_TAP3:
+            device->backend_kind = LIBXL__DEVICE_KIND_VBD3;
+            break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n",
                        disk->backend);
@@ -2130,11 +2135,15 @@ static void device_disk_add(libxl__egc *
 
                 /* now create a phy device to export the device to the guest */
                 goto do_backend_phy;
+            case LIBXL_DISK_BACKEND_TAP3:
             case LIBXL_DISK_BACKEND_QDISK:
                 flexarray_append(back, "params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
                               libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
-                assert(device->backend_kind == LIBXL__DEVICE_KIND_QDISK);
+                assert(
+                        (disk->backend == LIBXL_DISK_BACKEND_QDISK
+                         && device->backend_kind == LIBXL__DEVICE_KIND_QDISK)
+                        || (disk->backend == LIBXL_DISK_BACKEND_TAP3));
                 break;
             default:
                 LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk backend type: %d\n", disk->backend);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -190,6 +190,23 @@ static int disk_try_backend(disk_try_bac
         }
         return backend;
 
+    case LIBXL_DISK_BACKEND_TAP3:
+        /* TODO What's that script thing? */
+        if (a->disk->script) goto bad_script;
+
+        if (!libxl__blktap3_enabled(a->gc)) {
+            LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
+                       " unsuitable because blktap3 not available",
+                       a->disk->vdev);
+            return 0;
+        }
+        /* TODO other formats supported by blktap3? */
+        if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
+              a->disk->format == LIBXL_DISK_FORMAT_VHD)) {
+            goto bad_format;
+        }
+        return backend;
+
     case LIBXL_DISK_BACKEND_QDISK:
         if (a->disk->script) goto bad_script;
         return backend;
@@ -258,6 +275,7 @@ int libxl__device_disk_set_backend(libxl
         ok=
             disk_try_backend(&a, LIBXL_DISK_BACKEND_PHY) ?:
             disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP) ?:
+            disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP3) ?:
             disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK);
         if (ok)
             LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend %s",
@@ -290,6 +308,7 @@ char *libxl__device_disk_string_of_backe
     switch (backend) {
         case LIBXL_DISK_BACKEND_QDISK: return "qdisk";
         case LIBXL_DISK_BACKEND_TAP: return "phy";
+        case LIBXL_DISK_BACKEND_TAP3: return "phy";
         case LIBXL_DISK_BACKEND_PHY: return "phy";
         default: return NULL;
     }