Bamvor Jian Zhang wrote:

This patch introduce a lock for protecting the long-running api (save, dump, migration and so on) from the other api which may update the status of the virtual machine.

Hi Bamvor, Thanks for the patches and sorry for the delayed response. I've been traveling quite a bit lately and just got around to reviewing and testing your work. Testing so far looks good. I can save and dump vm's while at the same time list and retrieve info. See my comments inline, but did want to raise a more general comment first. There is a quite a bit of code here borrowed from the qemu driver, which in general is fine since the libxl driver does not need the same locking features as the qemu one. I'd like to hear the opinion of other libvirt maintainers wrt the duplicated code.

Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com> --- src/libxl/libxl_conf.h | 58 ++++++ src/libxl/libxl_driver.c | 446 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 504 insertions(+)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 56bf85c..dab1466 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -73,6 +73,62 @@ struct _libxlDriverPrivate { char *saveDir; };

+# define JOB_MASK(job) (1 << (job - 1)) +# define DEFAULT_JOB_MASK \ + (JOB_MASK(LIBXL_JOB_DESTROY) | \ + JOB_MASK(LIBXL_JOB_ABORT)) + +/* Jobs which have to be tracked in domain state XML. */ +# define LIBXL_DOMAIN_TRACK_JOBS \ + (JOB_MASK(LIBXL_JOB_DESTROY) | \ + JOB_MASK(LIBXL_JOB_ASYNC)) + +/* 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_DESTROY, /* Destroys the domain (cannot be masked out) */ + LIBXL_JOB_MODIFY, /* May change state */ + LIBXL_JOB_ABORT, /* Abort current async job */ + LIBXL_JOB_MIGRATION_OP, /* Operation influencing outgoing migration */ + + /* The following two items must always be the last items before JOB_LAST */ + LIBXL_JOB_ASYNC, /* Asynchronous job */

There is only one item, so the comment needs updated.

