Re: [Xen-devel] [PATCH 2/3] libxl: acquire a job when destroying a domain

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 25, 2015 at 02:08:35PM -0600, Jim Fehlig wrote:
> A job should be acquired at the beginning of a domain destroy operation,
> not at the end when cleaning up the domain.  Fix two occurances of this
> late job acquisition in the libxl driver.  Doing so renders
> libxlDomainCleanup unused, so it is removed.
> 
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
>  src/libxl/libxl_domain.c | 49 +++++++++++++++++-------------------------------
>  src/libxl/libxl_domain.h |  4 ----
>  src/libxl/libxl_driver.c | 20 ++++++++++++--------
>  3 files changed, 29 insertions(+), 44 deletions(-)
> 
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index eca1ff0..e240eae 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -371,6 +371,9 @@ libxlDomainShutdownThread(void *opaque)
>  
>      cfg = libxlDriverConfigGet(driver);
>  
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> +        goto cleanup;
> +

Won't this a deadlock with 'libxlDomainAutoCoreDump'  (line 410) (which also
calls :

 727     if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
 728         goto cleanup;

well, not deadlock - but spin up to 30 seconds? and then give up?

Perhaps this patch should be folded in?

>From 9f2bac0c28815fc51850290c4b1962881691bfca Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Wed, 25 Mar 2015 22:34:51 -0400
Subject: [PATCH] squash

---
 src/libxl/libxl_domain.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index aef0157..774b070 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -723,15 +723,11 @@ libxlDomainAutoCoreDump(libxlDriverPrivatePtr driver,
                     timestr) < 0)
         goto cleanup;
 
-    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
-        goto cleanup;
-
     /* Unlock virDomainObj while dumping core */
     virObjectUnlock(vm);
     libxl_domain_core_dump(cfg->ctx, vm->def->id, dumpfile, NULL);
     virObjectLock(vm);
 
-    ignore_value(libxlDomainObjEndJob(driver, vm));
     ret = 0;
 
  cleanup:
-- 
1.8.4.2

>      if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) {
>          dom_event = virDomainEventLifecycleNewFromObj(vm,
>                                             VIR_DOMAIN_EVENT_STOPPED,
> @@ -384,7 +387,7 @@ libxlDomainShutdownThread(void *opaque)
>              goto restart;
>          case VIR_DOMAIN_LIFECYCLE_PRESERVE:
>          case VIR_DOMAIN_LIFECYCLE_LAST:
> -            goto cleanup;
> +            goto endjob;
>          }
>      } else if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) {
>          dom_event = virDomainEventLifecycleNewFromObj(vm,
> @@ -399,7 +402,7 @@ libxlDomainShutdownThread(void *opaque)
>              goto restart;
>          case VIR_DOMAIN_LIFECYCLE_CRASH_PRESERVE:
>          case VIR_DOMAIN_LIFECYCLE_CRASH_LAST:
> -            goto cleanup;
> +            goto endjob;
>          case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY:
>              libxlDomainAutoCoreDump(driver, vm);
>              goto destroy;
> @@ -420,11 +423,11 @@ libxlDomainShutdownThread(void *opaque)
>              goto restart;
>          case VIR_DOMAIN_LIFECYCLE_PRESERVE:
>          case VIR_DOMAIN_LIFECYCLE_LAST:
> -            goto cleanup;
> +            goto endjob;
>          }
>      } else {
>          VIR_INFO("Unhandled shutdown_reason %d", xl_reason);
> -        goto cleanup;
> +        goto endjob;
>      }
>  
>   destroy:
> @@ -433,13 +436,11 @@ libxlDomainShutdownThread(void *opaque)
>          dom_event = NULL;
>      }
>      libxl_domain_destroy(cfg->ctx, vm->def->id, NULL);
> -    if (libxlDomainCleanupJob(driver, vm, reason)) {
> -        if (!vm->persistent) {
> -            virDomainObjListRemove(driver->domains, vm);
> -            vm = NULL;
> -        }
> -    }
> -    goto cleanup;
> +    libxlDomainCleanup(driver, vm, reason);
> +    if (!vm->persistent)
> +        virDomainObjListRemove(driver->domains, vm);
> +
> +    goto endjob;
>  
>   restart:
>      if (dom_event) {
> @@ -447,13 +448,17 @@ libxlDomainShutdownThread(void *opaque)
>          dom_event = NULL;
>      }
>      libxl_domain_destroy(cfg->ctx, vm->def->id, NULL);
> -    libxlDomainCleanupJob(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
> +    libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
>      if (libxlDomainStart(driver, vm, false, -1) < 0) {
>          virErrorPtr err = virGetLastError();
>          VIR_ERROR(_("Failed to restart VM '%s': %s"),
>                    vm->def->name, err ? err->message : _("unknown error"));
>      }
>  
> + endjob:
> +    if (!libxlDomainObjEndJob(driver, vm))
> +        vm = NULL;
> +
>   cleanup:
>      if (vm)
>          virObjectUnlock(vm);
> @@ -690,26 +695,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>  }
>  
>  /*
> - * Cleanup function for domain that has reached shutoff state.
> - * Executed in the context of a job.
> - *
> - * virDomainObjPtr should be locked on invocation
> - * Returns true if references remain on virDomainObjPtr, false otherwise.
> - */
> -bool
> -libxlDomainCleanupJob(libxlDriverPrivatePtr driver,
> -                      virDomainObjPtr vm,
> -                      virDomainShutoffReason reason)
> -{
> -    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_DESTROY) < 0)
> -        return true;
> -
> -    libxlDomainCleanup(driver, vm, reason);
> -
> -    return libxlDomainObjEndJob(driver, vm);
> -}
> -
> -/*
>   * Core dump domain to default dump path.
>   *
>   * virDomainObjPtr must be locked on invocation
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index a032e46..30855a2 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -108,10 +108,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>                     virDomainObjPtr vm,
>                     virDomainShutoffReason reason);
>  
> -bool
> -libxlDomainCleanupJob(libxlDriverPrivatePtr driver,
> -                      virDomainObjPtr vm,
> -                      virDomainShutoffReason reason);
>  /*
>   * Note: Xen 4.3 removed the const from the event handler signature.
>   * Detect which signature to use based on
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index da4c1c7..a1977c2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1238,10 +1238,13 @@ libxlDomainDestroyFlags(virDomainPtr dom,
>      if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0)
>          goto cleanup;
>  
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
> +        goto cleanup;
> +
>      if (!virDomainObjIsActive(vm)) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         "%s", _("Domain is not running"));
> -        goto cleanup;
> +        goto endjob;
>      }
>  
>      event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
> @@ -1250,18 +1253,19 @@ libxlDomainDestroyFlags(virDomainPtr dom,
>      if (libxl_domain_destroy(cfg->ctx, vm->def->id, NULL) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to destroy domain '%d'"), vm->def->id);
> -        goto cleanup;
> +        goto endjob;
>      }
>  
> -    if (libxlDomainCleanupJob(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED)) {
> -        if (!vm->persistent) {
> -            virDomainObjListRemove(driver->domains, vm);
> -            vm = NULL;
> -        }
> -    }
> +    libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED);
> +    if (!vm->persistent)
> +        virDomainObjListRemove(driver->domains, vm);
>  
>      ret = 0;
>  
> + endjob:
> +    if (!libxlDomainObjEndJob(driver, vm))
> +        vm = NULL;
> +
>   cleanup:
>      if (vm)
>          virObjectUnlock(vm);
> -- 
> 1.8.4.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]