libxl device_disk_add orphans blktap devices on transaction error

libxl device_disk_add orphans blktap devices on transaction error

[Darren Shepherd]

I'm using the CentOS 6 bundle of xen from
http://dev.centos.org/centos/6/xen-c6/ and ran into an issue when
creating domains with multiple VHD tap disks.  Comparing unstable to
the 4.2.1 code I'm using, it seems this issue still applies.  I'm
using a configuration line that looks something like

disk = [ "tap:vhd:/var/norm/pools/agentTest/d894b704-b890-488d-b66e-1422b2b9a7a5.vhd,xvda,w",
"tap:vhd:/var/norm/pools/agentTest/70bd3927-0e27-4830-9bf0-5b66ba290547.vhd,xvdb,w"
]

What I noticed happening in the code is that in device_disk_add the
call to libxl__xs_transaction_commit returns rc=1 so the "for (;;)"
loop just tries again.  The problem though is that the blktap device
was already created and assigned to a tapdisk process.  So it will
just leave that process hanging out there and allocated a new device
on the second pass through the for loop.  The second run through
succeeds.  What I typically see is that if I have more than 1 disk on
the domain, the first disk gets created fine and then each subsequent
disk gets two tapdisk processes.

Thanks,
Darren

Re: libxl device_disk_add orphans blktap devices on transaction error

[Roger Pau Monné]

On 02/03/13 23:23, Darren Shepherd wrote:
> I'm using the CentOS 6 bundle of xen from
> http://dev.centos.org/centos/6/xen-c6/ and ran into an issue when
> creating domains with multiple VHD tap disks.  Comparing unstable to
> the 4.2.1 code I'm using, it seems this issue still applies.  I'm
> using a configuration line that looks something like
> 
> disk = [ "tap:vhd:/var/norm/pools/agentTest/d894b704-b890-488d-b66e-1422b2b9a7a5.vhd,xvda,w",
> "tap:vhd:/var/norm/pools/agentTest/70bd3927-0e27-4830-9bf0-5b66ba290547.vhd,xvdb,w"
> ]
> 
> What I noticed happening in the code is that in device_disk_add the
> call to libxl__xs_transaction_commit returns rc=1 so the "for (;;)"
> loop just tries again.  The problem though is that the blktap device
> was already created and assigned to a tapdisk process.  So it will
> just leave that process hanging out there and allocated a new device
> on the second pass through the for loop.  The second run through
> succeeds.  What I typically see is that if I have more than 1 disk on
> the domain, the first disk gets created fine and then each subsequent
> disk gets two tapdisk processes.

Hello Darren

Right now I don't have a system with tapdisk, but I think the following 
patch should fix the problem, could you please try it and report back?

Thanks, Roger.

---
>From e142569922ab810d960e05d26440fa1284e759e7 Mon Sep 17 00:00:00 2001
From: Roger Pau Monne <roger.pau@citrix.com>
Date: Mon, 4 Mar 2013 11:36:41 +0100
Subject: [PATCH] libxl: remove tapdisk process if adding disk fails
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Destroy tapdisk process if transaction fails, and also destroy tapdisk
process if disk adding fails.

Reported-by: Darren Shepherd <darren.s.shepherd@gmail.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 73e0dc3..30f2b04 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2160,6 +2160,10 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
         rc = libxl__xs_transaction_commit(gc, &t);
         if (!rc) break;
         if (rc < 0) goto out;
+        if (disk->backend == LIBXL_DISK_BACKEND_TAP)
+            libxl__device_destroy_tapdisk(gc, GCSPRINTF("%s:%s",
+                    libxl__device_disk_string_of_format(disk->format),
+                    disk->pdev_path));
     }
 
     aodev->dev = device;
@@ -2171,6 +2175,10 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
 out:
     libxl__xs_transaction_abort(gc, &t);
     aodev->rc = rc;