+ + LIBXL_JOB_LAST +}; +VIR_ENUM_DECL(libxlDomainJob) + +/* Async job consists of a series of jobs that may change state. Independent + * jobs that do not change state (and possibly others if explicitly allowed by + * current async job) are allowed to be run even if async job is active. + */ +enum libxlDomainAsyncJob { + LIBXL_ASYNC_JOB_NONE = 0, + LIBXL_ASYNC_JOB_MIGRATION_OUT, + LIBXL_ASYNC_JOB_MIGRATION_IN,

The libxl driver doesn't currently support migration. These could be added in a patch set implementing migration.

+ LIBXL_ASYNC_JOB_SAVE, + LIBXL_ASYNC_JOB_DUMP,

IMO, LIBXL_ASYNC_JOB_RESTORE should be added to this list. I see It is omitted in the qemu driver as well, so perhaps there is a good reason for it?

+ + LIBXL_ASYNC_JOB_LAST +}; +VIR_ENUM_DECL(libxlDomainAsyncJob) + +struct libxlDomainJobObj { + virCond cond; /* Use to coordinate jobs */ + enum libxlDomainJob active; /* Currently running job */ + int owner; /* Thread which set current job */ + + virCond asyncCond; /* Use to coordinate with async jobs */ + enum libxlDomainAsyncJob asyncJob; /* Currently active async job */ + int asyncOwner; /* Thread which set current async job */ + int phase; /* Job phase (mainly for migrations) */ + unsigned long long mask; /* Jobs allowed during async job */ + unsigned long long start; /* When the async job started */ + virDomainJobInfo info; /* Async job progress data */ +}; + typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate; typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr; struct _libxlDomainObjPrivate { @@ -81,6 +137,8 @@ struct _libxlDomainObjPrivate { libxl_waiter *dWaiter; int waiterFD; int eventHdl; + + struct libxlDomainJobObj job; };

# define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r" diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6fe284f..d01d915 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -45,6 +45,7 @@ #include "xen_xm.h" #include "virtypedparam.h" #include "viruri.h" +#include "virtime.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -84,6 +85,336 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver) virMutexUnlock(&driver->lock); }

+/* job */ +VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST, + "none", + "destroy", + "modify", + "abort", + "migration operation", + "none", /* async job is never stored in job.active */ +); + +VIR_ENUM_IMPL(libxlDomainAsyncJob, LIBXL_ASYNC_JOB_LAST, + "none", + "migration out", + "migration in",

Can be added with a migration implementation.

+ "save", + "dump",

"restore"?

+); + +static int +libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv) +{ + memset(&priv->job, 0, sizeof(priv->job)); + + if (virCondInit(&priv->job.cond) < 0) + return -1; + + if (virCondInit(&priv->job.asyncCond) < 0) { + ignore_value(virCondDestroy(&priv->job.cond)); + return -1; + } + + return 0; +} + +static void +libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv) +{ + struct libxlDomainJobObj *job = &priv->job; + + job->active = LIBXL_JOB_NONE; + job->owner = 0; +} + +static void +libxlDomainObjResetAsyncJob(libxlDomainObjPrivatePtr priv) +{ + struct libxlDomainJobObj *job = &priv->job; + + job->asyncJob = LIBXL_ASYNC_JOB_NONE; + job->asyncOwner = 0; + job->phase = 0; + job->mask = DEFAULT_JOB_MASK; + job->start = 0; + memset(&job->info, 0, sizeof(job->info)); +} + +static void +libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv) +{ + ignore_value(virCondDestroy(&priv->job.cond)); + ignore_value(virCondDestroy(&priv->job.asyncCond)); +} + +static bool +libxlDomainTrackJob(enum libxlDomainJob job) +{ + return (LIBXL_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0; +} + +static void +libxlDomainObjSaveJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) +{ + if (!virDomainObjIsActive(obj)) { + /* don't write the state file yet, it will be written once the domain + * gets activated */ + return; + } + + if (virDomainSaveStatus(driver->caps, driver->stateDir, obj) < 0) + VIR_WARN("Failed to save status on vm %s", obj->def->name); +} + +static bool +libxlDomainNestedJobAllowed(libxlDomainObjPrivatePtr priv, enum libxlDomainJob job) +{ + return !priv->job.asyncJob || (priv->job.mask & JOB_MASK(job)) != 0; +} + +static void +libxlDomainObjSetAsyncJobMask(virDomainObjPtr obj, + unsigned long long allowedJobs) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + + if (!priv->job.asyncJob) + return; + + priv->job.mask = allowedJobs | JOB_MASK(LIBXL_JOB_DESTROY); +} + +/* Give up waiting for mutex after 30 seconds */ +#define LIBXL_JOB_WAIT_TIME (1000ull * 30) + +/* + * obj must be locked before calling; driver_locked says if libxlDriverPrivatePtr + * is locked or not. + */ +static int ATTRIBUTE_NONNULL(1) +libxlDomainObjBeginJobInternal(libxlDriverPrivatePtr driver, + bool driver_locked, + virDomainObjPtr obj, + enum libxlDomainJob job, + enum libxlDomainAsyncJob asyncJob) +{ + 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); + if (driver_locked) { + libxlDriverUnlock(driver); + } + +retry: + while (!libxlDomainNestedJobAllowed(priv, job)) { + VIR_DEBUG("Wait async job condition for starting job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (virCondWaitUntil(&priv->job.asyncCond, &obj->lock, then) < 0) + goto error; + } + + while (priv->job.active) { + VIR_DEBUG("Wait normal job condition for starting job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (virCondWaitUntil(&priv->job.cond, &obj->lock, then) < 0) + goto error; + } + + /* No job is active but a new async job could have been started while obj + * was unlocked, so we need to recheck it. */ + if (!libxlDomainNestedJobAllowed(priv, job)) + goto retry; + + libxlDomainObjResetJob(priv); + + if (job != LIBXL_JOB_ASYNC) { + VIR_DEBUG("Starting job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + priv->job.active = job; + priv->job.owner = virThreadSelfID(); + } else { + VIR_DEBUG("Starting async job: %s", + libxlDomainAsyncJobTypeToString(asyncJob)); + libxlDomainObjResetAsyncJob(priv); + priv->job.asyncJob = asyncJob; + priv->job.asyncOwner = virThreadSelfID(); + priv->job.start = now; + } + + if (driver_locked) { + virDomainObjUnlock(obj); + libxlDriverLock(driver); + virDomainObjLock(obj); + } + + if (libxlDomainTrackJob(job)) + libxlDomainObjSaveJob(driver, obj); + + return 0; + +error: + VIR_WARN("Cannot start job (%s, %s) for domain %s;" + " current job is (%s, %s) owned by (%d, %d)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(asyncJob), + obj->def->name, + libxlDomainJobTypeToString(priv->job.active), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob), + priv->job.owner, priv->job.asyncOwner); + + if (errno == ETIMEDOUT) + virReportError(VIR_ERR_OPERATION_TIMEOUT, + "%s", _("cannot acquire state change lock")); + else + virReportSystemError(errno, + "%s", _("cannot acquire job mutex")); + if (driver_locked) { + virDomainObjUnlock(obj); + libxlDriverLock(driver); + virDomainObjLock(obj); + } + virObjectUnref(obj); + return -1; +} + +/* + * 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 + */ +static int +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainJob job) +{ + return libxlDomainObjBeginJobInternal(driver, false, obj, job, + LIBXL_ASYNC_JOB_NONE); +} + +static int ATTRIBUTE_UNUSED +libxlDomainObjBeginAsyncJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainAsyncJob asyncJob) +{ + return libxlDomainObjBeginJobInternal(driver, false, obj, LIBXL_JOB_ASYNC, + asyncJob); +} + +/* + * obj must be locked before calling. If libxlDriverPrivatePtr is passed, it + * MUST be locked; otherwise it MUST NOT be locked.

IMO, libxlDriverPrivatePtr should always be passed, in which case it MUST 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 + */ +static int +libxlDomainObjBeginJobWithDriver(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainJob job) +{ + if (job <= LIBXL_JOB_NONE || job >= LIBXL_JOB_ASYNC) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempt to start invalid job")); + return -1; + } + + return libxlDomainObjBeginJobInternal(driver, true, obj, job, + LIBXL_ASYNC_JOB_NONE); +} + +static int +libxlDomainObjBeginAsyncJobWithDriver(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainAsyncJob asyncJob) +{ + return libxlDomainObjBeginJobInternal(driver, true, obj, LIBXL_JOB_ASYNC, + asyncJob); +} + +/* + * obj must be locked before calling, libxlDriverPrivatePtr does not matter + * + * To be called after completing the work associated with the + * earlier libxlDomainBeginJob() call + * + * Returns remaining refcount on 'obj', maybe 0 to indicated it + * was deleted + */ +static bool +libxlDomainObjEndJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + enum libxlDomainJob job = priv->job.active; + + VIR_DEBUG("Stopping job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + + libxlDomainObjResetJob(priv); + if (libxlDomainTrackJob(job)) + libxlDomainObjSaveJob(driver, obj); + virCondSignal(&priv->job.cond); + + return virObjectUnref(obj); +} + +static bool +libxlDomainObjEndAsyncJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + + VIR_DEBUG("Stopping async job: %s", + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + + libxlDomainObjResetAsyncJob(priv); + libxlDomainObjSaveJob(driver, obj); + virCondBroadcast(&priv->job.asyncCond); + + return virObjectUnref(obj); +} + +static int ATTRIBUTE_UNUSED +libxlMigrationJobStart(libxlDriverPrivatePtr driver, + virDomainObjPtr vm, + enum libxlDomainAsyncJob job)

This function is not used in the patch series and should be removed.

+{ + libxlDomainObjPrivatePtr priv = vm->privateData; + + if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, job) < 0) + return -1; + + libxlDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK | + JOB_MASK(LIBXL_JOB_MIGRATION_OP)); + + priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED; + + return 0; +} + +static bool ATTRIBUTE_UNUSED +libxlMigrationJobFinish(libxlDriverPrivatePtr driver, virDomainObjPtr vm)

Same here, not used so should be removed.

+{ + return libxlDomainObjEndAsyncJob(driver, vm); +} +/* job function finish */ + static void * libxlDomainObjPrivateAlloc(void) { @@ -92,11 +423,18 @@ libxlDomainObjPrivateAlloc(void) if (VIR_ALLOC(priv) < 0) return NULL;

+ if (libxlDomainObjInitJob(priv) < 0) + goto error; + libxl_ctx_init(&priv->ctx, LIBXL_VERSION, libxl_driver->logger); priv->waiterFD = -1; priv->eventHdl = -1;

return priv; + +error: + VIR_FREE(priv); + return NULL; }

static void @@ -114,6 +452,7 @@ libxlDomainObjPrivateFree(void *data) }

libxl_ctx_free(&priv->ctx); + libxlDomainObjFreeJob(priv); VIR_FREE(priv); }

@@ -3830,6 +4169,111 @@ cleanup: }

static int +libxlDomainGetJobInfo(virDomainPtr dom, + virDomainJobInfoPtr info) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + libxlDomainObjPrivatePtr priv; + + libxlDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + libxlDriverUnlock(driver); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + priv = vm->privateData; + + if (virDomainObjIsActive(vm)) { + if (priv->job.asyncJob) { + memcpy(info, &priv->job.info, sizeof(*info)); + + /* Refresh elapsed time again just to ensure it + * is fully updated. This is primarily for benefit + * of incoming migration which we don't currently + * monitor actively in the background thread + */ + if (virTimeMillisNow(&info->timeElapsed) < 0) + goto cleanup; + info->timeElapsed -= priv->job.start; + } else { + memset(info, 0, sizeof(*info)); + info->type = VIR_DOMAIN_JOB_NONE; + } + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + +static int +libxlDomainAbortJob(virDomainPtr dom) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + libxlDomainObjPrivatePtr priv; + + libxlDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + libxlDriverUnlock(driver); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_ABORT) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + priv = vm->privateData; + + if (!priv->job.asyncJob) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("no job is active on the domain")); + goto endjob; + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + _("cannot abort %s; use virDomainDestroy instead"), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + goto endjob; + }

This function will always fail with the above logic. ret is initialized to -1 and is never changed. Is it even possible to safely abort a libxl operation? If not, this function should probably remain unimplemented. Maybe it will be useful when the libxl driver supports migration. Regards, Jim

