Re: [Xen-devel] Bug in Xen 4.1.0: Xen leaks tapdisk2 processes

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

(add Andreas back, please don't drop cc's on xen-devel)

On Fri, 2011-04-29 at 20:39 +0100, Nathan March wrote:
> > in Xen 4.1.0 using "tap2:aio:/dev/sdaX" Xen leaks tapdisk2 processes.
> > After running some VMs and terminating them, only dom0 running, "ps
> > aux" shows:
> 
> I can definitely confirm this, on a machine with 2 active VMs:

I'm assuming this is xl specific and that xend isn't behaving this way
too, is that correct?

The following fixes it for xl, although I'm not exactly thrilled with
the solution.

Calling libxl__device_destroy_tapdisk from the generic
libxl__device_destroy is a bit skanky but is needed so that
libxl__devices_destroy (which just iterates over all device entries in
xenstore) also does the right thing.

The change to the tapctrl library API was necessary to expose the "id"
parameter necessary for tap_ctl_destroy.

I also seem to be seeing an inexplicable delay at the point at which the
tapdisk2 process is shutdown. I'm not sure where to look for clues as to
what happens there.

I know Ian J is looking at improving disk handling for 4.2 but I was
hoping for something somewhat backportable to 4.1 at this stage.

Does it work for you guys?

8<----------------------------

# HG changeset patch
# User Ian Campbell <ian.campbell@xxxxxxxxxx>
# Date 1304438002 -3600
# Node ID 4d0e906543dc115b5b2f90e0cb44f6affb3f1f99
# Parent  66f006f9ed34e58c10a56026bde41c116e680c8b
libxl: attempt to cleanup tapdisk processes on disk backend destroy.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

diff -r 66f006f9ed34 -r 4d0e906543dc tools/blktap2/control/tap-ctl-list.c
--- a/tools/blktap2/control/tap-ctl-list.c      Tue May 03 15:03:29 2011 +0100
+++ b/tools/blktap2/control/tap-ctl-list.c      Tue May 03 16:53:22 2011 +0100
@@ -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 -r 66f006f9ed34 -r 4d0e906543dc tools/blktap2/control/tap-ctl.h
--- a/tools/blktap2/control/tap-ctl.h   Tue May 03 15:03:29 2011 +0100
+++ b/tools/blktap2/control/tap-ctl.h   Tue May 03 16:53:22 2011 +0100
@@ -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 -r 66f006f9ed34 -r 4d0e906543dc tools/libxl/libxl_blktap2.c
--- a/tools/libxl/libxl_blktap2.c       Tue May 03 15:03:29 2011 +0100
+++ b/tools/libxl/libxl_blktap2.c       Tue May 03 16:53:22 2011 +0100
@@ -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 @@ char *libxl__blktap_devpath(libxl__gc *g
 {
     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;
     }
@@ -49,3 +52,28 @@ char *libxl__blktap_devpath(libxl__gc *g
 
     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 -r 66f006f9ed34 -r 4d0e906543dc tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c        Tue May 03 15:03:29 2011 +0100
+++ b/tools/libxl/libxl_device.c        Tue May 03 16:53:22 2011 +0100
@@ -253,6 +253,7 @@ int libxl__device_destroy(libxl__gc *gc,
     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;
     }
@@ -273,6 +274,7 @@ retry_transaction:
         xs_watch(ctx->xsh, state_path, be_path);
         rc = 1;
     }
+    libxl__device_destroy_tapdisk(gc, be_path);
 out:
     return rc;
 }
diff -r 66f006f9ed34 -r 4d0e906543dc tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h      Tue May 03 15:03:29 2011 +0100
+++ b/tools/libxl/libxl_internal.h      Tue May 03 16:53:22 2011 +0100
@@ -346,6 +346,12 @@ _hidden char *libxl__blktap_devpath(libx
                                     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 -r 66f006f9ed34 -r 4d0e906543dc tools/libxl/libxl_noblktap2.c
--- a/tools/libxl/libxl_noblktap2.c     Tue May 03 15:03:29 2011 +0100
+++ b/tools/libxl/libxl_noblktap2.c     Tue May 03 16:53:22 2011 +0100
@@ -27,3 +27,7 @@ char *libxl__blktap_devpath(libxl__gc *g
 {
     return NULL;
 }
+
+void libxl__device_destroy_tapdisk(libxl__gc *gc, char *be_path)
+{
+}



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel