3:16 a.m.
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
The patch establishes a single order of taking locks: driver->domains list
first, then a particular VM. This is done by implementing a set of
virDomainObjListXxxLocked functions working with driver->domains that assume
that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com>
---
src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++--------
src/conf/virdomainobjlist.h | 9 ++++++
src/libvirt_private.syms | 4 +++
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++---
4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 7568f93..89e28a8 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -132,21 +132,19 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
static virDomainObjPtr
-virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
+virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms,
const unsigned char *uuid,
bool ref)
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainObjPtr obj;
- virObjectLock(doms);
virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr);
- if (ref) {
+ if (ref)
virObjectRef(obj);
- virObjectUnlock(doms);
- }
+
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
obj = NULL;
}
}
- if (!ref)
- virObjectUnlock(doms);
+ return obj;
+}
+
+static virDomainObjPtr
+virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
+ const unsigned char *uuid,
+ bool ref)
+{
+ virDomainObjPtr obj;
+
+ virObjectLock(doms);
+ obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref);
+ virObjectUnlock(doms);
return obj;
}
@@ -169,6 +178,13 @@ virDomainObjListFindByUUID(virDomainObjListPtr doms,
return virDomainObjListFindByUUIDInternal(doms, uuid, false);
}
+virDomainObjPtr
+virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms,
+ const unsigned char *uuid)
+{
+ return virDomainObjListFindByUUIDInternalLocked(doms, uuid, true);
+}
+
virDomainObjPtr
virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
@@ -177,6 +193,23 @@ virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
return virDomainObjListFindByUUIDInternal(doms, uuid, true);
}
+virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
+ const char *name)
+{
+ virDomainObjPtr obj;
+
+ obj = virHashLookup(doms->objsName, name);
+ virObjectRef(obj);
+ if (obj) {
+ virObjectLock(obj);
+ if (obj->removing) {
+ virObjectUnlock(obj);
+ virObjectUnref(obj);
+ obj = NULL;
+ }
+ }
+ return obj;
+}
virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
const char *name)
@@ -312,14 +345,12 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
return ret;
}
-
int
-virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+virDomainObjListRenameAddNewLocked(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)
@@ -332,21 +363,38 @@ virDomainObjListRenameAddNew(virDomainObjListPtr doms,
ret = 0;
cleanup:
+ return ret;
+}
+
+int
+virDomainObjListRenameAddNew(virDomainObjListPtr doms,
+ virDomainObjPtr vm,
+ const char *name)
+{
+ int ret = -1;
+ virObjectLock(doms);
+ ret = virDomainObjListRenameAddNewLocked(doms, vm, name);
virObjectUnlock(doms);
return ret;
}
+int
+virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms, const char *name)
+{
+ virHashRemoveEntry(doms->objsName, name);
+ return 0;
+}
int
virDomainObjListRenameRemove(virDomainObjListPtr doms, const char *name)
{
+ int ret;
virObjectLock(doms);
- virHashRemoveEntry(doms->objsName, name);
+ ret = virDomainObjListRenameRemoveLocked(doms, name);
virObjectUnlock(doms);
- return 0;
+ return ret;
}
-
/*
* The caller must hold a lock on the driver owning 'doms',
* and must also have locked 'dom', to ensure no one else
diff --git a/src/conf/virdomainobjlist.h b/src/conf/virdomainobjlist.h
index f479598..bca31cf 100644
--- a/src/conf/virdomainobjlist.h
+++ b/src/conf/virdomainobjlist.h
@@ -38,8 +38,12 @@ virDomainObjPtr virDomainObjListFindByUUID(virDomainObjListPtr doms,
const unsigned char *uuid);
virDomainObjPtr virDomainObjListFindByUUIDRef(virDomainObjListPtr doms,
const unsigned char *uuid);
+virDomainObjPtr virDomainObjListFindByUUIDRefLocked(virDomainObjListPtr doms,
+ const unsigned char *uuid);
virDomainObjPtr virDomainObjListFindByName(virDomainObjListPtr doms,
const char *name);
+virDomainObjPtr virDomainObjListFindByNameLocked(virDomainObjListPtr doms,
+ const char *name);
enum {
VIR_DOMAIN_OBJ_LIST_ADD_LIVE = (1 << 0),
@@ -54,8 +58,13 @@ virDomainObjPtr virDomainObjListAdd(virDomainObjListPtr doms,
int virDomainObjListRenameAddNew(virDomainObjListPtr doms,
virDomainObjPtr vm,
const char *name);
+int virDomainObjListRenameAddNewLocked(virDomainObjListPtr doms,
+ virDomainObjPtr vm,
+ const char *name);
int virDomainObjListRenameRemove(virDomainObjListPtr doms,
const char *name);
+int virDomainObjListRenameRemoveLocked(virDomainObjListPtr doms,
+ const char *name);
void virDomainObjListRemove(virDomainObjListPtr doms,
virDomainObjPtr dom);
void virDomainObjListRemoveLocked(virDomainObjListPtr doms,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5e05a98..b05d6cc 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -881,8 +881,10 @@ virDomainObjListConvert;
virDomainObjListExport;
virDomainObjListFindByID;
virDomainObjListFindByName;
+virDomainObjListFindByNameLocked;
virDomainObjListFindByUUID;
virDomainObjListFindByUUIDRef;
+virDomainObjListFindByUUIDRefLocked;
virDomainObjListForEach;
virDomainObjListGetActiveIDs;
virDomainObjListGetInactiveNames;
@@ -892,7 +894,9 @@ virDomainObjListNumOfDomains;
virDomainObjListRemove;
virDomainObjListRemoveLocked;
virDomainObjListRenameAddNew;
+virDomainObjListRenameAddNewLocked;
virDomainObjListRenameRemove;
+virDomainObjListRenameRemoveLocked;
# cpu/cpu.h
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e46404b..b012516 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -206,6 +206,36 @@ struct qemuAutostartData {
virConnectPtr conn;
};
+/**
+ * qemuDomObjFromDomainLocked:
+ * @domain: Domain pointer that has to be looked up
+ *
+ * This function looks up @domain and returns the appropriate virDomainObjPtr
+ * that has to be released by calling virDomainObjEndAPI().
+ * It assumes that driver->domains list is locked already, thus it uses Locked
+ * variant of virDomainObjListFindByUUIDRef function.
+ *
+ * Returns the domain object with incremented reference counter which is locked
+ * on success, NULL otherwise.
+ */
+static virDomainObjPtr
+qemuDomObjFromDomainLocked(virDomainPtr domain)
+{
+ virDomainObjPtr vm;
+ virQEMUDriverPtr driver = domain->conn->privateData;
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ vm = virDomainObjListFindByUUIDRefLocked(driver->domains, domain->uuid);
+ if (!vm) {
+ virUUIDFormat(domain->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s' (%s)"),
+ uuidstr, domain->name);
+ return NULL;
+ }
+
+ return vm;
+}
/**
* qemuDomObjFromDomain:
@@ -19892,7 +19922,8 @@ static int qemuDomainRename(virDomainPtr dom,
virCheckFlags(0, ret);
- if (!(vm = qemuDomObjFromDomain(dom)))
+ virObjectLock(driver->domains);
+ if (!(vm = qemuDomObjFromDomainLocked(dom)))
goto cleanup;
if (virDomainRenameEnsureACL(dom->conn, vm->def) < 0)
@@ -19938,7 +19969,7 @@ static int qemuDomainRename(virDomainPtr dom,
* internal error. And since new_name != name here, there's no
* deadlock imminent.
*/
- tmp_dom = virDomainObjListFindByName(driver->domains, new_name);
+ tmp_dom = virDomainObjListFindByNameLocked(driver->domains, new_name);
if (tmp_dom) {
virObjectUnlock(tmp_dom);
virObjectUnref(tmp_dom);
@@ -19956,7 +19987,7 @@ static int qemuDomainRename(virDomainPtr dom,
goto endjob;
}
- if (virDomainObjListRenameAddNew(driver->domains, vm, new_name) < 0)
+ if (virDomainObjListRenameAddNewLocked(driver->domains, vm, new_name) < 0)
goto endjob;
event_old = virDomainEventLifecycleNewFromObj(vm,
@@ -19980,7 +20011,7 @@ static int qemuDomainRename(virDomainPtr dom,
}
/* Remove old domain name from table. */
- virDomainObjListRenameRemove(driver->domains, old_dom_name);
+ virDomainObjListRenameRemoveLocked(driver->domains, old_dom_name);
event_new = virDomainEventLifecycleNewFromObj(vm,
VIR_DOMAIN_EVENT_DEFINED,
@@ -20000,6 +20031,7 @@ static int qemuDomainRename(virDomainPtr dom,
qemuDomainEventQueue(driver, event_old);
qemuDomainEventQueue(driver, event_new);
virObjectUnref(cfg);
+ virObjectUnlock(driver->domains);
return ret;
rollback:
--
1.8.3.1
3:29 a.m.
On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
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
Ouch, the rename API should hold the list lock all the time it's doing
something and not acquire it when any domain is locked.
But you are duplicating a lot of code,
The patch establishes a single order of taking locks:
driver->domains list
first, then a particular VM. This is done by implementing a set of
virDomainObjListXxxLocked functions working with driver->domains that assume
that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com>
---
src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++--------
src/conf/virdomainobjlist.h | 9 ++++++
src/libvirt_private.syms | 4 +++
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++---
4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
index 7568f93..89e28a8 100644
--- a/src/conf/virdomainobjlist.c
+++ b/src/conf/virdomainobjlist.c
@@ -132,21 +132,19 @@ virDomainObjPtr virDomainObjListFindByID(virDomainObjListPtr doms,
static virDomainObjPtr
-virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
+virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms,
const unsigned char *uuid,
bool ref)
Indentation is off for renamed functions.
{
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainObjPtr obj;
- virObjectLock(doms);
virUUIDFormat(uuid, uuidstr);
obj = virHashLookup(doms->objs, uuidstr);
- if (ref) {
+ if (ref)
virObjectRef(obj);
- virObjectUnlock(doms);
- }
+
if (obj) {
virObjectLock(obj);
if (obj->removing) {
@@ -156,8 +154,19 @@ virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
obj = NULL;
}
}
- if (!ref)
- virObjectUnlock(doms);
+ return obj;
+}
+
+static virDomainObjPtr
+virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
+ const unsigned char *uuid,
+ bool ref)
+{
+ virDomainObjPtr obj;
+
+ virObjectLock(doms);
+ obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref);
+ virObjectUnlock(doms);
This won't work for ref == true as in that case the unlock is not the
last thing done in virDomainObjListFindByUUIDInternalLocked().
I would rather we didn't invent bunch of new functions whose names
aren't readable, but mayb even preferred open-coding some of them (or
their parts) in the rename itself. The reasoning behind it is that
rename is special API and it touches code that's not exposed normally.
Or the other way would be moving parts of the rename code into this file
to make it available for other drivers as well.
Adding Michal to Cc as the original idea-haver =) to see if we can
agree on that or I am mistaken and your approach is the way to go.
Martin
4:31 a.m.
26.01.2016 12:29, Martin Kletzander пишет:
On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
> 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
>
Ouch, the rename API should hold the list lock all the time it's doing
something and not acquire it when any domain is locked.
But you are duplicating a lot of code,
Not that much
> The patch establishes a single order of taking locks: driver->domains
> list
> first, then a particular VM. This is done by implementing a set of
> virDomainObjListXxxLocked functions working with driver->domains that
> assume
> that list lock is already aquired by calling functions.
>
> Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com>
> ---
> src/conf/virdomainobjlist.c | 74
> +++++++++++++++++++++++++++++++++++++--------
> src/conf/virdomainobjlist.h | 9 ++++++
> src/libvirt_private.syms | 4 +++
> src/qemu/qemu_driver.c | 40 +++++++++++++++++++++---
> 4 files changed, 110 insertions(+), 17 deletions(-)
>
> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
> index 7568f93..89e28a8 100644
> --- a/src/conf/virdomainobjlist.c
> +++ b/src/conf/virdomainobjlist.c
> @@ -132,21 +132,19 @@ virDomainObjPtr
> virDomainObjListFindByID(virDomainObjListPtr doms,
>
>
> static virDomainObjPtr
> -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
> +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms,
> const unsigned char *uuid,
> bool ref)
Indentation is off for renamed functions.
Ok. I'll fix it if we decide to go on with my approach.
> {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virDomainObjPtr obj;
>
> - virObjectLock(doms);
> virUUIDFormat(uuid, uuidstr);
>
> obj = virHashLookup(doms->objs, uuidstr);
> - if (ref) {
> + if (ref)
> virObjectRef(obj);
> - virObjectUnlock(doms);
> - }
> +
> if (obj) {
> virObjectLock(obj);
> if (obj->removing) {
> @@ -156,8 +154,19 @@
> virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
> obj = NULL;
> }
> }
> - if (!ref)
> - virObjectUnlock(doms);
> + return obj;
> +}
> +
> +static virDomainObjPtr
> +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
> + const unsigned char *uuid,
> + bool ref)
> +{
> + virDomainObjPtr obj;
> +
> + virObjectLock(doms);
> + obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref);
> + virObjectUnlock(doms);
This won't work for ref == true as in that case the unlock is not the
last thing done in virDomainObjListFindByUUIDInternalLocked().
I still think that
this won't cause any problem as far as list lock is
aquired.
Otherwise we would have seen them in virDomainObjListNumOfDomains
I would rather we didn't invent bunch of new functions whose names
aren't readable, but mayb even preferred open-coding some of them (or
their parts) in the rename itself. The reasoning behind it is that
rename is special API and it touches code that's not exposed normally.
Or the other way would be moving parts of the rename code into this file
to make it available for other drivers as well.
I don't insist on the way
I'm proposing, let the patch be the reference
of the problem.
Adding Michal to Cc as the original idea-haver =) to see if we can
agree on that or I am mistaken and your approach is the way to go.
Martin
4:50 a.m.
On Tue, Jan 26, 2016 at 01:31:25PM +0300, Maxim Nestratov wrote:
26.01.2016 12:29, Martin Kletzander пишет:
> On Mon, Jan 25, 2016 at 12:16:12PM +0300, Maxim Nestratov wrote:
>> 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
>>
>
> Ouch, the rename API should hold the list lock all the time it's doing
> something and not acquire it when any domain is locked.
>
> But you are duplicating a lot of code,
Not that much
I meant to edit that out after reading the whole e-mail, my apologies.
>
>> The patch establishes a single order of taking locks: driver->domains
>> list
>> first, then a particular VM. This is done by implementing a set of
>> virDomainObjListXxxLocked functions working with driver->domains that
>> assume
>> that list lock is already aquired by calling functions.
>>
>> Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com>
>> ---
>> src/conf/virdomainobjlist.c | 74
>> +++++++++++++++++++++++++++++++++++++--------
>> src/conf/virdomainobjlist.h | 9 ++++++
>> src/libvirt_private.syms | 4 +++
>> src/qemu/qemu_driver.c | 40 +++++++++++++++++++++---
>> 4 files changed, 110 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c
>> index 7568f93..89e28a8 100644
>> --- a/src/conf/virdomainobjlist.c
>> +++ b/src/conf/virdomainobjlist.c
>> @@ -132,21 +132,19 @@ virDomainObjPtr
>> virDomainObjListFindByID(virDomainObjListPtr doms,
>>
>>
>> static virDomainObjPtr
>> -virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>> +virDomainObjListFindByUUIDInternalLocked(virDomainObjListPtr doms,
>> const unsigned char *uuid,
>> bool ref)
>
> Indentation is off for renamed functions.
Ok. I'll fix it if we decide to go on with my approach.
>
>> {
>> char uuidstr[VIR_UUID_STRING_BUFLEN];
>> virDomainObjPtr obj;
>>
>> - virObjectLock(doms);
>> virUUIDFormat(uuid, uuidstr);
>>
>> obj = virHashLookup(doms->objs, uuidstr);
>> - if (ref) {
>> + if (ref)
>> virObjectRef(obj);
>> - virObjectUnlock(doms);
>> - }
>> +
>> if (obj) {
>> virObjectLock(obj);
>> if (obj->removing) {
>> @@ -156,8 +154,19 @@
>> virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>> obj = NULL;
>> }
>> }
>> - if (!ref)
>> - virObjectUnlock(doms);
>> + return obj;
>> +}
>> +
>> +static virDomainObjPtr
>> +virDomainObjListFindByUUIDInternal(virDomainObjListPtr doms,
>> + const unsigned char *uuid,
>> + bool ref)
>> +{
>> + virDomainObjPtr obj;
>> +
>> + virObjectLock(doms);
>> + obj = virDomainObjListFindByUUIDInternalLocked(doms, uuid, ref);
>> + virObjectUnlock(doms);
>
> This won't work for ref == true as in that case the unlock is not the
> last thing done in virDomainObjListFindByUUIDInternalLocked().
I still think that this won't cause any problem as far as list lock is
aquired.
Otherwise we would have seen them in virDomainObjListNumOfDomains
That's kind of why there's the difference between ref=true and
ref=false. The thing is that ref=false is prone to deadlocks even
though they are not as easy to reproduce. We'll see what the decision
will be.
Martin
6:26 a.m.
On 25.01.2016 10:16, Maxim Nestratov wrote:
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
The patch establishes a single order of taking locks: driver->domains list
first, then a particular VM. This is done by implementing a set of
virDomainObjListXxxLocked functions working with driver->domains that assume
that list lock is already aquired by calling functions.
Signed-off-by: Maxim Nestratov <mnestratov(a)virtuozzo.com>
---
src/conf/virdomainobjlist.c | 74 +++++++++++++++++++++++++++++++++++++--------
src/conf/virdomainobjlist.h | 9 ++++++
src/libvirt_private.syms | 4 +++
src/qemu/qemu_driver.c | 40 +++++++++++++++++++++---
4 files changed, 110 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e46404b..b012516 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -206,6 +206,36 @@ struct qemuAutostartData {
virConnectPtr conn;
};
+/**
+ * qemuDomObjFromDomainLocked:
+ * @domain: Domain pointer that has to be looked up
+ *
+ * This function looks up @domain and returns the appropriate virDomainObjPtr
+ * that has to be released by calling virDomainObjEndAPI().
+ * It assumes that driver->domains list is locked already, thus it uses Locked
+ * variant of virDomainObjListFindByUUIDRef function.
+ *
+ * Returns the domain object with incremented reference counter which is locked
+ * on success, NULL otherwise.
+ */
+static virDomainObjPtr
+qemuDomObjFromDomainLocked(virDomainPtr domain)
+{
+ virDomainObjPtr vm;
+ virQEMUDriverPtr driver = domain->conn->privateData;
+ char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+ vm = virDomainObjListFindByUUIDRefLocked(driver->domains, domain->uuid);
+ if (!vm) {
+ virUUIDFormat(domain->uuid, uuidstr);
+ virReportError(VIR_ERR_NO_DOMAIN,
+ _("no domain with matching uuid '%s' (%s)"),
+ uuidstr, domain->name);
+ return NULL;
+ }
+
+ return vm;
+}
/**
* qemuDomObjFromDomain:
@@ -19892,7 +19922,8 @@ static int qemuDomainRename(virDomainPtr dom,
virCheckFlags(0, ret);
- if (!(vm = qemuDomObjFromDomain(dom)))
+ virObjectLock(driver->domains);
This is rather ugly. While driver->domains is a lockable object, it's
internals are intentionally hidden from the rest of the code so that
nobody from outside should touch them. That's why we have locking APIs
over virDomainObjList object. I know this will work, I just find it hackish.
However, I find your approach understandable and kind of agree with it.
What we should do is:
1) lock list
2) lookup domain
3) begin MODIFY job on the domain (*)
4) rename the domain (here are hidden all the checks, moving the XML
file, etc. - basically everything from the qemuDomainRename())
5) end job
6) unlock domain
7) unlock list
* - here we will need BeginJob() without timeout - we don't want to keep
the list locked longer than needed. Either setting the job is done
instantly or not at all.
What if, instead of introducing bunch of Locked() functions we introduce
a new internal API to virDomainObjList that will look like this:
int virDomainObjListRename(virDomainObjListPtr domains,
virDomainObjPtr dom,
const char *new_name,
int (*driverRename)(virDomainObjPtr, ...),
int (*rollBack)(virDomainObjPtr, ...));
This function will encapsulate the renaming and at the correct spot it
will call driverRename() so that driver can adjust its internal state.
If the driver is successful, just remove the old entry. If it is not,
call rollBack() callback which will cancel partially performed operation
and restore original state of the driver.
Moreover, in the virDomainObjListRename() we can ensure the locking
order and we don't need to introduce new BeginJob() API with no timeout.
Michal
7:14 a.m.
On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
On 25.01.2016 10:16, Maxim Nestratov wrote:
...
>
> virCheckFlags(0, ret);
>
> - if (!(vm = qemuDomObjFromDomain(dom)))
> + virObjectLock(driver->domains);
This is rather ugly. While driver->domains is a lockable object, it's
internals are intentionally hidden from the rest of the code so that
nobody from outside should touch them. That's why we have locking APIs
over virDomainObjList object. I know this will work, I just find it hackish.
Yeah, I don't like this either.
However, I find your approach understandable and kind of agree with
it.
What we should do is:
1) lock list
2) lookup domain
3) begin MODIFY job on the domain (*)
4) rename the domain (here are hidden all the checks, moving the XML
file, etc. - basically everything from the qemuDomainRename())
5) end job
6) unlock domain
7) unlock list
* - here we will need BeginJob() without timeout - we don't want to keep
the list locked longer than needed. Either setting the job is done
instantly or not at all.
This sounds very ugly and fragile too.
What if, instead of introducing bunch of Locked() functions we
introduce
a new internal API to virDomainObjList that will look like this:
int virDomainObjListRename(virDomainObjListPtr domains,
virDomainObjPtr dom,
const char *new_name,
int (*driverRename)(virDomainObjPtr, ...),
int (*rollBack)(virDomainObjPtr, ...));
This function will encapsulate the renaming and at the correct spot it
will call driverRename() so that driver can adjust its internal state.
If the driver is successful, just remove the old entry. If it is not,
call rollBack() callback which will cancel partially performed operation
and restore original state of the driver.
Moreover, in the virDomainObjListRename() we can ensure the locking
order and we don't need to introduce new BeginJob() API with no timeout.
But very much this approach I like :-)
Jirka
7:33 a.m.
On Tue, Jan 26, 2016 at 02:14:23PM +0100, Jiri Denemark wrote:
On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
> On 25.01.2016 10:16, Maxim Nestratov wrote:
...
> >
> > virCheckFlags(0, ret);
> >
> > - if (!(vm = qemuDomObjFromDomain(dom)))
> > + virObjectLock(driver->domains);
>
> This is rather ugly. While driver->domains is a lockable object, it's
> internals are intentionally hidden from the rest of the code so that
> nobody from outside should touch them. That's why we have locking APIs
> over virDomainObjList object. I know this will work, I just find it hackish.
Yeah, I don't like this either.
> However, I find your approach understandable and kind of agree with it.
> What we should do is:
>
> 1) lock list
> 2) lookup domain
> 3) begin MODIFY job on the domain (*)
>
> 4) rename the domain (here are hidden all the checks, moving the XML
> file, etc. - basically everything from the qemuDomainRename())
>
> 5) end job
> 6) unlock domain
> 7) unlock list
>
> * - here we will need BeginJob() without timeout - we don't want to keep
> the list locked longer than needed. Either setting the job is done
> instantly or not at all.
This sounds very ugly and fragile too.
> What if, instead of introducing bunch of Locked() functions we introduce
> a new internal API to virDomainObjList that will look like this:
>
> int virDomainObjListRename(virDomainObjListPtr domains,
> virDomainObjPtr dom,
> const char *new_name,
> int (*driverRename)(virDomainObjPtr, ...),
> int (*rollBack)(virDomainObjPtr, ...));
>
> This function will encapsulate the renaming and at the correct spot it
> will call driverRename() so that driver can adjust its internal state.
> If the driver is successful, just remove the old entry. If it is not,
> call rollBack() callback which will cancel partially performed operation
> and restore original state of the driver.
> Moreover, in the virDomainObjListRename() we can ensure the locking
> order and we don't need to introduce new BeginJob() API with no timeout.
But very much this approach I like :-)
That's what I meant with:
"Or the other way would be moving parts of the rename code into this file
to make it available for other drivers as well."
but you explained it in more detail and understandably. ACK for that
idea as well.
Jirka
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
7:52 a.m.
26.01.2016 16:33, Martin Kletzander пишет:
On Tue, Jan 26, 2016 at 02:14:23PM +0100, Jiri Denemark wrote:
> On Tue, Jan 26, 2016 at 13:26:32 +0100, Michal Privoznik wrote:
>> On 25.01.2016 10:16, Maxim Nestratov wrote:
> ...
[snip]
>> What if, instead of introducing bunch of Locked() functions
we
>> introduce
>> a new internal API to virDomainObjList that will look like this:
>>
>> int virDomainObjListRename(virDomainObjListPtr domains,
>> virDomainObjPtr dom,
>> const char *new_name,
>> int (*driverRename)(virDomainObjPtr, ...),
>> int (*rollBack)(virDomainObjPtr, ...));
>>
>> This function will encapsulate the renaming and at the correct spot it
>> will call driverRename() so that driver can adjust its internal state.
>> If the driver is successful, just remove the old entry. If it is not,
>> call rollBack() callback which will cancel partially performed
>> operation
>> and restore original state of the driver.
>> Moreover, in the virDomainObjListRename() we can ensure the locking
>> order and we don't need to introduce new BeginJob() API with no
>> timeout.
>
> But very much this approach I like :-)
>
That's what I meant with:
"Or the other way would be moving parts of the rename code into this
file
to make it available for other drivers as well."
but you explained it in more detail and understandably. ACK for that
idea as well.
I like it too. I can rework the patch following this way if no one wants
to volunteer.
> Jirka
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
3217
days inactive
3218
days old
7 comments
4 participants
participants (4)
-
Jiri Denemark -
Martin Kletzander -
Maxim Nestratov -
Michal Privoznik