+ + VIR_DEBUG("Not job could be cancelled at current version"); + +endjob: + if (libxlDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + +static int libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, virConnectDomainEventGenericCallback callback, void *opaque, virFreeCallback freecb) @@ -3963,6 +4407,8 @@ static virDriver libxlDriver = { .domainIsActive = libxlDomainIsActive, /* 0.9.0 */ .domainIsPersistent = libxlDomainIsPersistent, /* 0.9.0 */ .domainIsUpdated = libxlDomainIsUpdated, /* 0.9.0 */ + .domainGetJobInfo = libxlDomainGetJobInfo, /* 0.10.0 */ + .domainAbortJob = libxlDomainAbortJob, /* 0.10.0 */ .domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */ .domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */ .isAlive = libxlIsAlive, /* 0.9.8 */

...

...

Jim Fehlig <jfehlig@suse.com> wrote: Bamvor Jian Zhang wrote: This patch introduce a lock for protecting the long-running api (save, dump, migration and so on) from the other api which may update the status of the virtual machine.

Hi Bamvor,

Thanks for the patches and sorry for the delayed response. I've been traveling quite a bit lately and just got around to reviewing and testing your work.

Testing so far looks good. I can save and dump vm's while at the same time list and retrieve info.

See my comments inline, but did want to raise a more general comment first. There is a quite a bit of code here borrowed from the qemu driver, which in general is fine since the libxl driver does not need the same locking features as the qemu one. I'd like to hear the opinion of other libvirt maintainers wrt the duplicated code.

...

Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com> --- src/libxl/libxl_conf.h | 58 ++++++ src/libxl/libxl_driver.c | 446 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 504 insertions(+)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 56bf85c..dab1466 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -73,6 +73,62 @@ struct _libxlDriverPrivate { char *saveDir; };

+# define JOB_MASK(job) (1 << (job - 1)) +# define DEFAULT_JOB_MASK \ + (JOB_MASK(LIBXL_JOB_DESTROY) | \ + JOB_MASK(LIBXL_JOB_ABORT)) + +/* Jobs which have to be tracked in domain state XML. */ +# define LIBXL_DOMAIN_TRACK_JOBS \ + (JOB_MASK(LIBXL_JOB_DESTROY) | \ + JOB_MASK(LIBXL_JOB_ASYNC)) + +/* 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_DESTROY, /* Destroys the domain (cannot be masked out) */ + LIBXL_JOB_MODIFY, /* May change state */ + LIBXL_JOB_ABORT, /* Abort current async job */ + LIBXL_JOB_MIGRATION_OP, /* Operation influencing outgoing migration */ + + /* The following two items must always be the last items before JOB_LAST */ + LIBXL_JOB_ASYNC, /* Asynchronous job */

There is only one item, so the comment needs updated.

...

+ + LIBXL_JOB_LAST +}; +VIR_ENUM_DECL(libxlDomainJob) + +/* Async job consists of a series of jobs that may change state. Independent + * jobs that do not change state (and possibly others if explicitly allowed by + * current async job) are allowed to be run even if async job is active. + */ +enum libxlDomainAsyncJob { + LIBXL_ASYNC_JOB_NONE = 0, + LIBXL_ASYNC_JOB_MIGRATION_OUT, + LIBXL_ASYNC_JOB_MIGRATION_IN,

The libxl driver doesn't currently support migration. These could be added in a patch set implementing migration.

...

+ LIBXL_ASYNC_JOB_SAVE, + LIBXL_ASYNC_JOB_DUMP,

IMO, LIBXL_ASYNC_JOB_RESTORE should be added to this list. I see It is omitted in the qemu driver as well, so perhaps there is a good reason for it? i do not know why it is omitted by qemu driver. but for libxl driver, it is hard to let other operation execution even if it is a async job. because virDomainObjPtr is used by libxlVmStart, especially by

Hi, Jim thanks your reply. i have update my patch and resend to the list. besides, there are two comments from you not modify in my v2 patch. see my comments below. libxl_domain_create_restore.

...

+ + LIBXL_ASYNC_JOB_LAST +}; +VIR_ENUM_DECL(libxlDomainAsyncJob) + +struct libxlDomainJobObj { + virCond cond; /* Use to coordinate jobs */ + enum libxlDomainJob active; /* Currently running job */ + int owner; /* Thread which set current job */ + + virCond asyncCond; /* Use to coordinate with async jobs */ + enum libxlDomainAsyncJob asyncJob; /* Currently active async job */ + int asyncOwner; /* Thread which set current async job */ + int phase; /* Job phase (mainly for migrations) */ + unsigned long long mask; /* Jobs allowed during async job */ + unsigned long long start; /* When the async job started */ + virDomainJobInfo info; /* Async job progress data */ +}; + typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate; typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr; struct _libxlDomainObjPrivate { @@ -81,6 +137,8 @@ struct _libxlDomainObjPrivate { libxl_waiter *dWaiter; int waiterFD; int eventHdl; + + struct libxlDomainJobObj job; };

# define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r" diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 6fe284f..d01d915 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -45,6 +45,7 @@ #include "xen_xm.h" #include "virtypedparam.h" #include "viruri.h" +#include "virtime.h"

#define VIR_FROM_THIS VIR_FROM_LIBXL

@@ -84,6 +85,336 @@ libxlDriverUnlock(libxlDriverPrivatePtr driver) virMutexUnlock(&driver->lock); }

