[libvirt] [PATCH 00/11] libxl: add job support
This patch series adds job support to the libxl driver, using techiques from the qemu driver. One benefit is no longer blocking get operations during long running modify operations. E.g. with these patches 'vish dominfo dom' will work while 'virsh save dom ...' is in progress. The first patch adds the job support machinery, followed by several patches that make use of it. I initially had all but the first in a single "use-job-support" patch, but hope breaking it out eases review. Jim Fehlig (11): libxl: Add job support to libxl driver libxl: use job functions in libxlVmStart libxl: use job functions in libxlDomainSetMemoryFlags libxl: use job functions in libxlDomain{Suspend,Resume} libxl: use job functions in libxlDomainDestroyFlags libxl: use job functions in domain save operations libxl: use job functions in libxlDomainCoreDump libxl: use job functions in vcpu set and pin functions libxl: use job functions in device attach and detach functions libxl: use job functions in libxlDomainSetAutostart libxl: use job functions in libxlDomainSetSchedulerParametersFlags src/libxl/libxl_domain.c | 128 ++++++++++++++++++ src/libxl/libxl_domain.h | 37 +++++ src/libxl/libxl_driver.c | 346 +++++++++++++++++++++++++++++++---------------- 3 files changed, 395 insertions(+), 116 deletions(-) -- 1.8.1.4
Follows the pattern used in the QEMU driver for managing multiple, simultaneous jobs within the driver. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_domain.h | 37 ++++++++++++++ 2 files changed, 165 insertions(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index eb2e50e..fdc4661 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -30,10 +30,18 @@ #include "virerror.h" #include "virlog.h" #include "virstring.h" +#include "virtime.h" #define VIR_FROM_THIS VIR_FROM_LIBXL +VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST, + "none", + "query", + "destroy", + "modify", +); + /* Object used to store info related to libxl event registrations */ typedef struct _libxlEventHookInfo libxlEventHookInfo; typedef libxlEventHookInfo *libxlEventHookInfoPtr; @@ -284,6 +292,119 @@ static const libxl_osevent_hooks libxl_event_callbacks = { .timeout_deregister = libxlDomainObjTimeoutDeregisterEventHook, }; +static int +libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv) +{ + memset(&priv->job, 0, sizeof(priv->job)); + + if (virCondInit(&priv->job.cond) < 0) + return -1; + + return 0; +} + +static void +libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv) +{ + struct libxlDomainJobObj *job = &priv->job; + + job->active = LIBXL_JOB_NONE; + job->owner = 0; +} + +static void +libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv) +{ + ignore_value(virCondDestroy(&priv->job.cond)); +} + +/* Give up waiting for mutex after 30 seconds */ +#define LIBXL_JOB_WAIT_TIME (1000ull * 30) + +/* + * obj must be locked before calling, libxlDriverPrivatePtr must NOT be locked + * + * This must be called by anything that will change the VM state + * in any way + * + * Upon successful return, the object will have its ref count increased, + * successful calls must be followed by EndJob eventually + */ +int +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr obj, + enum libxlDomainJob job) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + unsigned long long now; + unsigned long long then; + + if (virTimeMillisNow(&now) < 0) + return -1; + then = now + LIBXL_JOB_WAIT_TIME; + + virObjectRef(obj); + + while (priv->job.active) { + VIR_DEBUG("Wait normal job condition for starting job: %s", + libxlDomainJobTypeToString(job)); + if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) + goto error; + } + + libxlDomainObjResetJob(priv); + + VIR_DEBUG("Starting job: %s", libxlDomainJobTypeToString(job)); + priv->job.active = job; + priv->job.owner = virThreadSelfID(); + + return 0; + +error: + VIR_WARN("Cannot start job (%s) for domain %s;" + " current job is (%s) owned by (%d)", + libxlDomainJobTypeToString(job), + obj->def->name, + libxlDomainJobTypeToString(priv->job.active), + priv->job.owner); + + if (errno == ETIMEDOUT) + virReportError(VIR_ERR_OPERATION_TIMEOUT, + "%s", _("cannot acquire state change lock")); + else + virReportSystemError(errno, + "%s", _("cannot acquire job mutex")); + + virObjectUnref(obj); + return -1; +} + +/* + * obj must be locked before calling + * + * To be called after completing the work associated with the + * earlier libxlDomainBeginJob() call + * + * Returns true if the remaining reference count on obj is + * non-zero, false if the reference count has dropped to zero + * and obj is disposed. + */ +bool +libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr obj) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + enum libxlDomainJob job = priv->job.active; + + VIR_DEBUG("Stopping job: %s", + libxlDomainJobTypeToString(job)); + + libxlDomainObjResetJob(priv); + virCondSignal(&priv->job.cond); + + return virObjectUnref(obj); +} + static void * libxlDomainObjPrivateAlloc(void) { @@ -300,6 +421,12 @@ libxlDomainObjPrivateAlloc(void) return NULL; } + if (libxlDomainObjInitJob(priv) < 0) { + virChrdevFree(priv->devs); + virObjectUnref(priv); + return NULL; + } + return priv; } @@ -311,6 +438,7 @@ libxlDomainObjPrivateDispose(void *obj) if (priv->deathW) libxl_evdisable_domain_death(priv->ctx, priv->deathW); + libxlDomainObjFreeJob(priv); virChrdevFree(priv->devs); libxl_ctx_free(priv->ctx); if (priv->logger_file) diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 8565820..4decc40 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -30,6 +30,31 @@ # include "libxl_conf.h" # include "virchrdev.h" +# define JOB_MASK(job) (1 << (job - 1)) +# define DEFAULT_JOB_MASK \ + (JOB_MASK(LIBXL_JOB_DESTROY) | \ + JOB_MASK(LIBXL_JOB_ABORT)) + +/* Only 1 job is allowed at any time + * A job includes *all* libxl.so api, even those just querying + * information, not merely actions */ +enum libxlDomainJob { + LIBXL_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive) conditions */ + LIBXL_JOB_QUERY, /* Doesn't change any state */ + LIBXL_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */ + LIBXL_JOB_MODIFY, /* May change state */ + + LIBXL_JOB_LAST +}; +VIR_ENUM_DECL(libxlDomainJob) + + +struct libxlDomainJobObj { + virCond cond; /* Use to coordinate jobs */ + enum libxlDomainJob active; /* Currently running job */ + int owner; /* Thread which set current job */ +}; + typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate; typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr; struct _libxlDomainObjPrivate { @@ -43,6 +68,8 @@ struct _libxlDomainObjPrivate { /* console */ virChrdevsPtr devs; libxl_evgen_domain_death *deathW; + + struct libxlDomainJobObj job; }; @@ -53,4 +80,14 @@ extern virDomainDefParserConfig libxlDomainDefParserConfig; int libxlDomainObjPrivateInitCtx(virDomainObjPtr vm); +int +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainJob job) + ATTRIBUTE_RETURN_CHECK; + +bool +libxlDomainObjEndJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj); + #endif /* LIBXL_DOMAIN_H */ -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:05PM -0700, Jim Fehlig wrote:
Follows the pattern used in the QEMU driver for managing multiple, simultaneous jobs within the driver.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_domain.h | 37 ++++++++++++++ 2 files changed, 165 insertions(+)
ACK, simplified version of the QEMU job APIs. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Follows the pattern used in the QEMU driver for managing multiple, simultaneous jobs within the driver.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_domain.h | 37 ++++++++++++++ 2 files changed, 165 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index eb2e50e..fdc4661 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -30,10 +30,18 @@ #include "virerror.h" #include "virlog.h" #include "virstring.h" +#include "virtime.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
+VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST, + "none", + "query", + "destroy", + "modify", +); + /* Object used to store info related to libxl event registrations */ typedef struct _libxlEventHookInfo libxlEventHookInfo; typedef libxlEventHookInfo *libxlEventHookInfoPtr; @@ -284,6 +292,119 @@ static const libxl_osevent_hooks libxl_event_callbacks = { .timeout_deregister = libxlDomainObjTimeoutDeregisterEventHook, };
+static int +libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv) +{ + memset(&priv->job, 0, sizeof(priv->job)); + + if (virCondInit(&priv->job.cond) < 0) + return -1; + + return 0; +} + +static void +libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv) +{ + struct libxlDomainJobObj *job = &priv->job; + + job->active = LIBXL_JOB_NONE; + job->owner = 0; +} + +static void +libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv) +{ + ignore_value(virCondDestroy(&priv->job.cond)); +} + +/* Give up waiting for mutex after 30 seconds */ +#define LIBXL_JOB_WAIT_TIME (1000ull * 30) + +/* + * obj must be locked before calling, libxlDriverPrivatePtr must NOT be locked + * + * This must be called by anything that will change the VM state + * in any way + * + * Upon successful return, the object will have its ref count increased, + * successful calls must be followed by EndJob eventually + */ +int +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr obj, + enum libxlDomainJob job) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + unsigned long long now; + unsigned long long then; + + if (virTimeMillisNow(&now) < 0) + return -1; + then = now + LIBXL_JOB_WAIT_TIME; + + virObjectRef(obj); + + while (priv->job.active) { + VIR_DEBUG("Wait normal job condition for starting job: %s", + libxlDomainJobTypeToString(job)); + if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) + goto error; + } + + libxlDomainObjResetJob(priv); + + VIR_DEBUG("Starting job: %s", libxlDomainJobTypeToString(job)); + priv->job.active = job; + priv->job.owner = virThreadSelfID(); + + return 0; + +error: + VIR_WARN("Cannot start job (%s) for domain %s;" + " current job is (%s) owned by (%d)", + libxlDomainJobTypeToString(job), + obj->def->name, + libxlDomainJobTypeToString(priv->job.active), + priv->job.owner); + + if (errno == ETIMEDOUT) + virReportError(VIR_ERR_OPERATION_TIMEOUT, + "%s", _("cannot acquire state change lock")); + else + virReportSystemError(errno, + "%s", _("cannot acquire job mutex")); + + virObjectUnref(obj); + return -1; +} + +/* + * obj must be locked before calling + * + * To be called after completing the work associated with the + * earlier libxlDomainBeginJob() call + * + * Returns true if the remaining reference count on obj is + * non-zero, false if the reference count has dropped to zero + * and obj is disposed. + */ +bool +libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr obj) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + enum libxlDomainJob job = priv->job.active; + + VIR_DEBUG("Stopping job: %s", + libxlDomainJobTypeToString(job)); + + libxlDomainObjResetJob(priv); + virCondSignal(&priv->job.cond); + + return virObjectUnref(obj); +} + static void * libxlDomainObjPrivateAlloc(void) { @@ -300,6 +421,12 @@ libxlDomainObjPrivateAlloc(void) return NULL; }
+ if (libxlDomainObjInitJob(priv) < 0) { + virChrdevFree(priv->devs); + virObjectUnref(priv); + return NULL; + } + return priv; }
@@ -311,6 +438,7 @@ libxlDomainObjPrivateDispose(void *obj) if (priv->deathW) libxl_evdisable_domain_death(priv->ctx, priv->deathW);
+ libxlDomainObjFreeJob(priv); virChrdevFree(priv->devs); libxl_ctx_free(priv->ctx); if (priv->logger_file) diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index 8565820..4decc40 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -30,6 +30,31 @@ # include "libxl_conf.h" # include "virchrdev.h"
+# define JOB_MASK(job) (1 << (job - 1)) +# define DEFAULT_JOB_MASK \ + (JOB_MASK(LIBXL_JOB_DESTROY) | \ + JOB_MASK(LIBXL_JOB_ABORT)) + +/* Only 1 job is allowed at any time + * A job includes *all* libxl.so api, even those just querying + * information, not merely actions */ +enum libxlDomainJob { + LIBXL_JOB_NONE = 0, /* Always set to 0 for easy if (jobActive) conditions */ + LIBXL_JOB_QUERY, /* Doesn't change any state */ + LIBXL_JOB_DESTROY, /* Destroys the domain (cannot be masked out) */ + LIBXL_JOB_MODIFY, /* May change state */ + + LIBXL_JOB_LAST +}; +VIR_ENUM_DECL(libxlDomainJob) + + +struct libxlDomainJobObj { + virCond cond; /* Use to coordinate jobs */ + enum libxlDomainJob active; /* Currently running job */ + int owner; /* Thread which set current job */ +}; + typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate; typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr; struct _libxlDomainObjPrivate { @@ -43,6 +68,8 @@ struct _libxlDomainObjPrivate { /* console */ virChrdevsPtr devs; libxl_evgen_domain_death *deathW; + + struct libxlDomainJobObj job; };
@@ -53,4 +80,14 @@ extern virDomainDefParserConfig libxlDomainDefParserConfig; int libxlDomainObjPrivateInitCtx(virDomainObjPtr vm);
+int +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainJob job) + ATTRIBUTE_RETURN_CHECK; + +bool +libxlDomainObjEndJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj); + #endif /* LIBXL_DOMAIN_H */
ACK Michal
Creating a large domain could potentially be time consuming. Use the recently added job functions and unlock the virDomainObj while the create operation is in progress. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 59 ++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 8e4242a..7c964c5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -593,7 +593,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainDefPtr def = NULL; virObjectEventPtr event = NULL; libxlSavefileHeader hdr; - int ret; + int ret = -1; uint32_t domid = 0; char *dom_xml = NULL; char *managed_save_path = NULL; @@ -605,14 +605,17 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, #endif if (libxlDomainObjPrivateInitCtx(vm) < 0) - goto error; + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; /* 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 error; + goto endjob; if (virFileExists(managed_save_path)) { @@ -620,7 +623,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, managed_save_path, &def, &hdr); if (managed_save_fd < 0) - goto error; + goto endjob; restore_fd = managed_save_fd; @@ -634,7 +637,7 @@ libxlVmStart(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 error; + goto endjob; } virDomainObjAssignDef(vm, def, true, NULL); @@ -652,17 +655,17 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, libxl_domain_config_init(&d_config); if (libxlBuildDomainConfig(driver, vm, &d_config) < 0) - goto error; + goto endjob; if (cfg->autoballoon && libxlFreeMem(priv, &d_config) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to get free memory for domain '%s'"), d_config.c_info.name); - goto error; + goto endjob; } - /* use as synchronous operations => ao_how = NULL and no intermediate reports => ao_progress = NULL */ - + /* Unlock virDomainObj while creating the domain */ + virObjectUnlock(vm); if (restore_fd < 0) { ret = libxl_domain_create_new(priv->ctx, &d_config, &domid, NULL, NULL); @@ -676,6 +679,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, restore_fd, NULL, NULL); #endif } + virObjectLock(vm); if (ret) { if (restore_fd < 0) @@ -686,25 +690,25 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to restore domain '%s'"), d_config.c_info.name); - goto error; + goto endjob; } vm->def->id = domid; if (libxlDomEventsRegister(vm) < 0) - goto error; + goto endjob; if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL) - goto error; + goto endjob; if (libxl_userdata_store(priv->ctx, domid, "libvirt-xml", (uint8_t *)dom_xml, strlen(dom_xml) + 1)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxenlight failed to store userdata")); - goto error; + goto endjob; } if (libxlDomainSetVcpuAffinities(driver, vm) < 0) - goto error; + goto endjob; if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); @@ -715,7 +719,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto error; + goto endjob; if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -727,25 +731,26 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (event) libxlDomainEventQueue(driver, event); - libxl_domain_config_dispose(&d_config); - VIR_FREE(dom_xml); - VIR_FORCE_CLOSE(managed_save_fd); - virObjectUnref(cfg); - return 0; + ret = 0; -error: - if (domid > 0) { - libxl_domain_destroy(priv->ctx, domid, NULL); - vm->def->id = -1; - virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); +endjob: + libxlDomainObjEndJob(driver, vm); + +cleanup: + if (ret) { + if (domid > 0) { + libxl_domain_destroy(priv->ctx, domid, NULL); + vm->def->id = -1; + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + } + virDomainDefFree(def); } libxl_domain_config_dispose(&d_config); VIR_FREE(dom_xml); VIR_FREE(managed_save_path); - virDomainDefFree(def); VIR_FORCE_CLOSE(managed_save_fd); virObjectUnref(cfg); - return -1; + return ret; } -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:06PM -0700, Jim Fehlig wrote:
Creating a large domain could potentially be time consuming. Use the recently added job functions and unlock the virDomainObj while the create operation is in progress.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 59 ++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Creating a large domain could potentially be time consuming. Use the recently added job functions and unlock the virDomainObj while the create operation is in progress.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 59 ++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 8e4242a..7c964c5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -593,7 +593,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainDefPtr def = NULL; virObjectEventPtr event = NULL; libxlSavefileHeader hdr; - int ret; + int ret = -1; uint32_t domid = 0; char *dom_xml = NULL; char *managed_save_path = NULL; @@ -605,14 +605,17 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, #endif
if (libxlDomainObjPrivateInitCtx(vm) < 0) - goto error; + goto cleanup; + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup;
/* 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 error; + goto endjob;
if (virFileExists(managed_save_path)) {
@@ -620,7 +623,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, managed_save_path, &def, &hdr); if (managed_save_fd < 0) - goto error; + goto endjob;
restore_fd = managed_save_fd;
@@ -634,7 +637,7 @@ libxlVmStart(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 error; + goto endjob; }
virDomainObjAssignDef(vm, def, true, NULL); @@ -652,17 +655,17 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, libxl_domain_config_init(&d_config);
if (libxlBuildDomainConfig(driver, vm, &d_config) < 0) - goto error; + goto endjob;
if (cfg->autoballoon && libxlFreeMem(priv, &d_config) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to get free memory for domain '%s'"), d_config.c_info.name); - goto error; + goto endjob; }
- /* use as synchronous operations => ao_how = NULL and no intermediate reports => ao_progress = NULL */ - + /* Unlock virDomainObj while creating the domain */ + virObjectUnlock(vm); if (restore_fd < 0) { ret = libxl_domain_create_new(priv->ctx, &d_config, &domid, NULL, NULL); @@ -676,6 +679,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, restore_fd, NULL, NULL); #endif } + virObjectLock(vm);
if (ret) { if (restore_fd < 0) @@ -686,25 +690,25 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight failed to restore domain '%s'"), d_config.c_info.name); - goto error; + goto endjob; }
vm->def->id = domid; if (libxlDomEventsRegister(vm) < 0) - goto error; + goto endjob;
if ((dom_xml = virDomainDefFormat(vm->def, 0)) == NULL) - goto error; + goto endjob;
if (libxl_userdata_store(priv->ctx, domid, "libvirt-xml", (uint8_t *)dom_xml, strlen(dom_xml) + 1)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("libxenlight failed to store userdata")); - goto error; + goto endjob; }
if (libxlDomainSetVcpuAffinities(driver, vm) < 0) - goto error; + goto endjob;
if (!start_paused) { libxl_domain_unpause(priv->ctx, domid); @@ -715,7 +719,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto error; + goto endjob;
if (virAtomicIntInc(&driver->nactive) == 1 && driver->inhibitCallback) driver->inhibitCallback(true, driver->inhibitOpaque); @@ -727,25 +731,26 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (event) libxlDomainEventQueue(driver, event);
- libxl_domain_config_dispose(&d_config); - VIR_FREE(dom_xml); - VIR_FORCE_CLOSE(managed_save_fd); - virObjectUnref(cfg); - return 0; + ret = 0;
-error: - if (domid > 0) { - libxl_domain_destroy(priv->ctx, domid, NULL); - vm->def->id = -1; - virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); +endjob: + libxlDomainObjEndJob(driver, vm);
See my comment in the next patch where my concern about this pattern is more visible IMO.
+ +cleanup: + if (ret) {
s/ret/ret < 0/ please
+ if (domid > 0) { + libxl_domain_destroy(priv->ctx, domid, NULL); + vm->def->id = -1; + virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, VIR_DOMAIN_SHUTOFF_FAILED); + } + virDomainDefFree(def); } libxl_domain_config_dispose(&d_config); VIR_FREE(dom_xml); VIR_FREE(managed_save_path); - virDomainDefFree(def); VIR_FORCE_CLOSE(managed_save_fd); virObjectUnref(cfg); - return -1; + return ret; }
ACK Michal
Large balloon operation can be time consuming. Use the recently added job functions and unlock the virDomainObj while ballooning. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7c964c5..4f333bd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1646,6 +1646,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (virDomainSetMemoryFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + isActive = virDomainObjIsActive(vm); if (flags == VIR_DOMAIN_MEM_CURRENT) { @@ -1664,19 +1667,19 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot set memory on an inactive domain")); - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_MEM_CONFIG) { if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot change persistent config of a transient domain")); - goto cleanup; + goto endjob; } if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps, driver->xmlopt, vm))) - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_MEM_MAXIMUM) { @@ -1688,7 +1691,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set maximum memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } } @@ -1699,7 +1702,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; } } else { @@ -1708,17 +1711,23 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (newmem > vm->def->mem.max_balloon) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("cannot set memory higher than max memory")); - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_MEM_LIVE) { + int res; + priv = vm->privateData; - if (libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, - /* force */ 1) < 0) { + /* Unlock virDomainObj while ballooning memory */ + virObjectUnlock(vm); + res = libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, + /* force */ 1); + virObjectLock(vm); + if (res < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } } @@ -1726,12 +1735,15 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, sa_assert(persistentDef); persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; } } ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:07PM -0700, Jim Fehlig wrote:
Large balloon operation can be time consuming. Use the recently added job functions and unlock the virDomainObj while ballooning.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Large balloon operation can be time consuming. Use the recently added job functions and unlock the virDomainObj while ballooning.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7c964c5..4f333bd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1646,6 +1646,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (virDomainSetMemoryFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + isActive = virDomainObjIsActive(vm);
Correct, domain needs to be checked if still alive after job was successfully started.
if (flags == VIR_DOMAIN_MEM_CURRENT) { @@ -1664,19 +1667,19 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot set memory on an inactive domain")); - goto cleanup; + goto endjob; }
if (flags & VIR_DOMAIN_MEM_CONFIG) { if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot change persistent config of a transient domain")); - goto cleanup; + goto endjob; } if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps, driver->xmlopt, vm))) - goto cleanup; + goto endjob; }
if (flags & VIR_DOMAIN_MEM_MAXIMUM) { @@ -1688,7 +1691,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set maximum memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } }
@@ -1699,7 +1702,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; }
} else { @@ -1708,17 +1711,23 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (newmem > vm->def->mem.max_balloon) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("cannot set memory higher than max memory")); - goto cleanup; + goto endjob; }
if (flags & VIR_DOMAIN_MEM_LIVE) { + int res; + priv = vm->privateData; - if (libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, - /* force */ 1) < 0) { + /* Unlock virDomainObj while ballooning memory */ + virObjectUnlock(vm);
1: ^^
+ res = libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, + /* force */ 1); + virObjectLock(vm);
2: ^^
+ if (res < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } }
@@ -1726,12 +1735,15 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, sa_assert(persistentDef); persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; } }
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); +
Hm. I wonder if we should rather check for libxlDomainObjEndJob return value. Currently, as you unlock the domain at [1] another thread waiting on the domain lock may come and lock it for itself. Then we will wait at [2] until the domain is unlocked again. This is possibly dangerous if the other thread was executing libxlDomainDestroyFlags(). If that's the case libxlDomainObjEndJob shall return false, as we held the last reference to @vm. But the memory @vm points to has been already freed (in the EndJob()) ...
cleanup: if (vm) virObjectUnlock(vm);
... what means we are playing with fire here (SIGSEGV). Therefore I think we should check for libxlDomainObjEndJob() return value: endjob: if (!libxlDomainObjEndJob(driver, vm)) vm = NULL; BTW: the domain is unlocked and locked in libxlDomainObjBeginJob() too. Yes, it's for a very short time, but it may be sufficient to another thread hop in and do something bad. This reveals even more interesting bug: libxlVmCleanup() sets vm->dom->id = -1 only for persistent domains. So if the SetMemoryFlags() is running over a transient domain, after job was successfully acquired, virDomainObjIsActive() will return true. And since libxlVmCleanup() replaces vm->def with vm->newDef, the SetMemoryFlags() will continue working on (in general) completely different domain definition than the one when the API started. Sigh. (quick look over the rest of the patches reveals the same problem, but I won't repeat myself in each of them) Otherwise the code looks okay. Michal
Michal Privoznik wrote:
On 07.02.2014 04:53, Jim Fehlig wrote:
Large balloon operation can be time consuming. Use the recently added job functions and unlock the virDomainObj while ballooning.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7c964c5..4f333bd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1646,6 +1646,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (virDomainSetMemoryFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + isActive = virDomainObjIsActive(vm);
Correct, domain needs to be checked if still alive after job was successfully started.
if (flags == VIR_DOMAIN_MEM_CURRENT) { @@ -1664,19 +1667,19 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot set memory on an inactive domain")); - goto cleanup; + goto endjob; }
if (flags & VIR_DOMAIN_MEM_CONFIG) { if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot change persistent config of a transient domain")); - goto cleanup; + goto endjob; } if (!(persistentDef = virDomainObjGetPersistentDef(cfg->caps,
driver->xmlopt, vm))) - goto cleanup; + goto endjob; }
if (flags & VIR_DOMAIN_MEM_MAXIMUM) { @@ -1688,7 +1691,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set maximum memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } }
@@ -1699,7 +1702,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; }
} else { @@ -1708,17 +1711,23 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (newmem > vm->def->mem.max_balloon) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("cannot set memory higher than max memory")); - goto cleanup; + goto endjob; }
if (flags & VIR_DOMAIN_MEM_LIVE) { + int res; + priv = vm->privateData; - if (libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, - /* force */ 1) < 0) { + /* Unlock virDomainObj while ballooning memory */ + virObjectUnlock(vm);
1: ^^
+ res = libxl_set_memory_target(priv->ctx, dom->id, newmem, 0, + /* force */ 1); + virObjectLock(vm);
2: ^^
+ if (res < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } }
@@ -1726,12 +1735,15 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, sa_assert(persistentDef); persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; } }
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); +
Hm. I wonder if we should rather check for libxlDomainObjEndJob return value. Currently, as you unlock the domain at [1] another thread waiting on the domain lock may come and lock it for itself. Then we will wait at [2] until the domain is unlocked again. This is possibly dangerous if the other thread was executing libxlDomainDestroyFlags(). If that's the case libxlDomainObjEndJob shall return false, as we held the last reference to @vm. But the memory @vm points to has been already freed (in the EndJob()) ...
Thanks for the review and finding a bug! I've managed to cause a crash by attempting to concurrently save the same transient domain. In this case, one thread starts the job, saves the domain, and it is destroyed. The other thread then runs the job, fails, ends the job, and kaboom when unlocking the vm. Another twist of what you describe.
cleanup: if (vm) virObjectUnlock(vm);
... what means we are playing with fire here (SIGSEGV).
Therefore I think we should check for libxlDomainObjEndJob() return value:
endjob: if (!libxlDomainObjEndJob(driver, vm)) vm = NULL;
Yes, agreed. I'll rework the cleanup code in this series.
BTW: the domain is unlocked and locked in libxlDomainObjBeginJob() too. Yes, it's for a very short time, but it may be sufficient to another thread hop in and do something bad. This reveals even more interesting bug:
libxlVmCleanup() sets vm->dom->id = -1 only for persistent domains. So if the SetMemoryFlags() is running over a transient domain, after job was successfully acquired, virDomainObjIsActive() will return true. And since libxlVmCleanup() replaces vm->def with vm->newDef, the SetMemoryFlags() will continue working on (in general) completely different domain definition than the one when the API started. Sigh.
Hrm, seems the domid should be unconditionally set to -1 once it is shutdown. But I'm not sure if that solves the general case you describe. Perhaps it will with fixes to patch5. Regards, Jim
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4f333bd..caabb44 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1359,9 +1359,12 @@ libxlDomainSuspend(virDomainPtr dom) if (virDomainSuspendEnsureACL(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; } priv = vm->privateData; @@ -1371,7 +1374,7 @@ libxlDomainSuspend(virDomainPtr dom) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to suspend domain '%d' with libxenlight"), dom->id); - goto cleanup; + goto endjob; } virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_USER); @@ -1381,10 +1384,13 @@ libxlDomainSuspend(virDomainPtr dom) } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob; ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm); @@ -1411,9 +1417,12 @@ libxlDomainResume(virDomainPtr dom) if (virDomainResumeEnsureACL(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; } priv = vm->privateData; @@ -1423,7 +1432,7 @@ libxlDomainResume(virDomainPtr dom) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to resume domain '%d' with libxenlight"), dom->id); - goto cleanup; + goto endjob; } virDomainObjSetState(vm, VIR_DOMAIN_RUNNING, @@ -1434,10 +1443,13 @@ libxlDomainResume(virDomainPtr dom) } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob; ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:08PM -0700, Jim Fehlig wrote:
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4f333bd..caabb44 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c
@@ -1381,10 +1384,13 @@ libxlDomainSuspend(virDomainPtr dom) }
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob;
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm);
@@ -1434,10 +1443,13 @@ libxlDomainResume(virDomainPtr dom) }
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob;
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm);
conditional ACK because of EndJob. Michal
Modify operation that needs to wait in the queue of modify jobs. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index caabb44..bb574bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1549,6 +1549,7 @@ libxlDomainDestroyFlags(virDomainPtr dom, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + bool remove_dom = false; virObjectEventPtr event = NULL; virCheckFlags(0, -1); @@ -1559,10 +1560,13 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_DESTROY) < 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, @@ -1571,17 +1575,22 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup; + goto endjob; } - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true; ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); if (event) -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:09PM -0700, Jim Fehlig wrote:
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index caabb44..bb574bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1549,6 +1549,7 @@ libxlDomainDestroyFlags(virDomainPtr dom, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + bool remove_dom = false; virObjectEventPtr event = NULL;
virCheckFlags(0, -1); @@ -1559,10 +1560,13 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_DESTROY) < 0) + goto cleanup;
So there's one complication in the destroy method. You really, really want destroy to succeed no matter what. In the QEMU driver we will kill -9 the QEMU process from outside the job condition. This should unblock any currently active jobs, so when we then begin the destroy job there's no delay. In the libvirtd daemon RPC dispatch code, the destroy method is also marked as high priority so it gets processed in a dedicated thread pool for high priority RPC calls. Again we don't want high priority calls to be blocked by low priority calls. With this it looks like you're just going to wait indefinitely for any pending job to complete before attempting to kill the guest. Is there anything you can safely do with the libxl APIs which would be equivalent to kill -9, outside the job block. Then you just need the job to protect cleanup of the internal state on virDomainObjPtr.
+ if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); - goto cleanup; + goto endjob; }
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -1571,17 +1575,22 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup; + goto endjob; }
- if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true;
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); if (event)
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Thu, Feb 06, 2014 at 08:53:09PM -0700, Jim Fehlig wrote:
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index caabb44..bb574bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1549,6 +1549,7 @@ libxlDomainDestroyFlags(virDomainPtr dom, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + bool remove_dom = false; virObjectEventPtr event = NULL;
virCheckFlags(0, -1); @@ -1559,10 +1560,13 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_DESTROY) < 0) + goto cleanup;
So there's one complication in the destroy method. You really, really want destroy to succeed no matter what. In the QEMU driver we will kill -9 the QEMU process from outside the job condition. This should unblock any currently active jobs, so when we then begin the destroy job there's no delay. In the libvirtd daemon RPC dispatch code, the destroy method is also marked as high priority so it gets processed in a dedicated thread pool for high priority RPC calls. Again we don't want high priority calls to be blocked by low priority calls.
Thanks for the info.
With this it looks like you're just going to wait indefinitely for any pending job to complete before attempting to kill the guest.
Well, wait for 30 seconds and then fail, but yes, you are right :).
Is there anything you can safely do with the libxl APIs which would be equivalent to kill -9, outside the job block.
I think libxl_domain_destroy() is the kill -9 equivalent. Adding Ian Jackson to confirm...
Then you just need the job to protect cleanup of the internal state on virDomainObjPtr.
Yep, makes sense. I'll adjust the patches accordingly. Regards, Jim
On 07.02.2014 04:53, Jim Fehlig wrote:
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index caabb44..bb574bc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1549,6 +1549,7 @@ libxlDomainDestroyFlags(virDomainPtr dom, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + bool remove_dom = false; virObjectEventPtr event = NULL;
virCheckFlags(0, -1); @@ -1559,10 +1560,13 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_DESTROY) < 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, @@ -1571,17 +1575,22 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup; + goto endjob; }
- if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true;
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); if (event)
Okay, this helps a bit. Since currently you are not allowing any async jobs, the destroy API can't jump in the middle of other job-full APIs. But it still can jump into the job-less APIs (*) that unlocks VM somewhere in the middle. Again, conditional ACK. Michal a job-full API = API using BeginJob + EndJob() a job-less API = API using bare virObjectLock(vm)
Saving domain memory and cpu state can take considerable time. Use the recently added job functions and unlock the virDomainObj while saving the domain. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 54 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index bb574bc..a804b45 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1900,10 +1900,16 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, goto cleanup; } - if (libxl_domain_suspend(priv->ctx, vm->def->id, fd, 0, NULL) != 0) { + /* Unlock virDomainObj while saving domain */ + virObjectUnlock(vm); + ret = libxl_domain_suspend(priv->ctx, vm->def->id, fd, 0, NULL); + virObjectLock(vm); + + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to save domain '%d' with libxenlight"), vm->def->id); + ret = -1; goto cleanup; } @@ -1935,6 +1941,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + bool remove_dom = false; virCheckFlags(0, -1); if (dxml) { @@ -1949,22 +1956,30 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, if (virDomainSaveFlagsEnsureACL(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; } if (libxlDoDomainSave(driver, vm, to) < 0) - goto cleanup; + goto endjob; - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true; ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); return ret; @@ -2125,6 +2140,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm = NULL; char *name = NULL; int ret = -1; + bool remove_dom = false; virCheckFlags(0, -1); @@ -2134,33 +2150,41 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) if (virDomainManagedSaveEnsureACL(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; } if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot do managed save for transient domain")); - goto cleanup; + goto endjob; } name = libxlDomainManagedSavePath(driver, vm); if (name == NULL) - goto cleanup; + goto endjob; VIR_INFO("Saving state to %s", name); if (libxlDoDomainSave(driver, vm, name) < 0) - goto cleanup; + goto endjob; - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true; ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); VIR_FREE(name); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:10PM -0700, Jim Fehlig wrote:
Saving domain memory and cpu state can take considerable time. Use the recently added job functions and unlock the virDomainObj while saving the domain.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 54 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 15 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Saving domain memory and cpu state can take considerable time. Use the recently added job functions and unlock the virDomainObj while saving the domain.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 54 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 15 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index bb574bc..a804b45 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1900,10 +1900,16 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, goto cleanup; }
- if (libxl_domain_suspend(priv->ctx, vm->def->id, fd, 0, NULL) != 0) { + /* Unlock virDomainObj while saving domain */ + virObjectUnlock(vm); + ret = libxl_domain_suspend(priv->ctx, vm->def->id, fd, 0, NULL); + virObjectLock(vm); + + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to save domain '%d' with libxenlight"), vm->def->id); + ret = -1; goto cleanup; }
@@ -1935,6 +1941,7 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + bool remove_dom = false;
virCheckFlags(0, -1); if (dxml) { @@ -1949,22 +1956,30 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, if (virDomainSaveFlagsEnsureACL(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; }
if (libxlDoDomainSave(driver, vm, to) < 0) - goto cleanup; + goto endjob;
- if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true;
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); return ret; @@ -2125,6 +2140,7 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm = NULL; char *name = NULL; int ret = -1; + bool remove_dom = false;
virCheckFlags(0, -1);
@@ -2134,33 +2150,41 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags) if (virDomainManagedSaveEnsureACL(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; } if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot do managed save for transient domain")); - goto cleanup; + goto endjob; }
name = libxlDomainManagedSavePath(driver, vm); if (name == NULL) - goto cleanup; + goto endjob;
VIR_INFO("Saving state to %s", name);
if (libxlDoDomainSave(driver, vm, name) < 0) - goto cleanup; + goto endjob;
- if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true;
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); VIR_FREE(name);
Conditional ACK Michal
Dumping a domain's core can take considerable time. Use the recently added job functions and unlock the virDomainObj while dumping core. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a804b45..84d9ca3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2059,6 +2059,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDomainObjPrivatePtr priv; virDomainObjPtr vm; virObjectEventPtr event = NULL; + bool remove_dom = false; bool paused = false; int ret = -1; @@ -2070,9 +2071,12 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) if (virDomainCoreDumpEnsureACL(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; } priv = vm->privateData; @@ -2084,38 +2088,41 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) _("Before dumping core, failed to suspend domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP); paused = true; } - if (libxl_domain_core_dump(priv->ctx, dom->id, to, NULL) != 0) { + /* Unlock virDomainObj while dumping core */ + virObjectUnlock(vm); + ret = libxl_domain_core_dump(priv->ctx, dom->id, to, NULL); + virObjectLock(vm); + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to dump core of domain '%d' with libxenlight"), dom->id); - goto cleanup_unpause; + ret = -1; + goto unpause; } if (flags & VIR_DUMP_CRASH) { if (libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup_unpause; + goto unpause; } event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true; } ret = 0; -cleanup_unpause: - if (vm && virDomainObjIsActive(vm) && paused) { +unpause: + if (virDomainObjIsActive(vm) && paused) { if (libxl_domain_unpause(priv->ctx, dom->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("After dumping core, failed to resume domain '%d' with" @@ -2125,7 +2132,15 @@ cleanup_unpause: VIR_DOMAIN_RUNNING_UNPAUSED); } } + +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); if (event) -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:11PM -0700, Jim Fehlig wrote:
Dumping a domain's core can take considerable time. Use the recently added job functions and unlock the virDomainObj while dumping core.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Dumping a domain's core can take considerable time. Use the recently added job functions and unlock the virDomainObj while dumping core.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index a804b45..84d9ca3 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2059,6 +2059,7 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) libxlDomainObjPrivatePtr priv; virDomainObjPtr vm; virObjectEventPtr event = NULL; + bool remove_dom = false; bool paused = false; int ret = -1;
@@ -2070,9 +2071,12 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) if (virDomainCoreDumpEnsureACL(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; }
priv = vm->privateData; @@ -2084,38 +2088,41 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) _("Before dumping core, failed to suspend domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_DUMP); paused = true; }
- if (libxl_domain_core_dump(priv->ctx, dom->id, to, NULL) != 0) { + /* Unlock virDomainObj while dumping core */ + virObjectUnlock(vm); + ret = libxl_domain_core_dump(priv->ctx, dom->id, to, NULL); + virObjectLock(vm); + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to dump core of domain '%d' with libxenlight"), dom->id); - goto cleanup_unpause; + ret = -1; + goto unpause; }
if (flags & VIR_DUMP_CRASH) { if (libxlVmReap(driver, vm, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup_unpause; + goto unpause; }
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } + if (!vm->persistent) + remove_dom = true; }
ret = 0;
-cleanup_unpause: - if (vm && virDomainObjIsActive(vm) && paused) { +unpause: + if (virDomainObjIsActive(vm) && paused) { if (libxl_domain_unpause(priv->ctx, dom->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("After dumping core, failed to resume domain '%d' with" @@ -2125,7 +2132,15 @@ cleanup_unpause: VIR_DOMAIN_RUNNING_UNPAUSED); } } + +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: + if (remove_dom) { + virDomainObjListRemove(driver->domains, vm); + vm = NULL; + } if (vm) virObjectUnlock(vm); if (event)
Conditional ACK Michal
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 84d9ca3..7ce127a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2322,22 +2322,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_VCPU_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot set vcpus on an inactive domain")); - goto cleanup; + goto endjob; } if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot change persistent config of a transient domain")); - goto cleanup; + goto endjob; } if ((max = libxlConnectGetMaxVcpus(dom->conn, NULL)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not determine max vcpus for the domain")); - goto cleanup; + goto endjob; } if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus < max) { @@ -2348,17 +2351,17 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virReportError(VIR_ERR_INVALID_ARG, _("requested vcpus is greater than max allowable" " vcpus for the domain: %d > %d"), nvcpus, max); - goto cleanup; + goto endjob; } priv = vm->privateData; if (!(def = virDomainObjGetPersistentDef(cfg->caps, driver->xmlopt, vm))) - goto cleanup; + goto endjob; maplen = VIR_CPU_MAPLEN(nvcpus); if (VIR_ALLOC_N(bitmask, maplen) < 0) - goto cleanup; + goto endjob; for (i = 0; i < nvcpus; ++i) { pos = i / 8; @@ -2384,7 +2387,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set vcpus for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } break; @@ -2393,7 +2396,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set vcpus for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } def->vcpus = nvcpus; break; @@ -2404,6 +2407,9 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (flags & VIR_DOMAIN_VCPU_CONFIG) ret = virDomainSaveConfig(cfg->configDir, def); +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: VIR_FREE(bitmask); if (vm) @@ -2495,15 +2501,18 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, if (virDomainPinVcpuFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && !virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is inactive")); - goto cleanup; + goto endjob; } if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, &targetDef) < 0) - goto cleanup; + goto endjob; if (flags & VIR_DOMAIN_AFFECT_LIVE) { targetDef = vm->def; @@ -2514,7 +2523,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, pcpumap = virBitmapNewData(cpumap, maplen); if (!pcpumap) - goto cleanup; + goto endjob; if (flags & VIR_DOMAIN_AFFECT_LIVE) { libxl_bitmap map = { .size = maplen, .map = cpumap }; @@ -2525,7 +2534,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to pin vcpu '%d' with libxenlight"), vcpu); - goto cleanup; + goto endjob; } } @@ -2535,14 +2544,14 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to delete vcpupin xml for vcpu '%d'"), vcpu); - goto cleanup; + goto endjob; } - goto out; + goto done; } if (!targetDef->cputune.vcpupin) { if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0) - goto cleanup; + goto endjob; targetDef->cputune.nvcpupin = 0; } if (virDomainVcpuPinAdd(&targetDef->cputune.vcpupin, @@ -2552,10 +2561,10 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, vcpu) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update or add vcpupin xml")); - goto cleanup; + goto endjob; } -out: +done: ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -2564,6 +2573,9 @@ out: ret = virDomainSaveConfig(cfg->configDir, targetDef); } +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:12PM -0700, Jim Fehlig wrote:
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 46 +++++++++++++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 84d9ca3..7ce127a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2322,22 +2322,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (virDomainSetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_VCPU_LIVE)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot set vcpus on an inactive domain")); - goto cleanup; + goto endjob; }
if (!vm->persistent && (flags & VIR_DOMAIN_VCPU_CONFIG)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot change persistent config of a transient domain")); - goto cleanup; + goto endjob; }
if ((max = libxlConnectGetMaxVcpus(dom->conn, NULL)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("could not determine max vcpus for the domain")); - goto cleanup; + goto endjob; }
if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus < max) { @@ -2348,17 +2351,17 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virReportError(VIR_ERR_INVALID_ARG, _("requested vcpus is greater than max allowable" " vcpus for the domain: %d > %d"), nvcpus, max); - goto cleanup; + goto endjob; }
priv = vm->privateData;
if (!(def = virDomainObjGetPersistentDef(cfg->caps, driver->xmlopt, vm))) - goto cleanup; + goto endjob;
maplen = VIR_CPU_MAPLEN(nvcpus); if (VIR_ALLOC_N(bitmask, maplen) < 0) - goto cleanup; + goto endjob;
for (i = 0; i < nvcpus; ++i) { pos = i / 8; @@ -2384,7 +2387,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set vcpus for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } break;
@@ -2393,7 +2396,7 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set vcpus for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } def->vcpus = nvcpus; break; @@ -2404,6 +2407,9 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (flags & VIR_DOMAIN_VCPU_CONFIG) ret = virDomainSaveConfig(cfg->configDir, def);
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: VIR_FREE(bitmask); if (vm) @@ -2495,15 +2501,18 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, if (virDomainPinVcpuFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && !virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is inactive")); - goto cleanup; + goto endjob; }
if (virDomainLiveConfigHelperMethod(cfg->caps, driver->xmlopt, vm, &flags, &targetDef) < 0) - goto cleanup; + goto endjob;
if (flags & VIR_DOMAIN_AFFECT_LIVE) { targetDef = vm->def; @@ -2514,7 +2523,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu,
pcpumap = virBitmapNewData(cpumap, maplen); if (!pcpumap) - goto cleanup; + goto endjob;
if (flags & VIR_DOMAIN_AFFECT_LIVE) { libxl_bitmap map = { .size = maplen, .map = cpumap }; @@ -2525,7 +2534,7 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to pin vcpu '%d' with libxenlight"), vcpu); - goto cleanup; + goto endjob; } }
@@ -2535,14 +2544,14 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to delete vcpupin xml for vcpu '%d'"), vcpu); - goto cleanup; + goto endjob; } - goto out; + goto done; }
if (!targetDef->cputune.vcpupin) { if (VIR_ALLOC(targetDef->cputune.vcpupin) < 0) - goto cleanup; + goto endjob; targetDef->cputune.nvcpupin = 0; } if (virDomainVcpuPinAdd(&targetDef->cputune.vcpupin, @@ -2552,10 +2561,10 @@ libxlDomainPinVcpuFlags(virDomainPtr dom, unsigned int vcpu, vcpu) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update or add vcpupin xml")); - goto cleanup; + goto endjob; }
-out: +done: ret = 0;
if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -2564,6 +2573,9 @@ out: ret = virDomainSaveConfig(cfg->configDir, targetDef); }
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm);
Conditional ACK Michal
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7ce127a..528c2cb 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3414,6 +3414,9 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainAttachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (virDomainObjIsActive(vm)) { if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; @@ -3424,14 +3427,14 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); - goto cleanup; + goto endjob; } } if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify device on transient domain")); - goto cleanup; + goto endjob; } priv = vm->privateData; @@ -3440,15 +3443,15 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob; /* Make a copy for updated domain. */ if (!(vmdef = virDomainObjCopyPersistentDef(vm, cfg->caps, driver->xmlopt))) - goto cleanup; + goto endjob; if ((ret = libxlDomainAttachDeviceConfig(vmdef, dev)) < 0) - goto cleanup; + goto endjob; } else { ret = 0; } @@ -3459,10 +3462,10 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob; if ((ret = libxlDomainAttachDeviceLive(priv, vm, dev)) < 0) - goto cleanup; + goto endjob; /* * update domain status forcibly because the domain status may be @@ -3481,6 +3484,9 @@ libxlDomainAttachDeviceFlags(virDomainPtr dom, const char *xml, } } +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); @@ -3518,6 +3524,9 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (virDomainDetachDeviceFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (virDomainObjIsActive(vm)) { if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; @@ -3528,14 +3537,14 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_LIVE) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); - goto cleanup; + goto endjob; } } if ((flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) && !vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot modify device on transient domain")); - goto cleanup; + goto endjob; } priv = vm->privateData; @@ -3544,15 +3553,15 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob; /* Make a copy for updated domain. */ if (!(vmdef = virDomainObjCopyPersistentDef(vm, cfg->caps, driver->xmlopt))) - goto cleanup; + goto endjob; if ((ret = libxlDomainDetachDeviceConfig(vmdef, dev)) < 0) - goto cleanup; + goto endjob; } else { ret = 0; } @@ -3563,10 +3572,10 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, if (!(dev = virDomainDeviceDefParse(xml, vm->def, cfg->caps, driver->xmlopt, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob; if ((ret = libxlDomainDetachDeviceLive(priv, vm, dev)) < 0) - goto cleanup; + goto endjob; /* * update domain status forcibly because the domain status may be @@ -3585,6 +3594,9 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, } } +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:13PM -0700, Jim Fehlig wrote:
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
These operations aren't necessarily time consuming, but need to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7ce127a..528c2cb 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c
@@ -3585,6 +3594,9 @@ libxlDomainDetachDeviceFlags(virDomainPtr dom, const char *xml, } }
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev);
Conditional ACK Michal
Setting autostart is a modify operation that needs to wait in the queue of modify jobs. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 528c2cb..79d64a5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3855,40 +3855,43 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart) if (virDomainSetAutostartEnsureACL(dom->conn, vm->def) < 0) goto cleanup; + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (!vm->persistent) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot set autostart for transient domain")); - goto cleanup; + goto endjob; } autostart = (autostart != 0); if (vm->autostart != autostart) { if (!(configFile = virDomainConfigFile(cfg->configDir, vm->def->name))) - goto cleanup; + goto endjob; if (!(autostartLink = virDomainConfigFile(cfg->autostartDir, vm->def->name))) - goto cleanup; + goto endjob; if (autostart) { if (virFileMakePath(cfg->autostartDir) < 0) { virReportSystemError(errno, _("cannot create autostart directory %s"), cfg->autostartDir); - goto cleanup; + goto endjob; } if (symlink(configFile, autostartLink) < 0) { virReportSystemError(errno, _("Failed to create symlink '%s to '%s'"), autostartLink, configFile); - goto cleanup; + goto endjob; } } else { if (unlink(autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) { virReportSystemError(errno, _("Failed to delete symlink '%s'"), autostartLink); - goto cleanup; + goto endjob; } } @@ -3896,6 +3899,9 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart) } ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:14PM -0700, Jim Fehlig wrote:
Setting autostart is a modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Setting autostart is a modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 528c2cb..79d64a5 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c
@@ -3896,6 +3899,9 @@ libxlDomainSetAutostart(virDomainPtr dom, int autostart) } ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: VIR_FREE(configFile); VIR_FREE(autostartLink);
Conditional ACK Michal
Modify operation that needs to wait in the queue of modify jobs. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 79d64a5..a4b6ecd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -4045,6 +4045,7 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, int nparams, unsigned int flags) { + libxlDriverPrivatePtr driver = dom->conn->privateData; libxlDomainObjPrivatePtr priv; virDomainObjPtr vm; libxl_domain_sched_params sc_info; @@ -4067,9 +4068,12 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, if (virDomainSetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def, flags) < 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; } priv = vm->privateData; @@ -4079,14 +4083,14 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, if (sched_id != LIBXL_SCHEDULER_CREDIT) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Only 'credit' scheduler is supported")); - goto cleanup; + goto endjob; } if (libxl_domain_sched_params_get(priv->ctx, dom->id, &sc_info) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get scheduler parameters for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } for (i = 0; i < nparams; ++i) { @@ -4102,11 +4106,14 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set scheduler parameters for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } ret = 0; +endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm); -- 1.8.1.4
On Thu, Feb 06, 2014 at 08:53:15PM -0700, Jim Fehlig wrote:
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07.02.2014 04:53, Jim Fehlig wrote:
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_driver.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 79d64a5..a4b6ecd 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c
@@ -4102,11 +4106,14 @@ libxlDomainSetSchedulerParametersFlags(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set scheduler parameters for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; }
ret = 0;
+endjob: + libxlDomainObjEndJob(driver, vm); + cleanup: if (vm) virObjectUnlock(vm);
Conditional ACK Michal
On 07.02.2014 04:53, Jim Fehlig wrote:
This patch series adds job support to the libxl driver, using techiques from the qemu driver. One benefit is no longer blocking get operations during long running modify operations. E.g. with these patches 'vish dominfo dom' will work while 'virsh save dom ...' is in progress.
The first patch adds the job support machinery, followed by several patches that make use of it. I initially had all but the first in a single "use-job-support" patch, but hope breaking it out eases review.
Jim Fehlig (11): libxl: Add job support to libxl driver libxl: use job functions in libxlVmStart libxl: use job functions in libxlDomainSetMemoryFlags libxl: use job functions in libxlDomain{Suspend,Resume} libxl: use job functions in libxlDomainDestroyFlags libxl: use job functions in domain save operations libxl: use job functions in libxlDomainCoreDump libxl: use job functions in vcpu set and pin functions libxl: use job functions in device attach and detach functions libxl: use job functions in libxlDomainSetAutostart libxl: use job functions in libxlDomainSetSchedulerParametersFlags
src/libxl/libxl_domain.c | 128 ++++++++++++++++++ src/libxl/libxl_domain.h | 37 +++++ src/libxl/libxl_driver.c | 346 +++++++++++++++++++++++++++++++---------------- 3 files changed, 395 insertions(+), 116 deletions(-)
Aha! This needs to be applied on the top of your 'misc trivial cleanups' set. The last time I've tried to introduce job control to the storage driver, it got rejected as it duplicated a lot of code from the qemu driver. Somebody suggested to rip the job control out into a separate unit so the code could be shared. Having said that, I'll review the patches, but please take my ACKs as 'code is okay' not permission to push. I'd rather see somebody else giving ACK to the design. Michal
On Tue, Feb 11, 2014 at 03:36:15PM +0100, Michal Privoznik wrote:
On 07.02.2014 04:53, Jim Fehlig wrote:
This patch series adds job support to the libxl driver, using techiques from the qemu driver. One benefit is no longer blocking get operations during long running modify operations. E.g. with these patches 'vish dominfo dom' will work while 'virsh save dom ...' is in progress.
The first patch adds the job support machinery, followed by several patches that make use of it. I initially had all but the first in a single "use-job-support" patch, but hope breaking it out eases review.
Jim Fehlig (11): libxl: Add job support to libxl driver libxl: use job functions in libxlVmStart libxl: use job functions in libxlDomainSetMemoryFlags libxl: use job functions in libxlDomain{Suspend,Resume} libxl: use job functions in libxlDomainDestroyFlags libxl: use job functions in domain save operations libxl: use job functions in libxlDomainCoreDump libxl: use job functions in vcpu set and pin functions libxl: use job functions in device attach and detach functions libxl: use job functions in libxlDomainSetAutostart libxl: use job functions in libxlDomainSetSchedulerParametersFlags
src/libxl/libxl_domain.c | 128 ++++++++++++++++++ src/libxl/libxl_domain.h | 37 +++++ src/libxl/libxl_driver.c | 346 +++++++++++++++++++++++++++++++---------------- 3 files changed, 395 insertions(+), 116 deletions(-)
Aha! This needs to be applied on the top of your 'misc trivial cleanups' set.
The last time I've tried to introduce job control to the storage driver, it got rejected as it duplicated a lot of code from the qemu driver. Somebody suggested to rip the job control out into a separate unit so the code could be shared. Having said that, I'll review the patches, but please take my ACKs as 'code is okay' not permission to push. I'd rather see somebody else giving ACK to the design.
We could definitely use some shared module for job control APIs. I didn't want to put too much burden on Jim right now though, since we've still not merged the large PCI shared API work we asked for wrt libxl + hostdev support. So figured shared job control could be done as a followup. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Jim Fehlig -
Michal Privoznik