Blktap fixes and kernel patch.

Blktap fixes and kernel patch.

[Dr. Greg Wettstein]

Hi, hope the day is going well for everyone.  Just came in from a 90
mile cycle ride and wanted to get this out since 4.1.3 and/or 4.2 are
looming in the near future.

After becoming far more familiar with the XEN vbd infrastructure then
I had intended I was finally able to run down the problem with the
blktap2 driver which has plagued 4.1.2.  Most specifically the
orphaning of the tapdisk2 driver process and after Ian's fix for that,
the hang and orphaning of a tapdisk minor which occurs when libxl
attempts to shutdown the driver process.

In the process I updated the blktap2 kernel driver to patch cleanly
into the Linux 3.4 kernel.  These fixes have been validated against
the 3.4 kernel as well as the 3.2 kernel.

Locations for the files are as follows:

Kernel driver:
	ftp://ftp.enjellic.com/pub/xen/blktap2-3.4.patch

Xen fixes:
	ftp://ftp.enjellic.com/pub/xen/xen-4.1.2.blktap1.patch
	ftp://ftp.enjellic.com/pub/xen/xen-4.1.2.blktap2.patch

The two XEN patches overlay sequentially onto a virgin 4.1.2
distribution and both are required for correct behavior.

The first patch is one which was done by Ian for the development tree
with minor corrections for 4.1.2.  I'm including it for completeness
for those who want a trouble free patch set for a 4.1.2 distribution.
This patch fixes the orphaning of the tapdisk2 driver process when xl
shuts down.

The second patch corrects the deadlock which occurs between the
blktap2 kernel driver and the blktap2 userspace control plane.  The
deadlock causes a delay in the shutdown of a XEN guest and results in
the 'orphaning' of tapdisk minor number allocations.  As seems to be
typical with these types of things the fix was trivially straight
forward once I finally figured out what was going on.

Ian for your reference the following change which you introduced to
address this issue:

79e3dbe4b659e78408a9eea76c51a601bd4a383a
tapdisk: respond to destroy request before tearing down the commuication channel

Is not needed and does not provide formally correct behavior in the
presence of the two patches noted above.

I don't know where all this went in 4.2 since I heard mention that
someone was giving xl a good work over with respect to device
management.  I also don't know if a 4.1.3 is planned, if it is the
above patches go a long ways toward improving the user experience for
those sites which find blktap2 useful.

Now on to NPIV support since it was the reason I waded into the xl/vbd
in the first place.... :-)

Best wishes for a pleasant remainder of the week.

Greg

Cut here for patch 1: -----------------------------------------------------
diff -urNp v4.1.2/xen-4.1.2/tools/blktap2/control/tap-ctl-list.c xen-4.1.2/tools/blktap2/control/tap-ctl-list.c
--- v4.1.2/xen-4.1.2/tools/blktap2/control/tap-ctl-list.c	Thu Oct 20 12:05:41 2011
+++ xen-4.1.2/tools/blktap2/control/tap-ctl-list.c	Thu Jul 26 09:43:10 2012
@@ -506,17 +506,15 @@ out:
 }
 
 int