+/* job */ +VIR_ENUM_IMPL(libxlDomainJob, LIBXL_JOB_LAST, + "none", + "destroy", + "modify", + "abort", + "migration operation", + "none", /* async job is never stored in job.active */ +); + +VIR_ENUM_IMPL(libxlDomainAsyncJob, LIBXL_ASYNC_JOB_LAST, + "none", + "migration out", + "migration in",

Can be added with a migration implementation.

...

+ "save", + "dump",

"restore"?

...

+); + +static int +libxlDomainObjInitJob(libxlDomainObjPrivatePtr priv) +{ + memset(&priv->job, 0, sizeof(priv->job)); + + if (virCondInit(&priv->job.cond) < 0) + return -1; + + if (virCondInit(&priv->job.asyncCond) < 0) { + ignore_value(virCondDestroy(&priv->job.cond)); + return -1; + } + + return 0; +} + +static void +libxlDomainObjResetJob(libxlDomainObjPrivatePtr priv) +{ + struct libxlDomainJobObj *job = &priv->job; + + job->active = LIBXL_JOB_NONE; + job->owner = 0; +} + +static void +libxlDomainObjResetAsyncJob(libxlDomainObjPrivatePtr priv) +{ + struct libxlDomainJobObj *job = &priv->job; + + job->asyncJob = LIBXL_ASYNC_JOB_NONE; + job->asyncOwner = 0; + job->phase = 0; + job->mask = DEFAULT_JOB_MASK; + job->start = 0; + memset(&job->info, 0, sizeof(job->info)); +} + +static void +libxlDomainObjFreeJob(libxlDomainObjPrivatePtr priv) +{ + ignore_value(virCondDestroy(&priv->job.cond)); + ignore_value(virCondDestroy(&priv->job.asyncCond)); +} + +static bool +libxlDomainTrackJob(enum libxlDomainJob job) +{ + return (LIBXL_DOMAIN_TRACK_JOBS & JOB_MASK(job)) != 0; +} + +static void +libxlDomainObjSaveJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) +{ + if (!virDomainObjIsActive(obj)) { + /* don't write the state file yet, it will be written once the domain + * gets activated */ + return; + } + + if (virDomainSaveStatus(driver->caps, driver->stateDir, obj) < 0) + VIR_WARN("Failed to save status on vm %s", obj->def->name); +} + +static bool +libxlDomainNestedJobAllowed(libxlDomainObjPrivatePtr priv, enum libxlDomainJob job) +{ + return !priv->job.asyncJob || (priv->job.mask & JOB_MASK(job)) != 0; +} + +static void +libxlDomainObjSetAsyncJobMask(virDomainObjPtr obj, + unsigned long long allowedJobs) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + + if (!priv->job.asyncJob) + return; + + priv->job.mask = allowedJobs | JOB_MASK(LIBXL_JOB_DESTROY); +} + +/* Give up waiting for mutex after 30 seconds */ +#define LIBXL_JOB_WAIT_TIME (1000ull * 30) + +/* + * obj must be locked before calling; driver_locked says if libxlDriverPrivatePtr + * is locked or not. + */ +static int ATTRIBUTE_NONNULL(1) +libxlDomainObjBeginJobInternal(libxlDriverPrivatePtr driver, + bool driver_locked, + virDomainObjPtr obj, + enum libxlDomainJob job, + enum libxlDomainAsyncJob asyncJob) +{ + 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); + if (driver_locked) { + libxlDriverUnlock(driver); + } + +retry: + while (!libxlDomainNestedJobAllowed(priv, job)) { + VIR_DEBUG("Wait async job condition for starting job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (virCondWaitUntil(&priv->job.asyncCond, &obj->lock, then) < 0) + goto error; + } + + while (priv->job.active) { + VIR_DEBUG("Wait normal job condition for starting job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + if (virCondWaitUntil(&priv->job.cond, &obj->lock, then) < 0) + goto error; + } + + /* No job is active but a new async job could have been started while obj + * was unlocked, so we need to recheck it. */ + if (!libxlDomainNestedJobAllowed(priv, job)) + goto retry; + + libxlDomainObjResetJob(priv); + + if (job != LIBXL_JOB_ASYNC) { + VIR_DEBUG("Starting job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + priv->job.active = job; + priv->job.owner = virThreadSelfID(); + } else { + VIR_DEBUG("Starting async job: %s", + libxlDomainAsyncJobTypeToString(asyncJob)); + libxlDomainObjResetAsyncJob(priv); + priv->job.asyncJob = asyncJob; + priv->job.asyncOwner = virThreadSelfID(); + priv->job.start = now; + } + + if (driver_locked) { + virDomainObjUnlock(obj); + libxlDriverLock(driver); + virDomainObjLock(obj); + } + + if (libxlDomainTrackJob(job)) + libxlDomainObjSaveJob(driver, obj); + + return 0; + +error: + VIR_WARN("Cannot start job (%s, %s) for domain %s;" + " current job is (%s, %s) owned by (%d, %d)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(asyncJob), + obj->def->name, + libxlDomainJobTypeToString(priv->job.active), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob), + priv->job.owner, priv->job.asyncOwner); + + if (errno == ETIMEDOUT) + virReportError(VIR_ERR_OPERATION_TIMEOUT, + "%s", _("cannot acquire state change lock")); + else + virReportSystemError(errno, + "%s", _("cannot acquire job mutex")); + if (driver_locked) { + virDomainObjUnlock(obj); + libxlDriverLock(driver); + virDomainObjLock(obj); + } + virObjectUnref(obj); + return -1; +} + +/* + * 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 + */ +static int +libxlDomainObjBeginJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainJob job) +{ + return libxlDomainObjBeginJobInternal(driver, false, obj, job, + LIBXL_ASYNC_JOB_NONE); +} + +static int ATTRIBUTE_UNUSED +libxlDomainObjBeginAsyncJob(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainAsyncJob asyncJob) +{ + return libxlDomainObjBeginJobInternal(driver, false, obj, LIBXL_JOB_ASYNC, + asyncJob); +} + +/* + * obj must be locked before calling. If libxlDriverPrivatePtr is passed, it + * MUST be locked; otherwise it MUST NOT be locked.

IMO, libxlDriverPrivatePtr should always be passed, in which case it MUST 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 + */ +static int +libxlDomainObjBeginJobWithDriver(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainJob job) +{ + if (job <= LIBXL_JOB_NONE || job >= LIBXL_JOB_ASYNC) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Attempt to start invalid job")); + return -1; + } + + return libxlDomainObjBeginJobInternal(driver, true, obj, job, + LIBXL_ASYNC_JOB_NONE); +} + +static int +libxlDomainObjBeginAsyncJobWithDriver(libxlDriverPrivatePtr driver, + virDomainObjPtr obj, + enum libxlDomainAsyncJob asyncJob) +{ + return libxlDomainObjBeginJobInternal(driver, true, obj, LIBXL_JOB_ASYNC, + asyncJob); +} + +/* + * obj must be locked before calling, libxlDriverPrivatePtr does not matter + * + * To be called after completing the work associated with the + * earlier libxlDomainBeginJob() call + * + * Returns remaining refcount on 'obj', maybe 0 to indicated it + * was deleted + */ +static bool +libxlDomainObjEndJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + enum libxlDomainJob job = priv->job.active; + + VIR_DEBUG("Stopping job: %s (async=%s)", + libxlDomainJobTypeToString(job), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + + libxlDomainObjResetJob(priv); + if (libxlDomainTrackJob(job)) + libxlDomainObjSaveJob(driver, obj); + virCondSignal(&priv->job.cond); + + return virObjectUnref(obj); +} + +static bool +libxlDomainObjEndAsyncJob(libxlDriverPrivatePtr driver, virDomainObjPtr obj) +{ + libxlDomainObjPrivatePtr priv = obj->privateData; + + VIR_DEBUG("Stopping async job: %s", + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + + libxlDomainObjResetAsyncJob(priv); + libxlDomainObjSaveJob(driver, obj); + virCondBroadcast(&priv->job.asyncCond); + + return virObjectUnref(obj); +} + +static int ATTRIBUTE_UNUSED +libxlMigrationJobStart(libxlDriverPrivatePtr driver, + virDomainObjPtr vm, + enum libxlDomainAsyncJob job)

This function is not used in the patch series and should be removed.

...

+{ + libxlDomainObjPrivatePtr priv = vm->privateData; + + if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, job) < 0) + return -1; + + libxlDomainObjSetAsyncJobMask(vm, DEFAULT_JOB_MASK | + JOB_MASK(LIBXL_JOB_MIGRATION_OP)); + + priv->job.info.type = VIR_DOMAIN_JOB_UNBOUNDED; + + return 0; +} + +static bool ATTRIBUTE_UNUSED +libxlMigrationJobFinish(libxlDriverPrivatePtr driver, virDomainObjPtr vm)

