9:53 p.m.
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
9:53 p.m.
New subject: [libvirt] [PATCH 01/11] libxl: Add job support to libxl driver
Follows the pattern used in the QEMU driver for managing multiple,
simultaneous jobs within the driver.
Signed-off-by: Jim Fehlig <jfehlig(a)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
7:57 a.m.
New subject: [libvirt] [PATCH 01/11] libxl: Add job support to libxl driver
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 01/11] libxl: Add job support to libxl driver
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 02/11] libxl: use job functions in libxlVmStart
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(a)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
7:59 a.m.
New subject: [libvirt] [PATCH 02/11] libxl: use job functions in libxlVmStart
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 02/11] libxl: use job functions in libxlVmStart
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 03/11] libxl: use job functions in libxlDomainSetMemoryFlags
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(a)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
8 a.m.
New subject: [libvirt] [PATCH 03/11] libxl: use job functions in libxlDomainSetMemoryFlags
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 03/11] libxl: use job functions in libxlDomainSetMemoryFlags
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(a)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
10:28 p.m.
New subject: [libvirt] [PATCH 03/11] libxl: use job functions in libxlDomainSetMemoryFlags
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 04/11] libxl: use job functions in libxlDomain{Suspend, Resume}
These operations aren't necessarily time consuming, but need to
wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig(a)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
8 a.m.
New subject: [libvirt] [PATCH 04/11] libxl: use job functions in libxlDomain{Suspend, Resume}
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 04/11] libxl: use job functions in libxlDomain{Suspend, Resume}
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 05/11] libxl: use job functions in libxlDomainDestroyFlags
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig(a)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
8:05 a.m.
New subject: [libvirt] [PATCH 05/11] libxl: use job functions in libxlDomainDestroyFlags
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(a)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 :|
10:44 p.m.
New subject: [libvirt] [PATCH 05/11] libxl: use job functions in libxlDomainDestroyFlags
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(a)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
8:36 a.m.
New subject: [libvirt] [PATCH 05/11] libxl: use job functions in libxlDomainDestroyFlags
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(a)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)
9:53 p.m.
New subject: [libvirt] [PATCH 06/11] libxl: use job functions in domain save operations
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(a)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
8:06 a.m.
New subject: [libvirt] [PATCH 06/11] libxl: use job functions in domain save operations
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 06/11] libxl: use job functions in domain save operations
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 07/11] libxl: use job functions in libxlDomainCoreDump
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(a)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
8:10 a.m.
New subject: [libvirt] [PATCH 07/11] libxl: use job functions in libxlDomainCoreDump
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 07/11] libxl: use job functions in libxlDomainCoreDump
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 08/11] libxl: use job functions in vcpu set and pin functions
These operations aren't necessarily time consuming, but need to
wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig(a)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
8:10 a.m.
New subject: [libvirt] [PATCH 08/11] libxl: use job functions in vcpu set and pin functions
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 08/11] libxl: use job functions in vcpu set and pin functions
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 09/11] libxl: use job functions in device attach and detach functions
These operations aren't necessarily time consuming, but need to
wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig(a)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
8:17 a.m.
New subject: [libvirt] [PATCH 09/11] libxl: use job functions in device attach and detach functions
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(a)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 :|
8:35 a.m.
New subject: [libvirt] [PATCH 09/11] libxl: use job functions in device attach and detach functions
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 10/11] libxl: use job functions in libxlDomainSetAutostart
Setting autostart is a modify operation that needs to wait in the
queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig(a)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
8:17 a.m.
New subject: [libvirt] [PATCH 10/11] libxl: use job functions in libxlDomainSetAutostart
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 10/11] libxl: use job functions in libxlDomainSetAutostart
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(a)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
9:53 p.m.
New subject: [libvirt] [PATCH 11/11] libxl: use job functions in libxlDomainSetSchedulerParametersFlags
Modify operation that needs to wait in the queue of modify jobs.
Signed-off-by: Jim Fehlig <jfehlig(a)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
8:18 a.m.
New subject: [libvirt] [PATCH 11/11] libxl: use job functions in libxlDomainSetSchedulerParametersFlags
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(a)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 :|
8:36 a.m.
New subject: [libvirt] [PATCH 11/11] libxl: use job functions in libxlDomainSetSchedulerParametersFlags
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(a)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
8:36 a.m.
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
8:41 a.m.
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 :|
3909
days inactive
3914
days old
37 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Jim Fehlig -
Michal Privoznik