-tap_ctl_find_minor(const char *type, const char *path)
+tap_ctl_find(const char *type, const char *path, tap_list_t *tap)
 {
 	tap_list_t **list, **_entry;
-	int minor, err;
+	int ret = -ENOENT, err;
 
 	err = tap_ctl_list(&list);
 	if (err)
 		return err;
 
-	minor = -1;
-
 	for (_entry = list; *_entry != NULL; ++_entry) {
 		tap_list_t *entry  = *_entry;
 
@@ -526,11 +524,13 @@ tap_ctl_find_minor(const char *type, con
 		if (path && (!entry->path || strcmp(entry->path, path)))
 			continue;
 
-		minor = entry->minor;
+		*tap = *entry;
+		tap->type = tap->path = NULL;
+		ret = 0;
 		break;
 	}
 
 	tap_ctl_free_list(list);
 
-	return minor >= 0 ? minor : -ENOENT;
+	return ret;
 }
diff -urNp v4.1.2/xen-4.1.2/tools/blktap2/control/tap-ctl.h xen-4.1.2/tools/blktap2/control/tap-ctl.h
--- v4.1.2/xen-4.1.2/tools/blktap2/control/tap-ctl.h	Thu Oct 20 12:05:41 2011
+++ xen-4.1.2/tools/blktap2/control/tap-ctl.h	Thu Jul 26 09:43:10 2012
@@ -76,7 +76,7 @@ int tap_ctl_get_driver_id(const char *ha
 
 int tap_ctl_list(tap_list_t ***list);
 void tap_ctl_free_list(tap_list_t **list);
-int tap_ctl_find_minor(const char *type, const char *path);
+int tap_ctl_find(const char *type, const char *path, tap_list_t *tap);
 
 int tap_ctl_allocate(int *minor, char **devname);
 int tap_ctl_free(const int minor);
diff -urNp v4.1.2/xen-4.1.2/tools/libxl/libxl_blktap2.c xen-4.1.2/tools/libxl/libxl_blktap2.c
--- v4.1.2/xen-4.1.2/tools/libxl/libxl_blktap2.c	Thu Oct 20 12:05:42 2011
+++ xen-4.1.2/tools/libxl/libxl_blktap2.c	Thu Jul 26 09:43:10 2012
@@ -18,6 +18,8 @@
 
 #include "tap-ctl.h"
 
+#include <string.h>
+
 int libxl__blktap_enabled(libxl__gc *gc)
 {
     const char *msg;
@@ -30,12 +32,13 @@ const char *libxl__blktap_devpath(libxl_
 {
     const char *type;
     char *params, *devname = NULL;
-    int minor, err;
+    tap_list_t tap;
+    int err;
 
     type = libxl__device_disk_string_of_format(format);
-    minor = tap_ctl_find_minor(type, disk);
-    if (minor >= 0) {
-        devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d", minor);
+    err = tap_ctl_find(type, disk, &tap);
+    if (err == 0) {
+        devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d", tap.minor);
         if (devname)
             return devname;
     }
@@ -48,4 +51,29 @@ const char *libxl__blktap_devpath(libxl_
     }
 
     return NULL;
+}
+
+
+void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path)
+{
+    char *path, *params, *type, *disk;
+    int err;
+    tap_list_t tap;
+
+    path = libxl__sprintf(gc, "%s/tapdisk-params", be_path);
+    if (!path) return;
+
+    params = libxl__xs_read(gc, XBT_NULL, path);
+    if (!params) return;
+
+    type = params;
+    disk = strchr(params, ':');
+    if (!disk) return;
+
+    *disk++ = '\0';
+
+    err = tap_ctl_find(type, disk, &tap);
+    if (err < 0) return;
+
+    tap_ctl_destroy(tap.id, tap.minor);
 }
diff -urNp v4.1.2/xen-4.1.2/tools/libxl/libxl_device.c xen-4.1.2/tools/libxl/libxl_device.c
--- v4.1.2/xen-4.1.2/tools/libxl/libxl_device.c	Thu Oct 20 12:05:42 2011
+++ xen-4.1.2/tools/libxl/libxl_device.c	Thu Jul 26 09:48:03 2012
@@ -250,6 +250,7 @@ int libxl__device_destroy(libxl_ctx *ctx
     if (!state)
         goto out;
     if (atoi(state) != 4) {
+        libxl__device_destroy_tapdisk(&gc, be_path);
         xs_rm(ctx->xsh, XBT_NULL, be_path);
         goto out;
     }
@@ -368,6 +369,7 @@ int libxl__devices_destroy(libxl_ctx *ct
             }
         }
     }
+    libxl__device_destroy_tapdisk(&gc, be_path);
 out:
     libxl__free_all(&gc);
     return 0;
diff -urNp v4.1.2/xen-4.1.2/tools/libxl/libxl_internal.h xen-4.1.2/tools/libxl/libxl_internal.h
--- v4.1.2/xen-4.1.2/tools/libxl/libxl_internal.h	Thu Oct 20 12:05:43 2011
+++ xen-4.1.2/tools/libxl/libxl_internal.h	Thu Jul 26 09:43:10 2012
@@ -314,6 +314,12 @@ _hidden const char *libxl__blktap_devpat
                                  const char *disk,
                                  libxl_disk_format format);
 
+/* libxl__device_destroy_tapdisk:
+ *   Destroys any tapdisk process associated with the backend represented
+ *   by be_path.
+ */
+_hidden void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path);
+
 _hidden char *libxl__uuid2string(libxl__gc *gc, const libxl_uuid uuid);
 
 struct libxl__xen_console_reader {
diff -urNp v4.1.2/xen-4.1.2/tools/libxl/libxl_noblktap2.c xen-4.1.2/tools/libxl/libxl_noblktap2.c
--- v4.1.2/xen-4.1.2/tools/libxl/libxl_noblktap2.c	Thu Oct 20 12:05:43 2011
+++ xen-4.1.2/tools/libxl/libxl_noblktap2.c	Thu Jul 26 09:43:10 2012
@@ -27,3 +27,7 @@ const char *libxl__blktap_devpath(libxl_
 {
     return NULL;
 }
+
+void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path)
+{
+}
And here for patch1: ------------------------------------------------------


Cut here for patch 2: -----------------------------------------------------
diff -urNp v4.1.2/xen-4.1.2/tools/libxl/libxl_blktap2.c xen-4.1.2/tools/libxl/libxl_blktap2.c
--- v4.1.2/xen-4.1.2/tools/libxl/libxl_blktap2.c	Thu Jul 26 09:43:10 2012
+++ xen-4.1.2/tools/libxl/libxl_blktap2.c	Thu Jul 26 10:04:47 2012
@@ -59,6 +59,7 @@ void libxl__device_destroy_tapdisk(libxl
     char *path, *params, *type, *disk;
     int err;
     tap_list_t tap;
+    libxl_ctx *ctx = libxl__gc_owner(gc);
 
     path = libxl__sprintf(gc, "%s/tapdisk-params", be_path);
     if (!path) return;
@@ -74,6 +75,12 @@ void libxl__device_destroy_tapdisk(libxl
 
     err = tap_ctl_find(type, disk, &tap);
     if (err < 0) return;
+
+    /*
+     * Remove the instance of the backend device to avoid a deadlock with the
+     * removal of the tap device.
+     */
+    xs_rm(ctx->xsh, XBT_NULL, be_path);
 
     tap_ctl_destroy(tap.id, tap.minor);
 }
diff -urNp v4.1.2/xen-4.1.2/tools/libxl/libxl_device.c xen-4.1.2/tools/libxl/libxl_device.c
--- v4.1.2/xen-4.1.2/tools/libxl/libxl_device.c	Thu Jul 26 09:48:03 2012
+++ xen-4.1.2/tools/libxl/libxl_device.c	Thu Jul 26 10:06:20 2012
@@ -250,8 +250,7 @@ int libxl__device_destroy(libxl_ctx *ctx
     if (!state)
         goto out;
     if (atoi(state) != 4) {
-        libxl__device_destroy_tapdisk(&gc, be_path);
-        xs_rm(ctx->xsh, XBT_NULL, be_path);
+	libxl__device_destroy_tapdisk(&gc, be_path);
         goto out;
     }
 
And here for patch 2: -----------------------------------------------------

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"A distributed system is one on which I cannot get any work done because 
 a machine I have never heard of has crashed."
                                -- Leslie Lamport

Re: Blktap fixes and kernel patch.

[Ian Campbell]

On Tue, 2012-07-31 at 22:41 +0100, Dr. Greg Wettstein wrote:

> After becoming far more familiar with the XEN vbd infrastructure then
> I had intended I was finally able to run down the problem with the
> blktap2 driver which has plagued 4.1.2.  Most specifically the
> orphaning of the tapdisk2 driver process and after Ian's fix for that,
> the hang and orphaning of a tapdisk minor which occurs when libxl
> attempts to shutdown the driver process.

Thanks for digging into this Greg.

> In the process I updated the blktap2 kernel driver to patch cleanly
> into the Linux 3.4 kernel.  These fixes have been validated against
> the 3.4 kernel as well as the 3.2 kernel.

Just to be clear this is just a straight forward port, there's no part
of the deadlock fix in here?

> The first patch is one which was done by Ian for the development tree
> with minor corrections for 4.1.2.  I'm including it for completeness
> for those who want a trouble free patch set for a 4.1.2 distribution.
> This patch fixes the orphaning of the tapdisk2 driver process when xl
> shuts down.

This is a fairly straight backport of a patch in unstable?

If you send a mail with a subject "Xen 4.1.x backport request
<commit-id>" explaining which commit it is and CC keir@xen.org &
ian.jackson@eu.citrix.com then we can see about getting this into a
future 4.1.x (perhaps even 4.1.3, not sure which stage of rcs we are at
there).

If the backport is reasonably trivial then there is often no need to
include it but since you have done so you might as well include the
patch for reference.

> The second patch corrects the deadlock which occurs between the
> blktap2 kernel driver and the blktap2 userspace control plane.  The
> deadlock causes a delay in the shutdown of a XEN guest and results in
> the 'orphaning' of tapdisk minor number allocations.  As seems to be
> typical with these types of things the fix was trivially straight
> forward once I finally figured out what was going on.

Thanks for this.

Am I right that the important functional change here is that the xs_rm
needs to come after we read the params node but before tap_ctl_destroy?
Obviously removing the node before calling libxl__device_destroy_tapdisk
is wrong since libxl__device_destroy_tapdisk reads from be_path!

Looking at 4.2.0-rc1 I see that libxl__device_destroy removes the
backend before calling libxl__device_destroy_tapdisk, so I think that a
fix is needed there too.

I'm less sure about the usage in libxl__initiate_device_remove. I wonder
if the call to libxl__device_destroy_tapdisk there needs to move to
device_hotplug_done right after the transaction which cleans up the
backend back?

The code which used to be in libxl__devices_destroy is now in
libxl__initiate_device_remove so I expect that fixing that would be
sufficient.

I'd really appreciate it if you could validate whether 4.2.0-rc1 works
for you or not, I suspect not. We would usually want fix the development
version before considering fixes for the stable branches (even if the
actual patch ends up looking totally different) otherwise we run the
risk of regressions in the next version.

Is there a simple command which will list the leaked tap devices? If so
we can consider adding it to the leak-check phase of the automated tests
(although I'm not sure how much use these make of blktap)

For future reference if you intend for a patch to be applied it is best
to submit it in the form described in
http://wiki.xen.org/wiki/Submitting_Xen_Patches, that is one patch per
email, with a changelog specific to that change and a Signed-off-by. In
this sort of scenario (a patch going to 4.1 which isn't a backport) the
changelog should also mention why the patch isn't a backport.

> Ian for your reference the following change which you introduced to
> address this issue:
> 
> 79e3dbe4b659e78408a9eea76c51a601bd4a383a
> tapdisk: respond to destroy request before tearing down the commuication channel
> 
> Is not needed and does not provide formally correct behavior in the
> presence of the two patches noted above.

Is it incorrect (i.e. should be reverted) or is it just incomplete/not
helpful?

Ian.

Re: Blktap fixes and kernel patch.

[Dr. Greg Wettstein]

On Aug 1, 10:17am, Ian Campbell wrote:
} Subject: Re: Blktap fixes and kernel patch.

Hi Ian et. al, hope your week is proceeding well.

> On Tue, 2012-07-31 at 22:41 +0100, Dr. Greg Wettstein wrote:

> > In the process I updated the blktap2 kernel driver to patch cleanly
> > into the Linux 3.4 kernel.  These fixes have been validated against
> > the 3.4 kernel as well as the 3.2 kernel.

> Just to be clear this is just a straight forward port, there's no
> part of the deadlock fix in here?

The kernel patch is just a forward port to 3.4, there were no issues
with the driver itself that needed to be addressed.

It seemed the community lacks ready access to the kernel driver so
hopefully others will find a solid reference site for the patches
useful.

> > The first patch is one which was done by Ian for the development tree
> > with minor corrections for 4.1.2.  I'm including it for completeness
> > for those who want a trouble free patch set for a 4.1.2 distribution.
> > This patch fixes the orphaning of the tapdisk2 driver process when xl
> > shuts down.

> This is a fairly straight backport of a patch in unstable?

Correct, the only change besides some version skew noise is a change
in the calling convention for the libxl__gc context passed to
libxl__device_destroy_tapdisk().

> If you send a mail with a subject "Xen 4.1.x backport request
> <commit-id>" explaining which commit it is and CC keir@xen.org &
> ian.jackson@eu.citrix.com then we can see about getting this into a
> future 4.1.x (perhaps even 4.1.3, not sure which stage of rcs we are at
> there).
> 
> If the backport is reasonably trivial then there is often no need to
> include it but since you have done so you might as well include the
> patch for reference.

I will forward along the reference and the patch.

> > The second patch corrects the deadlock which occurs between the
> > blktap2 kernel driver and the blktap2 userspace control plane.  The
> > deadlock causes a delay in the shutdown of a XEN guest and results in
> > the 'orphaning' of tapdisk minor number allocations.  As seems to be
> > typical with these types of things the fix was trivially straight
> > forward once I finally figured out what was going on.

> Thanks for this.
> 
> Am I right that the important functional change here is that the xs_rm
> needs to come after we read the params node but before tap_ctl_destroy?
> Obviously removing the node before calling libxl__device_destroy_tapdisk
> is wrong since libxl__device_destroy_tapdisk reads from be_path!

Correct.

I debated a bit about how to do this in the cleanest fashion
possible.  Since the be_path is passed to libxl__device_destroy_tapdisk()
the simplest strategy seemed to be to abstract the libxl_ctx context
and pull the entry from xenstore after the tapdisk-params key was read
from the xenstore.

> Looking at 4.2.0-rc1 I see that libxl__device_destroy removes the
> backend before calling libxl__device_destroy_tapdisk, so I think that a
> fix is needed there too.
> 
> I'm less sure about the usage in libxl__initiate_device_remove. I wonder
> if the call to libxl__device_destroy_tapdisk there needs to move to
> device_hotplug_done right after the transaction which cleans up the
> backend back?
> 
> The code which used to be in libxl__devices_destroy is now in
> libxl__initiate_device_remove so I expect that fixing that would be
> sufficient.

The root cause of the problem is a deadlock between the xen-blkback
and blktap2 drivers when the tapdisk2 user space process requests
unmapping of the ring buffer.  The only thing which saves the kernel
is the select timeout on the IPC channel between libxl and the
userspace process.  That timeout allows xl to proceed forward and
teardown the backend which releases the deadlock but the error results
in orphaning of the tapdisk2 minor.

So the overal fix is pretty straight forward, libxl needs to shutdown
the backend before initiating the teardown of the blktap2 userspace
component of the device.

While my fix vaguely felt like a layering violation it seemed to be
the most correct approach.  Since libxl__device_destroy_tapdisk() is
stubbed out in the non-blktap2 case having the teardown of the backend
there would seem to generically fix the problem.  Provided of course
the function can be provided with the correct context, I'm not looking
at the current code.

> I'd really appreciate it if you could validate whether 4.2.0-rc1
> works for you or not, I suspect not. We would usually want fix the
> development version before considering fixes for the stable branches
> (even if the actual patch ends up looking totally different)
> otherwise we run the risk of regressions in the next version.

I'd be happy to give it a test.  Is there a tarball cut or should I
hone up my Mercurial skills?

> Is there a simple command which will list the leaked tap devices? If
> so we can consider adding it to the leak-check phase of the
> automated tests (although I'm not sure how much use these make of
> blktap)

The tap-ctl tools make managingn all this pretty straight forward.

If you issue the following command:

	tap-ctl list

On a faulty implementation after the startup and shutdown of a blktap2
using guest you will see the orphaned minor.  They steadily increase
as guests startup and shutdown.

> For future reference if you intend for a patch to be applied it is best
> to submit it in the form described in
> http://wiki.xen.org/wiki/Submitting_Xen_Patches, that is one patch per
> email, with a changelog specific to that change and a Signed-off-by. In
> this sort of scenario (a patch going to 4.1 which isn't a backport) the
> changelog should also mention why the patch isn't a backport.

Will do, should have been more methodical in my practice.

It appears that 4.1.2 is not properly cleaning up xenstore.  I'm
chasing that down now and if there are trivial correctness fixes I
will pass them onward.

> > Ian for your reference the following change which you introduced to
> > address this issue:
> > 
> > 79e3dbe4b659e78408a9eea76c51a601bd4a383a
> > tapdisk: respond to destroy request before tearing down the commuication channel
> > 
> > Is not needed and does not provide formally correct behavior in the
> > presence of the two patches noted above.

> Is it incorrect (i.e. should be reverted) or is it just incomplete/not
> helpful?

Its incorrect in that it changes a logically correct implementation
only for the purposes of masking the delay, which in this case, lead
to the discovery of the root cause of the problem.  Arguably libxl
needs a bit better error reporting around all this and the patch
arguably works against that.

I'm currently running without the patch in 4.1.2 and the tapdisk2
devices are performing very nicely.

> Ian.

Let me know if you have any additional questions/issues.

Best wishes for a pleasant weekend.

Greg

}-- End of excerpt from Ian Campbell

As always,
Dr. G.W. Wettstein, Ph.D.   Enjellic Systems Development, LLC.
4206 N. 19th Ave.           Specializing in information infra-structure
Fargo, ND  58102            development.
PH: 701-281-1686
FAX: 701-281-3949           EMAIL: greg@enjellic.com
------------------------------------------------------------------------------
"There is no heavier burden than a great potential."
                                -- Linus' Law

Re: Blktap fixes and kernel patch.

[Ian Campbell]

On Thu, 2012-08-02 at 17:06 +0100, Dr. Greg Wettstein wrote:
> On Aug 1, 10:17am, Ian Campbell wrote:
> } Subject: Re: Blktap fixes and kernel patch.
> 
> Hi Ian et. al, hope your week is proceeding well.
> 
> > On Tue, 2012-07-31 at 22:41 +0100, Dr. Greg Wettstein wrote:
> 
> > > In the process I updated the blktap2 kernel driver to patch cleanly
> > > into the Linux 3.4 kernel.  These fixes have been validated against
> > > the 3.4 kernel as well as the 3.2 kernel.
> 
> > Just to be clear this is just a straight forward port, there's no
> > part of the deadlock fix in here?
> 
> The kernel patch is just a forward port to 3.4, there were no issues
> with the driver itself that needed to be addressed.
> 
> It seemed the community lacks ready access to the kernel driver so
> hopefully others will find a solid reference site for the patches
> useful.

I think they will, thanks.

The closest thing we have for an "upstream" for these now is the
blktap-dkms packages which are in Debian and Ubuntu, but I don't think
those really have a maintainer as such (I think Mike, CCd, wrangles with
them a bit). Might that be something you would like to maintain?

[...]
> > If you send a mail with a subject "Xen 4.1.x backport request
> > <commit-id>" explaining which commit it is and CC keir@xen.org &
> > ian.jackson@eu.citrix.com then we can see about getting this into a
> > future 4.1.x (perhaps even 4.1.3, not sure which stage of rcs we are at
> > there).
> > 
> > If the backport is reasonably trivial then there is often no need to
> > include it but since you have done so you might as well include the
> > patch for reference.
> 
> I will forward along the reference and the patch.

Great thanks.

> > > The second patch corrects the deadlock which occurs between the
> > > blktap2 kernel driver and the blktap2 userspace control plane.  The
> > > deadlock causes a delay in the shutdown of a XEN guest and results in
> > > the 'orphaning' of tapdisk minor number allocations.  As seems to be
> > > typical with these types of things the fix was trivially straight
> > > forward once I finally figured out what was going on.
> 
> > Thanks for this.
> > 
> > Am I right that the important functional change here is that the xs_rm
> > needs to come after we read the params node but before tap_ctl_destroy?
> > Obviously removing the node before calling libxl__device_destroy_tapdisk
> > is wrong since libxl__device_destroy_tapdisk reads from be_path!
> 
> Correct.
> 
> I debated a bit about how to do this in the cleanest fashion
> possible.  Since the be_path is passed to libxl__device_destroy_tapdisk()
> the simplest strategy seemed to be to abstract the libxl_ctx context
> and pull the entry from xenstore after the tapdisk-params key was read
> from the xenstore.

[...]
> While my fix vaguely felt like a layering violation it seemed to be
> the most correct approach.  Since libxl__device_destroy_tapdisk() is
> stubbed out in the non-blktap2 case having the teardown of the backend
> there would seem to generically fix the problem.  Provided of course
> the function can be provided with the correct context, I'm not looking
> at the current code.

It was a little more complicated in unstable, because the rm is now in a
transaction. What I ended up doing was making
libxl__device_destroy_tapdisk take the actual params string instead of
the be_path, so I could read it as part of the transaction and then call
destroy_tapdisk.

I've just sent out V2 of that patch, I CCd you. 
<2c21d5c75dcbdf52987f.1343985208@cosworth.uk.xensource.com>

> > I'd really appreciate it if you could validate whether 4.2.0-rc1
> > works for you or not, I suspect not. We would usually want fix the
> > development version before considering fixes for the stable branches
> > (even if the actual patch ends up looking totally different)
> > otherwise we run the risk of regressions in the next version.
> 
> I'd be happy to give it a test.  Is there a tarball cut or should I
> hone up my Mercurial skills?

We don't do tarballs for rcs. In any case I suggest you use the
Mercurial tip anyway since it has the prerequisites for my patch in it
now.

Those prerequisistes are still being tested so you will need the staging
tree:
        hg clone http://xenbits.xen.org/hg/staging/xen-unstable.hg

once they pass testing (this afternoon BST, assuming all is well) then
you can drop the "/staging" bit. We'll probably to rc2 early next week.

> > Is there a simple command which will list the leaked tap devices? If
> > so we can consider adding it to the leak-check phase of the
> > automated tests (although I'm not sure how much use these make of
> > blktap)
> 
> The tap-ctl tools make managingn all this pretty straight forward.
> 
> If you issue the following command:
> 
> 	tap-ctl list
> 
> On a faulty implementation after the startup and shutdown of a blktap2
> using guest you will see the orphaned minor.  They steadily increase
> as guests startup and shutdown.

Thanks, this rings a bell from last time I looked at this.

> [..]

> It appears that 4.1.2 is not properly cleaning up xenstore.  I'm
> chasing that down now and if there are trivial correctness fixes I
> will pass them onward.

Thanks.

> 
> > > Ian for your reference the following change which you introduced to
> > > address this issue:
> > > 
> > > 79e3dbe4b659e78408a9eea76c51a601bd4a383a
> > > tapdisk: respond to destroy request before tearing down the commuication channel
> > > 
> > > Is not needed and does not provide formally correct behavior in the
> > > presence of the two patches noted above.
> 
> > Is it incorrect (i.e. should be reverted) or is it just incomplete/not
> > helpful?
> 
> Its incorrect in that it changes a logically correct implementation
> only for the purposes of masking the delay, which in this case, lead
> to the discovery of the root cause of the problem.  Arguably libxl
> needs a bit better error reporting around all this and the patch
> arguably works against that.
> 
> I'm currently running without the patch in 4.1.2 and the tapdisk2
> devices are performing very nicely.

I remember this patch now but I can't seem to find it in any tree, I
suspect it was never applied, which sounds like a good thing.

> Best wishes for a pleasant weekend.

Thanks, and you.

Ian.