Same here, not used so should be removed.

...

+{ + return libxlDomainObjEndAsyncJob(driver, vm); +} +/* job function finish */ + static void * libxlDomainObjPrivateAlloc(void) { @@ -92,11 +423,18 @@ libxlDomainObjPrivateAlloc(void) if (VIR_ALLOC(priv) < 0) return NULL;

+ if (libxlDomainObjInitJob(priv) < 0) + goto error; + libxl_ctx_init(&priv->ctx, LIBXL_VERSION, libxl_driver->logger); priv->waiterFD = -1; priv->eventHdl = -1;

return priv; + +error: + VIR_FREE(priv); + return NULL; }

static void @@ -114,6 +452,7 @@ libxlDomainObjPrivateFree(void *data) }

libxl_ctx_free(&priv->ctx); + libxlDomainObjFreeJob(priv); VIR_FREE(priv); }

@@ -3830,6 +4169,111 @@ cleanup: }

static int +libxlDomainGetJobInfo(virDomainPtr dom, + virDomainJobInfoPtr info) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + libxlDomainObjPrivatePtr priv; + + libxlDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + libxlDriverUnlock(driver); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + priv = vm->privateData; + + if (virDomainObjIsActive(vm)) { + if (priv->job.asyncJob) { + memcpy(info, &priv->job.info, sizeof(*info)); + + /* Refresh elapsed time again just to ensure it + * is fully updated. This is primarily for benefit + * of incoming migration which we don't currently + * monitor actively in the background thread + */ + if (virTimeMillisNow(&info->timeElapsed) < 0) + goto cleanup; + info->timeElapsed -= priv->job.start; + } else { + memset(info, 0, sizeof(*info)); + info->type = VIR_DOMAIN_JOB_NONE; + } + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto cleanup; + } + + ret = 0; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + +static int +libxlDomainAbortJob(virDomainPtr dom) +{ + libxlDriverPrivatePtr driver = dom->conn->privateData; + virDomainObjPtr vm; + int ret = -1; + libxlDomainObjPrivatePtr priv; + + libxlDriverLock(driver); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + libxlDriverUnlock(driver); + if (!vm) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(dom->uuid, uuidstr); + virReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_ABORT) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is not running")); + goto endjob; + } + + priv = vm->privateData; + + if (!priv->job.asyncJob) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("no job is active on the domain")); + goto endjob; + } else { + virReportError(VIR_ERR_OPERATION_INVALID, + _("cannot abort %s; use virDomainDestroy instead"), + libxlDomainAsyncJobTypeToString(priv->job.asyncJob)); + goto endjob; + }

This function will always fail with the above logic. ret is initialized to -1 and is never changed.

Is it even possible to safely abort a libxl operation? If not, this function should probably remain unimplemented. Maybe it will be useful when the libxl driver supports migration. return error because of the there is no cancelation opeartion in libvirt libxl driver with xen 4.1. according to xen4.2 release document, maybe the cancelation of long-running jobs is supported. but it is still useful for save, dump and migration(in future), because libvirt should block the user abort operation othervise xenlight might crash

Regards, Jim

...

+ + VIR_DEBUG("Not job could be cancelled at current version"); + +endjob: + if (libxlDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + if (vm) + virDomainObjUnlock(vm); + return ret; +} + +static int libxlDomainEventRegisterAny(virConnectPtr conn, virDomainPtr dom, int eventID, virConnectDomainEventGenericCallback callback, void *opaque, virFreeCallback freecb) @@ -3963,6 +4407,8 @@ static virDriver libxlDriver = { .domainIsActive = libxlDomainIsActive, /* 0.9.0 */ .domainIsPersistent = libxlDomainIsPersistent, /* 0.9.0 */ .domainIsUpdated = libxlDomainIsUpdated, /* 0.9.0 */ + .domainGetJobInfo = libxlDomainGetJobInfo, /* 0.10.0 */ + .domainAbortJob = libxlDomainAbortJob, /* 0.10.0 */ .domainEventRegisterAny = libxlDomainEventRegisterAny, /* 0.9.0 */ .domainEventDeregisterAny = libxlDomainEventDeregisterAny, /* 0.9.0 */ .isAlive = libxlIsAlive, /* 0.9.8 */

Bamvor Jian Zhang wrote:

Add long-running jobs for save, dump. Add normal job for the api maybe modify the domain.

Signed-off-by: Bamvor Jian Zhang <bjzhang@suse.com> --- src/libxl/libxl_driver.c | 219 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 158 insertions(+), 61 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index d01d915..28e50b0 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1534,9 +1534,13 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, goto cleanup; def = NULL;

+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1) < 0) { - virDomainRemoveInactive(&driver->domains, vm); + if (libxlDomainObjEndJob(driver, vm)) + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; goto cleanup; } @@ -1545,6 +1549,8 @@ libxlDomainCreateXML(virConnectPtr conn, const char *xml, if (dom) dom->id = vm->def->id;

+ if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; cleanup: virDomainDefFree(def); if (vm) @@ -1651,9 +1657,13 @@ libxlDomainSuspend(virDomainPtr dom) _("No domain with matching uuid '%s'"), uuidstr); 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; }

IMO, we should check if the domain is active before calling libxlDomainObjBeginJob().

priv = vm->privateData; @@ -1663,7 +1673,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); @@ -1673,10 +1683,14 @@ libxlDomainSuspend(virDomainPtr dom) }

if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) - goto cleanup; + goto endjob;

ret = 0;

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virDomainObjUnlock(vm); @@ -1710,9 +1724,12 @@ libxlDomainResume(virDomainPtr dom) 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; }

Same here.

priv = vm->privateData; @@ -1722,7 +1739,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, @@ -1733,10 +1750,14 @@ libxlDomainResume(virDomainPtr dom) }

if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) - goto cleanup; + goto endjob;

ret = 0;

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virDomainObjUnlock(vm); @@ -1768,10 +1789,13 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) goto cleanup; }

+ if (libxlDomainObjBeginJobWithDriver(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; }

And here.

priv = vm->privateData; @@ -1779,7 +1803,7 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to shutdown domain '%d' with libxenlight"), dom->id); - goto cleanup; + goto endjob; }

/* vm is marked shutoff (or removed from domains list if not persistent) @@ -1787,6 +1811,10 @@ libxlDomainShutdownFlags(virDomainPtr dom, unsigned int flags) */ ret = 0;

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virDomainObjUnlock(vm); @@ -1821,10 +1849,13 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) goto cleanup; }

+ if (libxlDomainObjBeginJobWithDriver(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; }

And here.

priv = vm->privateData; @@ -1832,10 +1863,14 @@ libxlDomainReboot(virDomainPtr dom, unsigned int flags) virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to reboot domain '%d' with libxenlight"), dom->id); - goto cleanup; + goto endjob; } ret = 0;

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virDomainObjUnlock(vm); @@ -1864,10 +1899,13 @@ libxlDomainDestroyFlags(virDomainPtr dom, goto cleanup; }

+ if (libxlDomainObjBeginJobWithDriver(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; }

I think you get the picture :).

event = virDomainEventNewFromObj(vm,VIR_DOMAIN_EVENT_STOPPED, @@ -1876,16 +1914,21 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_DESTROYED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup; + goto endjob; }

if (!vm->persistent) { - virDomainRemoveInactive(&driver->domains, vm); + if (libxlDomainObjEndJob(driver, vm)) + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; }

ret = 0;

+endjob: + if ( vm && !libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virDomainObjUnlock(vm); @@ -1975,6 +2018,9 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, goto cleanup; }

+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + isActive = virDomainObjIsActive(vm);

if (flags == VIR_DOMAIN_MEM_CURRENT) { @@ -1993,17 +2039,17 @@ 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(driver->caps, vm))) - goto cleanup; + goto endjob; }

if (flags & VIR_DOMAIN_MEM_MAXIMUM) { @@ -2015,7 +2061,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; } }

@@ -2026,7 +2072,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (persistentDef->mem.cur_balloon > newmem) persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(driver->configDir, persistentDef); - goto cleanup; + goto endjob; }

} else { @@ -2035,7 +2081,7 @@ 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) { @@ -2045,7 +2091,7 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set memory for domain '%d'" " with libxenlight"), dom->id); - goto cleanup; + goto endjob; } }

@@ -2053,11 +2099,14 @@ libxlDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, sa_assert(persistentDef); persistentDef->mem.cur_balloon = newmem; ret = virDomainSaveConfig(driver->configDir, persistentDef); - goto cleanup; + goto endjob; } }

ret = 0; +endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL;

cleanup: if (vm) @@ -2165,22 +2214,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, int fd; int ret = -1;

+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, + LIBXL_ASYNC_JOB_SAVE) < 0) + goto cleanup; + if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) { virReportError(VIR_ERR_OPERATION_INVALID, _("Domain '%d' has to be running because libxenlight will" " suspend it"), vm->def->id); - goto cleanup; + goto endjob; }

if ((fd = virFileOpenAs(to, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR, -1, -1, 0)) < 0) { virReportSystemError(-fd, _("Failed to create domain save file '%s'"), to); - goto cleanup; + goto endjob; }

if ((xml = virDomainDefFormat(vm->def, 0)) == NULL) - goto cleanup; + goto endjob; xml_len = strlen(xml) + 1;

memset(&hdr, 0, sizeof(hdr)); @@ -2191,20 +2244,26 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (safewrite(fd, &hdr, sizeof(hdr)) != sizeof(hdr)) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Failed to write save file header")); - goto cleanup; + goto endjob; }

if (safewrite(fd, xml, xml_len) != xml_len) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Failed to write xml description")); - goto cleanup; + goto endjob; }

- if (libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd) != 0) { + virDomainObjUnlock(vm); + libxlDriverUnlock(driver); + ret = libxl_domain_suspend(&priv->ctx, NULL, vm->def->id, fd); + libxlDriverLock(driver); + virDomainObjLock(vm); + + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to save domain '%d' with libxenlight"), vm->def->id); - goto cleanup; + goto endjob; }

event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -2213,18 +2272,23 @@ libxlDoDomainSave(libxlDriverPrivatePtr driver, virDomainObjPtr vm, if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_SAVED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), vm->def->id); - goto cleanup; + goto endjob; }

vm->hasManagedSave = true;

if (!vm->persistent) { - virDomainRemoveInactive(&driver->domains, vm); + if (libxlDomainObjEndAsyncJob(driver, vm)) + virDomainRemoveInactive(&driver->domains, vm); vm = NULL; }

ret = 0;

+endjob: + if ( vm && !libxlDomainObjEndAsyncJob(driver, vm)) + vm = NULL; + cleanup: VIR_FREE(xml); if (VIR_CLOSE(fd) < 0) @@ -2312,12 +2376,18 @@ libxlDomainRestoreFlags(virConnectPtr conn, const char *from,

def = NULL;

+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup;

From my testing, this prevents listing domains, getting domain info, etc. while restoring the domain. As mentioned in patch 1, we could use LIBXL_ASYNC_JOB_RESTORE here.

+ if ((ret = libxlVmStart(driver, vm, false, fd)) < 0 && !vm->persistent) { - virDomainRemoveInactive(&driver->domains, vm); + if (libxlDomainObjEndAsyncJob(driver, vm)) + virDomainRemoveInactive(&driver->domains, vm);

Shouldn't we call virDomainRemoveInactive() even if libxlDomainObjEndAsyncJob() fails?

vm = NULL; }

+ if (vm && !libxlDomainObjEndJob(driver, vm)) + vm = NULL; cleanup: if (VIR_CLOSE(fd) < 0) virReportSystemError(errno, "%s", _("cannot close file")); @@ -2348,7 +2418,6 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags)

libxlDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); - libxlDriverUnlock(driver);

if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; @@ -2358,9 +2427,13 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) goto cleanup; }

+ if (libxlDomainObjBeginAsyncJobWithDriver(driver, vm, + LIBXL_ASYNC_JOB_DUMP) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); - goto cleanup; + goto endjob; }

priv = vm->privateData; @@ -2372,25 +2445,29 @@ 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) != 0) { + virDomainObjUnlock(vm); + libxlDriverUnlock(driver); + ret = libxl_domain_core_dump(&priv->ctx, dom->id, to); + libxlDriverLock(driver); + virDomainObjLock(vm); + if (ret != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to dump core of domain '%d' with libxenlight"), dom->id); - goto cleanup_unpause; + goto endjob_unpause; }

- libxlDriverLock(driver); if (flags & VIR_DUMP_CRASH) { if (libxlVmReap(driver, vm, 1, VIR_DOMAIN_SHUTOFF_CRASHED) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), dom->id); - goto cleanup_unlock; + goto endjob_unpause; }

event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -2398,15 +2475,14 @@ libxlDomainCoreDump(virDomainPtr dom, const char *to, unsigned int flags) }

if ((flags & VIR_DUMP_CRASH) && !vm->persistent) { - virDomainRemoveInactive(&driver->domains, vm); + if (libxlDomainObjEndAsyncJob(driver, vm)) + virDomainRemoveInactive(&driver->domains, vm);

Same comment here about conditionally calling virDomainRemoveInactive(). Regards, Jim

vm = NULL; }

ret = 0;

-cleanup_unlock: - libxlDriverUnlock(driver); -cleanup_unpause: +endjob_unpause: if (virDomainObjIsActive(vm) && paused) { if (libxl_domain_unpause(&priv->ctx, dom->id) != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -2417,14 +2493,15 @@ cleanup_unpause: VIR_DOMAIN_RUNNING_UNPAUSED); } } +endjob: + if (vm && !libxlDomainObjEndAsyncJob(driver, vm)) + vm = NULL; cleanup: if (vm) virDomainObjUnlock(vm); - if (event) { - libxlDriverLock(driver); + if (event) libxlDomainEventQueue(driver, event); - libxlDriverUnlock(driver); - } + libxlDriverUnlock(driver); return ret; }

@@ -2601,22 +2678,25 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, 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 = libxlGetMaxVcpus(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) { @@ -2627,18 +2707,18 @@ 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(driver->caps, vm))) - goto cleanup; + goto endjob;

maplen = VIR_CPU_MAPLEN(nvcpus); if (VIR_ALLOC_N(bitmask, maplen) < 0) { virReportOOMError(); - goto cleanup; + goto endjob; }

for (i = 0; i < nvcpus; ++i) { @@ -2665,7 +2745,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;

@@ -2674,7 +2754,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; @@ -2685,6 +2765,9 @@ libxlDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (flags & VIR_DOMAIN_VCPU_CONFIG) ret = virDomainSaveConfig(driver->configDir, def);

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; cleanup: VIR_FREE(bitmask); if (vm) @@ -3053,14 +3136,21 @@ libxlDomainCreateWithFlags(virDomainPtr dom, goto cleanup; }

+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is already running")); - goto cleanup; + goto endjob; }

ret = libxlVmStart(driver, vm, (flags & VIR_DOMAIN_START_PAUSED) != 0, -1);

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virDomainObjUnlock(vm); @@ -3589,6 +3679,9 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, goto cleanup; }

+ if (libxlDomainObjBeginJobWithDriver(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (virDomainObjIsActive(vm)) { if (flags == VIR_DOMAIN_DEVICE_MODIFY_CURRENT) flags |= VIR_DOMAIN_DEVICE_MODIFY_LIVE; @@ -3599,14 +3692,14 @@ libxlDomainModifyDeviceFlags(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; @@ -3614,11 +3707,11 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, if (flags & VIR_DOMAIN_DEVICE_MODIFY_CONFIG) { if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob;

/* Make a copy for updated domain. */ if (!(vmdef = virDomainObjCopyPersistentDef(driver->caps, vm))) - goto cleanup; + goto endjob;

switch (action) { case LIBXL_DEVICE_ATTACH: @@ -3642,7 +3735,7 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, virDomainDeviceDefFree(dev); if (!(dev = virDomainDeviceDefParse(driver->caps, vm->def, xml, VIR_DOMAIN_XML_INACTIVE))) - goto cleanup; + goto endjob;

switch (action) { case LIBXL_DEVICE_ATTACH: @@ -3675,6 +3768,10 @@ libxlDomainModifyDeviceFlags(virDomainPtr dom, const char *xml, } }

+endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(dev);