thr3ads.net - Xen devel - libxl device_disk_add orphans blktap devices on transaction error [Mar 2013]

If this information is useful, please help other people find it:
Share via:

Darren Shepherd

2013-Mar-02 22:23 UTC

libxl device_disk_add orphans blktap devices on transaction error

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

Roger Pau Monné

2013-Mar-04 10:59 UTC

head link

Re: libxl device_disk_add orphans blktap devices on transaction error

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)

Darren Shepherd

2013-Mar-04 21:42 UTC

head link

Re: libxl device_disk_add orphans blktap devices on transaction error

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

Roger Pau Monne

2013-Mar-05 16:56 UTC

head link

[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.

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

Darren Shepherd

2013-Mar-10 16:06 UTC

head link

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

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",

Pasi Kärkkäinen

2013-Mar-12 08:16 UTC

head link

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

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

Ian Jackson

2013-Mar-13 14:41 UTC

head link

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

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.

Pasi Kärkkäinen

2013-Mar-17 11:47 UTC

head link

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

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

Possibly Parallel Threads

Search for more seemingly similar threads

Xen devel - Mar 2013 - libxl device_disk_add orphans blktap devices on transaction error

libxl device_disk_add orphans blktap devices on transaction error

Re: libxl device_disk_add orphans blktap devices on transaction error

Re: libxl device_disk_add orphans blktap devices on transaction error

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

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

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

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

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

Possibly Parallel Threads