mirror of
https://gitlab.com/libvirt/libvirt.git
synced 2024-08-28 19:41:16 +00:00
qemu: qemuDomainRename and virDomainObjListNumOfDomains ABBA deadlock fix
A pretty nasty deadlock occurs while trying to rename a VM in parallel with virDomainObjListNumOfDomains. The short description of the problem is as follows: Thread #1: qemuDomainRename: ------> aquires domain lock by qemuDomObjFromDomain ---------> waits for domain list lock in any of the listed functions: - virDomainObjListFindByName - virDomainObjListRenameAddNew - virDomainObjListRenameRemove Thread #2: virDomainObjListNumOfDomains: ------> aquires domain list lock ---------> waits for domain lock in virDomainObjListCount Introduce generic virDomainObjListRename function for renaming domains. It aquires list lock in right order to avoid deadlock. Callback is used to make driver specific domain updates. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This commit is contained in:
parent
92757d4d2d
commit
1e93470df0
@ -313,40 +313,6 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
int
|
|
||||||
virDomainObjListRenameAddNew(virDomainObjListPtr doms,
|
|
||||||
virDomainObjPtr vm,
|
|
||||||
const char *name)
|
|
||||||
{
|
|
||||||
int ret = -1;
|
|
||||||
virObjectLock(doms);
|
|
||||||
|
|
||||||
/* Add new name into the hash table of domain names. */
|
|
||||||
if (virHashAddEntry(doms->objsName, name, vm) < 0)
|
|
||||||
goto cleanup;
|
|
||||||
|
|
||||||
/* Okay, this is crazy. virHashAddEntry() does not increment
|
|
||||||
* the refcounter of @vm, but virHashRemoveEntry() does
|
|
||||||
* decrement it. We need to work around it. */
|
|
||||||
virObjectRef(vm);
|
|
||||||
|
|
||||||
ret = 0;
|
|
||||||
cleanup:
|
|
||||||
virObjectUnlock(doms);
|
|
||||||
return ret;
|
|
||||||
}
|
|
||||||
|
|
||||||
|
|
||||||
int
|
|
||||||
virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name)
|
|
||||||
{
|
|
||||||
virObjectLock(doms);
|
|
||||||
virHashRemoveEntry(doms->objsName, name);
|
|
||||||
virObjectUnlock(doms);
|
|
||||||
return 0;
|
|
||||||
}
|
|
||||||
|
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* The caller must hold a lock on the driver owning 'doms',
|
* The caller must hold a lock on the driver owning 'doms',
|
||||||
* and must also have locked 'dom', to ensure no one else
|
* and must also have locked 'dom', to ensure no one else
|
||||||
@ -372,6 +338,72 @@ void virDomainObjListRemove(virDomainObjListPtr doms,
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
/**
|
||||||
|
* virDomainObjListRename:
|
||||||
|
*
|
||||||
|
* The caller must hold a lock on dom. Callbacks should not
|
||||||
|
* sleep/wait otherwise operations on all domains will be blocked
|
||||||
|
* as the callback is called with domains lock hold. Domain lock
|
||||||
|
* is dropped/reacquired during this operation thus domain
|
||||||
|
* consistency must not rely on this lock solely.
|
||||||
|
*/
|
||||||
|
int
|
||||||
|
virDomainObjListRename(virDomainObjListPtr doms,
|
||||||
|
virDomainObjPtr dom,
|
||||||
|
const char *new_name,
|
||||||
|
unsigned int flags,
|
||||||
|
virDomainObjListRenameCallback callback,
|
||||||
|
void *opaque)
|
||||||
|
{
|
||||||
|
int ret = -1;
|
||||||
|
char *old_name = NULL;
|
||||||
|
int rc;
|
||||||
|
|
||||||
|
if (STREQ(dom->def->name, new_name)) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
|
||||||
|
_("Can't rename domain to itself"));
|
||||||
|
return ret;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (VIR_STRDUP(old_name, dom->def->name) < 0)
|
||||||
|
return ret;
|
||||||
|
|
||||||
|
/* doms and dom locks must be attained in right order thus relock dom. */
|
||||||
|
/* dom reference is touched for the benefit of those callers that
|
||||||
|
* hold a lock on dom but not refcount it. */
|
||||||
|
virObjectRef(dom);
|
||||||
|
virObjectUnlock(dom);
|
||||||
|
virObjectLock(doms);
|
||||||
|
virObjectLock(dom);
|
||||||
|
virObjectUnref(dom);
|
||||||
|
|
||||||
|
if (virHashLookup(doms->objsName, new_name) != NULL) {
|
||||||
|
virReportError(VIR_ERR_OPERATION_INVALID,
|
||||||
|
_("domain with name '%s' already exists"),
|
||||||
|
new_name);
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (virHashAddEntry(doms->objsName, new_name, dom) < 0)
|
||||||
|
goto cleanup;
|
||||||
|
|
||||||
|
/* Okay, this is crazy. virHashAddEntry() does not increment
|
||||||
|
* the refcounter of @dom, but virHashRemoveEntry() does
|
||||||
|
* decrement it. We need to work around it. */
|
||||||
|
virObjectRef(dom);
|
||||||
|
|
||||||
|
rc = callback(dom, new_name, flags, opaque);
|
||||||
|
virHashRemoveEntry(doms->objsName, rc < 0 ? new_name : old_name);
|
||||||
|
if (rc < 0)
|
||||||
|
goto cleanup;
|
||||||
|
|
||||||
|
ret = 0;
|
||||||
|
cleanup:
|
||||||
|
virObjectUnlock(doms);
|
||||||
|
VIR_FREE(old_name);
|
||||||
|
return ret;
|
||||||
|
}
|
||||||
|
|
||||||
/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove'
|
/* The caller must hold lock on 'doms' in addition to 'virDomainObjListRemove'
|
||||||
* requirements
|
* requirements
|
||||||
*
|
*
|
||||||
|
@ -51,11 +51,17 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
|
|||||||
unsigned int flags,
|
unsigned int flags,
|
||||||
virDomainDefPtr *oldDef);
|
virDomainDefPtr *oldDef);
|
||||||
|
|
||||||
int virDomainObjListRenameAddNew(virDomainObjListPtr doms,
|
typedef int (*virDomainObjListRenameCallback)(virDomainObjPtr dom,
|
||||||
virDomainObjPtr vm,
|
const char *new_name,
|
||||||
const char *name);
|
unsigned int flags,
|
||||||
int virDomainObjListRenameRemove(virDomainObjListPtr doms,
|
void *opaque);
|
||||||
const char *name);
|
int virDomainObjListRename(virDomainObjListPtr doms,
|
||||||
|
virDomainObjPtr dom,
|
||||||
|
const char *new_name,
|
||||||
|
unsigned int flags,
|
||||||
|
virDomainObjListRenameCallback callback,
|
||||||
|
void *opaque);
|
||||||
|
|
||||||
void virDomainObjListRemove(virDomainObjListPtr doms,
|
void virDomainObjListRemove(virDomainObjListPtr doms,
|
||||||
virDomainObjPtr dom);
|
virDomainObjPtr dom);
|
||||||
void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
|
void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
|
||||||
|
@ -894,8 +894,7 @@ virDomainObjListNew;
|
|||||||
virDomainObjListNumOfDomains;
|
virDomainObjListNumOfDomains;
|
||||||
virDomainObjListRemove;
|
virDomainObjListRemove;
|
||||||
virDomainObjListRemoveLocked;
|
virDomainObjListRemoveLocked;
|
||||||
virDomainObjListRenameAddNew;
|
virDomainObjListRename;
|
||||||
virDomainObjListRenameRemove;
|
|
||||||
|
|
||||||
|
|
||||||
# cpu/cpu.h
|
# cpu/cpu.h
|
||||||
|
@ -19999,14 +19999,14 @@ qemuDomainSetUserPassword(virDomainPtr dom,
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
static int qemuDomainRename(virDomainPtr dom,
|
static int
|
||||||
const char *new_name,
|
qemuDomainRenameCallback(virDomainObjPtr vm,
|
||||||
unsigned int flags)
|
const char *new_name,
|
||||||
|
unsigned int flags,
|
||||||
|
void *opaque)
|
||||||
{
|
{
|
||||||
virQEMUDriverPtr driver = dom->conn->privateData;
|
virQEMUDriverPtr driver = opaque;
|
||||||
virQEMUDriverConfigPtr cfg = NULL;
|
virQEMUDriverConfigPtr cfg = NULL;
|
||||||
virDomainObjPtr vm = NULL;
|
|
||||||
virDomainObjPtr tmp_dom = NULL;
|
|
||||||
virObjectEventPtr event_new = NULL;
|
virObjectEventPtr event_new = NULL;
|
||||||
virObjectEventPtr event_old = NULL;
|
virObjectEventPtr event_old = NULL;
|
||||||
int ret = -1;
|
int ret = -1;
|
||||||
@ -20016,14 +20016,75 @@ static int qemuDomainRename(virDomainPtr dom,
|
|||||||
|
|
||||||
virCheckFlags(0, ret);
|
virCheckFlags(0, ret);
|
||||||
|
|
||||||
|
cfg = virQEMUDriverGetConfig(driver);
|
||||||
|
|
||||||
|
if (VIR_STRDUP(new_dom_name, new_name) < 0)
|
||||||
|
goto cleanup;
|
||||||
|
|
||||||
|
if (!(old_dom_cfg_file = virDomainConfigFile(cfg->configDir,
|
||||||
|
vm->def->name))) {
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
|
event_old = virDomainEventLifecycleNewFromObj(vm,
|
||||||
|
VIR_DOMAIN_EVENT_UNDEFINED,
|
||||||
|
VIR_DOMAIN_EVENT_UNDEFINED_RENAMED);
|
||||||
|
|
||||||
|
/* Switch name in domain definition. */
|
||||||
|
old_dom_name = vm->def->name;
|
||||||
|
vm->def->name = new_dom_name;
|
||||||
|
new_dom_name = NULL;
|
||||||
|
|
||||||
|
if (virDomainSaveConfig(cfg->configDir, vm->def) < 0)
|
||||||
|
goto rollback;
|
||||||
|
|
||||||
|
if (virFileExists(old_dom_cfg_file) &&
|
||||||
|
unlink(old_dom_cfg_file) < 0) {
|
||||||
|
virReportSystemError(errno,
|
||||||
|
_("cannot remove old domain config file %s"),
|
||||||
|
old_dom_cfg_file);
|
||||||
|
goto rollback;
|
||||||
|
}
|
||||||
|
|
||||||
|
event_new = virDomainEventLifecycleNewFromObj(vm,
|
||||||
|
VIR_DOMAIN_EVENT_DEFINED,
|
||||||
|
VIR_DOMAIN_EVENT_DEFINED_RENAMED);
|
||||||
|
ret = 0;
|
||||||
|
|
||||||
|
cleanup:
|
||||||
|
VIR_FREE(old_dom_cfg_file);
|
||||||
|
VIR_FREE(old_dom_name);
|
||||||
|
VIR_FREE(new_dom_name);
|
||||||
|
qemuDomainEventQueue(driver, event_old);
|
||||||
|
qemuDomainEventQueue(driver, event_new);
|
||||||
|
virObjectUnref(cfg);
|
||||||
|
return ret;
|
||||||
|
|
||||||
|
rollback:
|
||||||
|
if (old_dom_name) {
|
||||||
|
new_dom_name = vm->def->name;
|
||||||
|
vm->def->name = old_dom_name;
|
||||||
|
old_dom_name = NULL;
|
||||||
|
}
|
||||||
|
goto cleanup;
|
||||||
|
}
|
||||||
|
|
||||||
|
static int qemuDomainRename(virDomainPtr dom,
|
||||||
|
const char *new_name,
|
||||||
|
unsigned int flags)
|
||||||
|
{
|
||||||
|
virQEMUDriverPtr driver = dom->conn->privateData;
|
||||||
|
virDomainObjPtr vm = NULL;
|
||||||
|
int ret = -1;
|
||||||
|
|
||||||
|
virCheckFlags(0, ret);
|
||||||
|
|
||||||
if (!(vm = qemuDomObjFromDomain(dom)))
|
if (!(vm = qemuDomObjFromDomain(dom)))
|
||||||
goto cleanup;
|
goto cleanup;
|
||||||
|
|
||||||
if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0)
|
if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0)
|
||||||
goto cleanup;
|
goto cleanup;
|
||||||
|
|
||||||
cfg = virQEMUDriverGetConfig(driver);
|
|
||||||
|
|
||||||
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
|
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
|
||||||
goto cleanup;
|
goto cleanup;
|
||||||
|
|
||||||
@ -20051,64 +20112,9 @@ static int qemuDomainRename(virDomainPtr dom,
|
|||||||
goto endjob;
|
goto endjob;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (STREQ(vm->def->name, new_name)) {
|
if (virDomainObjListRename(driver->domains, vm, new_name, flags,
|
||||||
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
|
qemuDomainRenameCallback, driver) < 0)
|
||||||
_("Can't rename domain to itself"));
|
|
||||||
goto endjob;
|
goto endjob;
|
||||||
}
|
|
||||||
|
|
||||||
/*
|
|
||||||
* This is a rather racy check, but still better than reporting
|
|
||||||
* internal error. And since new_name != name here, there's no
|
|
||||||
* deadlock imminent.
|
|
||||||
*/
|
|
||||||
tmp_dom = virDomainObjListFindByName(driver->domains, new_name);
|
|
||||||
if (tmp_dom) {
|
|
||||||
virObjectUnlock(tmp_dom);
|
|
||||||
virObjectUnref(tmp_dom);
|
|
||||||
virReportError(VIR_ERR_OPERATION_INVALID,
|
|
||||||
_("domain with name '%s' already exists"),
|
|
||||||
new_name);
|
|
||||||
goto endjob;
|
|
||||||
}
|
|
||||||
|
|
||||||
if (VIR_STRDUP(new_dom_name, new_name) < 0)
|
|
||||||
goto endjob;
|
|
||||||
|
|
||||||
if (!(old_dom_cfg_file = virDomainConfigFile(cfg->configDir,
|
|
||||||
vm->def->name))) {
|
|
||||||
goto endjob;
|
|
||||||
}
|
|
||||||
|
|
||||||
if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0)
|
|
||||||
goto endjob;
|
|
||||||
|
|
||||||
event_old = virDomainEventLifecycleNewFromObj(vm,
|
|
||||||
VIR_DOMAIN_EVENT_UNDEFINED,
|
|
||||||
VIR_DOMAIN_EVENT_UNDEFINED_RENAMED);
|
|
||||||
|
|
||||||
/* Switch name in domain definition. */
|
|
||||||
old_dom_name = vm->def->name;
|
|
||||||
vm->def->name = new_dom_name;
|
|
||||||
new_dom_name = NULL;
|
|
||||||
|
|
||||||
if (virDomainSaveConfig(cfg->configDir, vm->def) < 0)
|
|
||||||
goto rollback;
|
|
||||||
|
|
||||||
if (virFileExists(old_dom_cfg_file) &&
|
|
||||||
unlink(old_dom_cfg_file) < 0) {
|
|
||||||
virReportSystemError(errno,
|
|
||||||
_("cannot remove old domain config file %s"),
|
|
||||||
old_dom_cfg_file);
|
|
||||||
goto rollback;
|
|
||||||
}
|
|
||||||
|
|
||||||
/* Remove old domain name from table. */
|
|
||||||
virDomainObjListRenameRemove(driver->domains, old_dom_name);
|
|
||||||
|
|
||||||
event_new = virDomainEventLifecycleNewFromObj(vm,
|
|
||||||
VIR_DOMAIN_EVENT_DEFINED,
|
|
||||||
VIR_DOMAIN_EVENT_DEFINED_RENAMED);
|
|
||||||
|
|
||||||
/* Success, domain has been renamed. */
|
/* Success, domain has been renamed. */
|
||||||
ret = 0;
|
ret = 0;
|
||||||
@ -20118,23 +20124,7 @@ static int qemuDomainRename(virDomainPtr dom,
|
|||||||
|
|
||||||
cleanup:
|
cleanup:
|
||||||
virDomainObjEndAPI(&vm);
|
virDomainObjEndAPI(&vm);
|
||||||
VIR_FREE(old_dom_cfg_file);
|
|
||||||
VIR_FREE(old_dom_name);
|
|
||||||
VIR_FREE(new_dom_name);
|
|
||||||
qemuDomainEventQueue(driver, event_old);
|
|
||||||
qemuDomainEventQueue(driver, event_new);
|
|
||||||
virObjectUnref(cfg);
|
|
||||||
return ret;
|
return ret;
|
||||||
|
|
||||||
rollback:
|
|
||||||
if (old_dom_name) {
|
|
||||||
new_dom_name = vm->def->name;
|
|
||||||
vm->def->name = old_dom_name;
|
|
||||||
old_dom_name = NULL;
|
|
||||||
}
|
|
||||||
|
|
||||||
virDomainObjListRenameRemove(driver->domains, new_name);
|
|
||||||
goto endjob;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
static virHypervisorDriver qemuHypervisorDriver = {
|
static virHypervisorDriver qemuHypervisorDriver = {
|
||||||
|
Loading…
Reference in New Issue
Block a user