3:45 p.m.
V2 of a small series to fix issues wrt domain destroy
https://www.redhat.com/archives/libvir-list/2015-March/msg01337.html
Comments from Konrad and Martin are addressed in this version.
Jim Fehlig (3):
libxl: Move job acquisition in libxlDomainStart to callers
libxl: acquire a job when destroying a domain
libxl: drop virDomainObj lock when destroying a domain
src/libxl/libxl_domain.c | 81 ++++++++++++++++++------------------------------
src/libxl/libxl_domain.h | 4 ---
src/libxl/libxl_driver.c | 80 +++++++++++++++++++++++++++++++++++------------
3 files changed, 91 insertions(+), 74 deletions(-)
--
1.8.4.5
3:45 p.m.
New subject: [libvirt] [PATCH V2 1/3] libxl: Move job acquisition in libxlDomainStart to callers
Let callers of libxlDomainStart decide when it is appropriate to
acquire a job on the associated virDomainObj.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
V2:
Don't call virDomainObjListRemove() on persistent domain
src/libxl/libxl_domain.c | 24 ++++++++-------------
src/libxl/libxl_driver.c | 55 +++++++++++++++++++++++++++++++++++++++---------
2 files changed, 54 insertions(+), 25 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index ce9e4d8..6b9959f 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -908,16 +908,13 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
libxl_domain_config_init(&d_config);
- if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
- return ret;
-
cfg = libxlDriverConfigGet(driver);
/* If there is a managed saved state restore it instead of starting
* from scratch. The old state is removed once the restoring succeeded. */
if (restore_fd < 0) {
managed_save_path = libxlDomainManagedSavePath(driver, vm);
if (managed_save_path == NULL)
- goto endjob;
+ goto cleanup;
if (virFileExists(managed_save_path)) {
@@ -925,7 +922,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
managed_save_path,
&def, &hdr);
if (managed_save_fd < 0)
- goto endjob;
+ goto cleanup;
restore_fd = managed_save_fd;
@@ -939,7 +936,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
_("cannot restore domain '%s' uuid %s from a
file"
" which belongs to domain '%s' uuid
%s"),
vm->def->name, vm_uuidstr, def->name,
def_uuidstr);
- goto endjob;
+ goto cleanup;
}
virDomainObjAssignDef(vm, def, true, NULL);
@@ -956,18 +953,18 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
if (libxlBuildDomainConfig(driver->reservedVNCPorts, vm->def,
cfg->ctx, &d_config) < 0)
- goto endjob;
+ goto cleanup;
if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config)
< 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight failed to get free memory for domain
'%s'"),
d_config.c_info.name);
- goto endjob;
+ goto cleanup;
}
if (virHostdevPrepareDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
vm->def, VIR_HOSTDEV_SP_PCI) < 0)
- goto endjob;
+ goto cleanup;
/* Unlock virDomainObj while creating the domain */
virObjectUnlock(vm);
@@ -999,7 +996,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("libxenlight failed to restore domain
'%s'"),
d_config.c_info.name);
- goto endjob;
+ goto cleanup;
}
/*
@@ -1046,7 +1043,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
libxlDomainEventQueue(driver, event);
ret = 0;
- goto endjob;
+ goto cleanup;
cleanup_dom:
if (priv->deathW) {
@@ -1057,10 +1054,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
vm->def->id = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED);
- endjob:
- if (!libxlDomainObjEndJob(driver, vm))
- vm = NULL;
-
+ cleanup:
libxl_domain_config_dispose(&d_config);
VIR_FREE(dom_xml);
VIR_FREE(managed_save_path);
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 05f6eb1..e315b32 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -303,18 +303,26 @@ libxlAutostartDomain(virDomainObjPtr vm,
virObjectLock(vm);
virResetLastError();
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ virObjectUnlock(vm);
+ return ret;
+ }
+
if (vm->autostart && !virDomainObjIsActive(vm) &&
libxlDomainStart(driver, vm, false, -1) < 0) {
err = virGetLastError();
VIR_ERROR(_("Failed to autostart VM '%s': %s"),
vm->def->name,
err ? err->message : _("unknown error"));
- goto cleanup;
+ goto endjob;
}
ret = 0;
- cleanup:
- virObjectUnlock(vm);
+
+ endjob:
+ if (libxlDomainObjEndJob(driver, vm))
+ virObjectUnlock(vm);
+
return ret;
}
@@ -885,17 +893,28 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml,
goto cleanup;
def = NULL;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ if (!vm->persistent) {
+ virDomainObjListRemove(driver->domains, vm);
+ vm = NULL;
+ }
+ goto cleanup;
+ }
+
if (libxlDomainStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0,
-1) < 0) {
virDomainObjListRemove(driver->domains, vm);
- vm = NULL;
- goto cleanup;
+ goto endjob;
}
dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
if (dom)
dom->id = vm->def->id;
+ endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
virDomainDefFree(def);
if (vm)
@@ -1681,7 +1700,7 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
fd = libxlDomainSaveImageOpen(driver, cfg, from, &def, &hdr);
if (fd < 0)
- return -1;
+ goto cleanup;
if (virDomainRestoreFlagsEnsureACL(conn, def) < 0)
goto cleanup;
@@ -1695,11 +1714,20 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,
def = NULL;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+ if (!vm->persistent) {
+ virDomainObjListRemove(driver->domains, vm);
+ vm = NULL;
+ }
+ goto cleanup;
+ }
+
ret = libxlDomainStart(driver, vm, (flags & VIR_DOMAIN_SAVE_PAUSED) != 0, fd);
- if (ret < 0 && !vm->persistent) {
+ if (ret < 0 && !vm->persistent)
virDomainObjListRemove(driver->domains, vm);
+
+ if (!libxlDomainObjEndJob(driver, vm))
vm = NULL;
- }
cleanup:
if (VIR_CLOSE(fd) < 0)
@@ -2567,17 +2595,24 @@ libxlDomainCreateWithFlags(virDomainPtr dom,
if (virDomainCreateWithFlagsEnsureACL(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 already running"));
- goto cleanup;
+ goto endjob;
}
ret = libxlDomainStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1);
if (ret < 0)
- goto cleanup;
+ goto endjob;
dom->id = vm->def->id;
+ endjob:
+ if (!libxlDomainObjEndJob(driver, vm))
+ vm = NULL;
+
cleanup:
if (vm)
virObjectUnlock(vm);
--
1.8.4.5
3:45 p.m.
New subject: [libvirt] [PATCH V2 2/3] libxl: acquire a job when destroying a domain
A job should be acquired at the beginning of a domain destroy operation,
not at the end when cleaning up the domain. Fix two occurrences of this
late job acquisition in the libxl driver. Doing so renders
libxlDomainCleanupJob unused, so it is removed.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
V2:
Don't acquire a job in libxlDomainAutoCoreDump since caller already
has a job.
Fix typos in commit message.
src/libxl/libxl_domain.c | 53 ++++++++++++++++--------------------------------
src/libxl/libxl_domain.h | 4 ----
src/libxl/libxl_driver.c | 20 ++++++++++--------
3 files changed, 29 insertions(+), 48 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 6b9959f..faf2c19 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;
+
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);
@@ -691,26 +696,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
@@ -735,15 +720,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:
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 e315b32..c9623ef 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1240,10 +1240,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,
@@ -1252,18 +1255,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
3:45 p.m.
New subject: [libvirt] [PATCH V2 3/3] libxl: drop virDomainObj lock when destroying a domain
A destroy operation can take considerable time on large memory
domains due to scrubbing the domain' memory. The operation is
running in the context of a job, so unlocking the domain and
allowing query operations is safe.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
V2:
Unchanged
src/libxl/libxl_domain.c | 4 ++++
src/libxl/libxl_driver.c | 5 ++++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index faf2c19..0ac5c62 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -435,7 +435,9 @@ libxlDomainShutdownThread(void *opaque)
libxlDomainEventQueue(driver, dom_event);
dom_event = NULL;
}
+ virObjectUnlock(vm);
libxl_domain_destroy(cfg->ctx, vm->def->id, NULL);
+ virObjectLock(vm);
libxlDomainCleanup(driver, vm, reason);
if (!vm->persistent)
virDomainObjListRemove(driver->domains, vm);
@@ -447,7 +449,9 @@ libxlDomainShutdownThread(void *opaque)
libxlDomainEventQueue(driver, dom_event);
dom_event = NULL;
}
+ virObjectUnlock(vm);
libxl_domain_destroy(cfg->ctx, vm->def->id, NULL);
+ virObjectLock(vm);
libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN);
if (libxlDomainStart(driver, vm, false, -1) < 0) {
virErrorPtr err = virGetLastError();
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index c9623ef..9e08de3 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1252,7 +1252,10 @@ libxlDomainDestroyFlags(virDomainPtr dom,
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED,
VIR_DOMAIN_EVENT_STOPPED_DESTROYED);
- if (libxl_domain_destroy(cfg->ctx, vm->def->id, NULL) < 0) {
+ virObjectUnlock(vm);
+ ret = libxl_domain_destroy(cfg->ctx, vm->def->id, NULL);
+ virObjectLock(vm);
+ if (ret < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Failed to destroy domain '%d'"),
vm->def->id);
goto endjob;
--
1.8.4.5
5:15 p.m.
Jim Fehlig wrote:
V2 of a small series to fix issues wrt domain destroy
https://www.redhat.com/archives/libvir-list/2015-March/msg01337.html
Comments from Konrad and Martin are addressed in this version.
Jim Fehlig (3):
libxl: Move job acquisition in libxlDomainStart to callers
libxl: acquire a job when destroying a domain
libxl: drop virDomainObj lock when destroying a domain
While testing this series along with some ref counting improvements
suggested by Martin, I noticed a few other places where
libxl_domain_destroy is called with virDomainObj locked. I'm going to
post a V3 that changes 3/3 to provide a wrapper for unlocking,
destroying, and locking the domain. Existing libxl_domain_destroy
callers are changed to use the wrapper.
Regards,
Jim
3521
days inactive
3523
days old
4 comments
1 participants
participants (1)
-
Jim Fehlig