+    if (disk->backend == LIBXL_DISK_BACKEND_TAP)
+        libxl__device_destroy_tapdisk(gc, GCSPRINTF("%s:%s",
+                    libxl__device_disk_string_of_format(disk->format),
+                    disk->pdev_path));
     if (rc) aodev->callback(egc, aodev);
     return;
 }
-- 
1.7.7.5 (Apple Git-26)

Re: libxl device_disk_add orphans blktap devices on transaction error

[Darren Shepherd]

That patch didn't work.  I think what's happening is that in the
successful flow of creating a tapdisk, it immediately tries to release
it right after the "out" label.  So after the first tapdisk is created
it just seems to hang for awhile attempting to destroy the newly
created tapdisk.

Darren

[PATCH] libxl: don't launch more than one tapdisk process for each disk

[Roger Pau Monne]

When adding a disk don't launch multiple tapdisk instances for the
same disk, if transaction fails in device_disk_add reuse the same
tapdisk for further tries instead of creating a new instance each
time a transaction fails.

Reported-by: Darren Shepherd <darren.s.shepherd@gmail.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxl/libxl.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 73e0dc3..345d88b 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2032,7 +2032,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
     STATE_AO_GC(aodev->ao);
     flexarray_t *front = NULL;
     flexarray_t *back = NULL;
-    char *dev, *script;
+    char *dev = NULL, *script;
     libxl__device *device;
     int rc;
     libxl_ctx *ctx = gc->owner;
@@ -2095,12 +2095,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                 break;
 
             case LIBXL_DISK_BACKEND_TAP:
-                dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
-                if (!dev) {
-                    LOG(ERROR, "failed to get blktap devpath for %p\n",
-                        disk->pdev_path);
-                    rc = ERROR_FAIL;
-                    goto out;
+                if (dev == NULL) {
+                    dev = libxl__blktap_devpath(gc, disk->pdev_path,
+                                                disk->format);
+                    if (!dev) {
+                        LOG(ERROR, "failed to get blktap devpath for %p\n",
+                            disk->pdev_path);
+                        rc = ERROR_FAIL;
+                        goto out;
+                    }
                 }
                 flexarray_append(back, "tapdisk-params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",
-- 
1.7.7.5 (Apple Git-26)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] libxl: don't launch more than one tapdisk process for each disk

[Darren Shepherd]

Roger,

I tried this patch out and it fixed my original issue of orphaning
tapdisks.  I actually tested it with the CentOS 6 Xen 4.2.1 build.  I
had to modify the patch slightly to apply (changed "out" to
"out_free").  Below is the patch I used.  Thanks


--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2032,7 +2032,7 @@ static void device_disk_add(libxl__egc *egc,
uint32_t domid,
     STATE_AO_GC(aodev->ao);
     flexarray_t *front = NULL;
     flexarray_t *back = NULL;
-    char *dev, *script;
+    char *dev = NULL, *script;
     libxl__device *device;
     int rc;
     libxl_ctx *ctx = gc->owner;
@@ -2095,12 +2095,15 @@ static void device_disk_add(libxl__egc *egc,
uint32_t domid,
                 break;

             case LIBXL_DISK_BACKEND_TAP:
-                dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
-                if (!dev) {
-                    LOG(ERROR, "failed to get blktap devpath for %p\n",
-                        disk->pdev_path);
-                    rc = ERROR_FAIL;
-                    goto out_free;
+                if (dev == NULL) {
+                    dev = libxl__blktap_devpath(gc, disk->pdev_path,
+                                                disk->format);
+                    if (!dev) {
+                        LOG(ERROR, "failed to get blktap devpath for %p\n",
+                            disk->pdev_path);
+                        rc = ERROR_FAIL;
+                        goto out;
+                    }
                 }
                 flexarray_append(back, "tapdisk-params");
                 flexarray_append(back, libxl__sprintf(gc, "%s:%s",

Re: [PATCH] libxl: don't launch more than one tapdisk process for each disk [and 2 more messages]

[Ian Jackson]

Roger Pau Monne writes ("[Xen-devel] [PATCH] libxl: don't launch more than one tapdisk process for each disk"):
> When adding a disk don't launch multiple tapdisk instances for the
> same disk, if transaction fails in device_disk_add reuse the same
> tapdisk for further tries instead of creating a new instance each
> time a transaction fails.

Applied, thanks.

Pasi Kärkkäinen writes ("Re: [Xen-devel] [PATCH] libxl: don't launch more than one tapdisk process for each disk"):
> So this patch should be backported to Xen 4.2 after it has passed xen-unstable tests.
> See Darren's reply with a Xen 4.2.1 backport included.

Right, thanks.

Ian.

Re: [PATCH] libxl: don't launch more than one tapdisk process for each disk [and 2 more messages]

[Pasi Kärkkäinen]

On Wed, Mar 13, 2013 at 02:41:51PM +0000, Ian Jackson wrote:
> Roger Pau Monne writes ("[Xen-devel] [PATCH] libxl: don't launch more than one tapdisk process for each disk"):
> > When adding a disk don't launch multiple tapdisk instances for the
> > same disk, if transaction fails in device_disk_add reuse the same
> > tapdisk for further tries instead of creating a new instance each
> > time a transaction fails.
> 
> Applied, thanks.
> 
> Pasi Kärkkäinen writes ("Re: [Xen-devel] [PATCH] libxl: don't launch more than one tapdisk process for each disk"):
> > So this patch should be backported to Xen 4.2 after it has passed xen-unstable tests.
> > See Darren's reply with a Xen 4.2.1 backport included.
> 
> Right, thanks.
>

The patch in question seems to have passed xen-unstable automated tests,
so it'd be nice to get this backported to 4.2 branch for 4.2.2.

Thanks,

-- Pasi
 

Re: [PATCH] libxl: don't launch more than one tapdisk process for each disk

[Pasi Kärkkäinen]

On Tue, Mar 05, 2013 at 05:56:42PM +0100, Roger Pau Monne wrote:
> When adding a disk don't launch multiple tapdisk instances for the
> same disk, if transaction fails in device_disk_add reuse the same
> tapdisk for further tries instead of creating a new instance each
> time a transaction fails.
>

So this patch should be backported to Xen 4.2 after it has passed xen-unstable tests.
See Darren's reply with a Xen 4.2.1 backport included.

-- Pasi
 
> Reported-by: Darren Shepherd <darren.s.shepherd@gmail.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/libxl/libxl.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 73e0dc3..345d88b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2032,7 +2032,7 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
>      STATE_AO_GC(aodev->ao);
>      flexarray_t *front = NULL;
>      flexarray_t *back = NULL;
> -    char *dev, *script;
> +    char *dev = NULL, *script;
>      libxl__device *device;
>      int rc;
>      libxl_ctx *ctx = gc->owner;
> @@ -2095,12 +2095,15 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
>                  break;
>  
>              case LIBXL_DISK_BACKEND_TAP:
> -                dev = libxl__blktap_devpath(gc, disk->pdev_path, disk->format);
> -                if (!dev) {
> -                    LOG(ERROR, "failed to get blktap devpath for %p\n",
> -                        disk->pdev_path);
> -                    rc = ERROR_FAIL;
> -                    goto out;
> +                if (dev == NULL) {
> +                    dev = libxl__blktap_devpath(gc, disk->pdev_path,
> +                                                disk->format);
> +                    if (!dev) {
> +                        LOG(ERROR, "failed to get blktap devpath for %p\n",
> +                            disk->pdev_path);
> +                        rc = ERROR_FAIL;
> +                        goto out;
> +                    }
>                  }
>                  flexarray_append(back, "tapdisk-params");
>                  flexarray_append(back, libxl__sprintf(gc, "%s:%s",
> -- 
> 1.7.7.5 (Apple Git-26)
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel