4:41 a.m.
On Wed, Apr 06, 2011 at 04:33:56PM -0600, Eric Blake wrote:
On 04/06/2011 01:19 AM, Hu Tao wrote:
> This patch also eliminates a dead-lock bug in
> qemuDomainObjBeginJobWithDriver: if virCondWaitUntil() timeouts, the
> thread tries to acquire qemu driver lock while holding virDomainObj
> lock.
Let's please separate that bug fix into a separate patch which gets
applied as soon as possible.
> ---
> src/conf/domain_conf.c | 56 ++++----
> src/conf/domain_conf.h | 6 +-
> src/openvz/openvz_conf.c | 8 +-
> src/qemu/qemu_domain.c | 32 ++---
> src/qemu/qemu_domain.h | 2 +-
> src/qemu/qemu_driver.c | 304 ++++++++++++++++++++-------------------------
> src/qemu/qemu_migration.c | 45 +++----
> src/qemu/qemu_process.c | 33 ++---
> src/vmware/vmware_conf.c | 2 +-
> 9 files changed, 215 insertions(+), 273 deletions(-)
> int virDomainObjListInit(virDomainObjListPtr doms)
> @@ -437,7 +436,7 @@ virDomainObjPtr virDomainFindByID(const virDomainObjListPtr
doms,
> virDomainObjPtr obj;
> obj = virHashSearch(doms->objs, virDomainObjListSearchID, &id);
> if (obj)
> - virDomainObjLock(obj);
> + virDomainObjRef(obj);
Wow - changing the semantics so the object is not locked by default,
just referenced.
> @@ -1010,6 +1007,11 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps)
> return NULL;
> }
>
> + if (virObjectInit(&domain->obj, doDomainObjFree)) {
> + VIR_FREE(domain);
> + return NULL;
Hmm. virObjectInit used VIR_ERROR, which logs, but doesn't call into
virtError. By returning NULL here, a user will get the dreaded "Unknown
error" message since we didn't hook into the virterror machinery.
Should virObjectInit instead be using virReportErrorHelper? Or is it
How to use virReportErrorHelper in this case?
considered a coding bug to ever have virObjectInit fail in the first
place, so we don't really have to worry about that.
Sorry for my bad English, I don't understand this sentence.
> @@ -1075,8 +1075,10 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps,
> domain->def = def;
>
> virUUIDFormat(def->uuid, uuidstr);
> + virDomainObjRef(domain);
> if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) {
It seems like incrementing ref count on container addition and
decrementing it on removal are common actions. Would it simplify code
any by making a wrapper for virHashAddEntry:
int virHashAddObjEntry(virHashTablePtr, const void *name, virObjectPtr
*data)
which does the referencing as part of adding/removing a virObject from a
hash table, rather than making all callers track it?
Agreed. But this needs many changes I think it is better to do it in
a seperate patch.
The only potential drawback to that is that if you use the wrong
function, the referencing doesn't happen. Or maybe even make
virHashCreateFull take a bool parameter of whether the data must be a
virObjectPtr, so you don't have to wrap virHashAddObjEntry (and since
virHashRemoveEntry doesn't really have any way to create a
virHashRemoveObjEntry wrapper, but it would need to be in on the game of
automatic reference count manipulations any time we know the table
hashes only virObjects as data).
Would it be better to have two types of hashtable, one hashes only
virObjects, the other hashes data except virObjects? this can minimize
the impact brought by the change of hashtable to existing code.
> @@ -1149,9 +1151,7 @@ virDomainObjGetPersistentDef(virCapsPtr caps,
> }
>
> /*
> - * The caller must hold a lock on the driver owning 'doms',
> - * and must also have locked 'dom', to ensure no one else
> - * is either waiting for 'dom' or still usingn it
> + * The caller must hold a lock on the driver owning 'doms'.
While touching that line, fix the spacing:
s/lock on/lock on/
OK.
> @@ -1159,9 +1159,8 @@ void virDomainRemoveInactive(virDomainObjListPtr doms,
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virUUIDFormat(dom->def->uuid, uuidstr);
>
> - virDomainObjUnlock(dom);
> -
> virHashRemoveEntry(doms->objs, uuidstr);
> + virDomainObjUnref(dom);
Hmm, this means you are dereferencing dom->def->uuid while holding the
driver lock but without holding the domain lock. If there is another
place in code that holds the domain lock but not the driver lock,
couldn't that cause a bad read of dom->def? I don't think you can
blindly get rid of holding the lock on dom unless we make additional
rules about which members of dom are safe to modify (for example,
stating that dom->def cannot be modified unless you hold the driver
lock), but that's hard to audit. :(
Don't understand you very well here. Let me explain what I understand:
If a dom is added into a hashtable, then it is not safe to modify its
uuid since it is the key of dom in hashtable. And we have to make some
rules about when the uuid can be modified(say, hold the driver lock
first when dom is in hashtable).
If I understand correctly, then I think the simple way to modify the
uuid is to remove the dom from hashtable, modify uuid, then reinsert
it into hashtable.
The qemu driver lock here is to protect the hashtable objs(remove entry),
not for unref dom. However, I think this code should be better like
this:
qemu driver lock
dom = virHashRemoveEntry(); /* not be exactly but a way to get the
removed dom */
qemu driver unlock
unref dom
> @@ -6146,7 +6145,7 @@ static virDomainObjPtr virDomainObjParseXML(virCapsPtr caps,
>
> error:
> /* obj was never shared, so unref should return 0 */
> - ignore_value(virDomainObjUnref(obj));
> + virDomainObjUnref(obj);
Do we also want to delete the comment, since it only served to justify
why we were using ignore_value()?
OK.
> @@ -8449,10 +8448,12 @@ static virDomainObjPtr virDomainLoadConfig(virCapsPtr caps,
> if (!(dom = virDomainAssignDef(caps, doms, def, false)))
> goto error;
>
> + virDomainObjLock(dom);
> dom->autostart = autostart;
>
> if (notify)
> (*notify)(dom, newVM, opaque);
> + virDomainObjUnlock(dom);
>
> VIR_FREE(configFile);
> VIR_FREE(autostartLink);
Ouch, this changes virDomainLoadConfig so that it returns an unlocked
domain, but virDomainLoadAllConfigs assumed that domain was still locked
and you now have a double-unlock.
Will check this carefully.
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index c2a1f9a..3a3c953 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -457,13 +457,13 @@ int qemuDomainObjBeginJob(virDomainObjPtr obj)
> }
> then = timeval_to_ms(now) + QEMU_JOB_WAIT_TIME;
>
> - virDomainObjRef(obj);
> + virDomainObjLock(obj);
This grabbed obj->privateData while obj was not locked. Is that safe,
or do you need to float the initialization of priv down?
Yes, you are right.
>
> while (priv->jobActive) {
> if (virCondWaitUntil(&priv->jobCond, &obj->lock, then) <
0) {
> - /* Safe to ignore value since ref count was incremented above */
> - ignore_value(virDomainObjUnref(obj));
> - if (errno == ETIMEDOUT)
> + int err = errno;
> + virDomainObjUnlock(obj);
> + if (err == ETIMEDOUT)
Good catch that pre-patch, errno was untouched by virDomainObjUnref, but
post-patch, you need to preserve errno before unlocking.
> qemuReportError(VIR_ERR_OPERATION_TIMEOUT,
> "%s", _("cannot acquire state change
lock"));
> else
> @@ -482,12 +482,10 @@ int qemuDomainObjBeginJob(virDomainObjPtr obj)
> }
>
> /*
> - * obj must be locked before calling, qemud_driver must be locked
> - *
> * This must be called by anything that will change the VM state
> * in any way, or anything that will use the QEMU monitor.
> */
> -int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver,
> +int qemuDomainObjBeginJobWithDriver(struct qemud_driver *driver ATTRIBUTE_UNUSED,
If we no longer care if driver was locked or unlocked, can we do a
followup patch that simplifies all callers to just use
qemuDomainObjBeginJob and delete this variant? But I don't think you
can get away with it; see below...
> virDomainObjPtr obj)
> {
> qemuDomainObjPrivatePtr priv = obj->privateData;
> @@ -501,20 +499,18 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver
*driver,
> }
> then = timeval_to_ms(now) + QEMU_JOB_WAIT_TIME;
>
> - virDomainObjRef(obj);
> - qemuDriverUnlock(driver);
...don't think you can delete this line...
> + virDomainObjLock(obj);
Again, locking after you already grabbed priv.
>
> while (priv->jobActive) {
> if (virCondWaitUntil(&priv->jobCond, &obj->lock, then) <
0) {
Ouch. You just broke the no sleeping while holding locks rule -
previously, virCondWaitUntil was called with no locks, now you are
calling it while holding both driver and domain lock. :(
> - /* Safe to ignore value since ref count was incremented above */
> - ignore_value(virDomainObjUnref(obj));
> - if (errno == ETIMEDOUT)
> + int err = errno;
> + virDomainObjUnlock(obj);
> + if (err == ETIMEDOUT)
> qemuReportError(VIR_ERR_OPERATION_TIMEOUT,
> "%s", _("cannot acquire state change
lock"));
> else
> virReportSystemError(errno,
> "%s", _("cannot acquire job
mutex"));
> - qemuDriverLock(driver);
Yep, this is the deadlock fix; pre-patch was definitely trying to regrab
the driver lock while still holding the domain lock. But given that you
_do_ have to regain driver lock (thanks to the blocking of
virCondWaitUntil), I'm afraid your fix will instead have to look like...
> return -1;
> }
> }
> @@ -524,10 +520,6 @@ int qemuDomainObjBeginJobWithDriver(struct qemud_driver
*driver,
> priv->jobStart = timeval_to_ms(now);
> memset(&priv->jobInfo, 0, sizeof(priv->jobInfo));
>
> - virDomainObjUnlock(obj);
> - qemuDriverLock(driver);
> - virDomainObjLock(obj);
...these lines, which also must remain (although possibly modified a
bit, if you are changing semantics to guarantee that you don't have to
have obj locked prior to calling this function).
> +++ b/src/qemu/qemu_driver.c
> @@ -139,7 +139,6 @@ qemuAutostartDomain(void *payload, const void *name
ATTRIBUTE_UNUSED, void *opaq
> struct qemuAutostartData *data = opaque;
> virErrorPtr err;
>
> - virDomainObjLock(vm);
> virResetLastError();
> if (qemuDomainObjBeginJobWithDriver(data->driver, vm) < 0) {
> err = virGetLastError();
> @@ -156,12 +155,8 @@ qemuAutostartDomain(void *payload, const void *name
ATTRIBUTE_UNUSED, void *opaq
> err ? err->message : _("unknown error"));
> }
>
> - if (qemuDomainObjEndJob(vm) == 0)
> - vm = NULL;
> + qemuDomainObjEndJob(vm);
> }
> -
> - if (vm)
> - virDomainObjUnlock(vm);
> }
Ah, so it qemuDomainObjBeginJobWithDriver is called while domain lock
not held on entry but exits with it held on success, and
qemuDomainObjEndJob is the counterpart that releases the domain lock.
Makes for some more compact code everywhere else. :)
> @@ -1239,39 +1243,57 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn,
const char *xml,
>
> qemuDriverLock(driver);
> if (!(def = virDomainDefParseString(driver->caps, xml,
> - VIR_DOMAIN_XML_INACTIVE)))
> + VIR_DOMAIN_XML_INACTIVE))) {
> + qemuDriverUnlock(driver);
> goto cleanup;
> + }
>
> - if (virSecurityManagerVerify(driver->securityManager, def) < 0)
> + if (virSecurityManagerVerify(driver->securityManager, def) < 0) {
> + qemuDriverUnlock(driver);
> goto cleanup;
> + }
>
> - if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0)
> + if (virDomainObjIsDuplicate(&driver->domains, def, 1) < 0) {
> + qemuDriverUnlock(driver);
> goto cleanup;
> + }
Why must every caller unlock the driver, instead of factoring it once
into the cleanup? I'd almost rather see a second label, as in:
goto cleanup_locked;
cleanup_locked:
qemuDriverUnlock(driver);
goto cleanup;
OK, makes code cleaner.
> + }
> +
> + qemuDriverUnlock(driver);
>
> def = NULL;
>
> - if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> + if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) {
With old semantics, you can't call qemuDomainObjBeginJobWithDriver if
driver is unlocked (then again, you just changed the body of that method
to not care if driver was locked, which matches with you just unlocking
it a couple lines ago). Then again, maybe we don't need
BeginJobWithDriver after all, if all of your conversions have changed
things to no longer hold the driver lock before beginning a job.
> @@ -1307,13 +1325,14 @@ static int qemudDomainSuspend(virDomainPtr dom) {
>
> qemuDriverLock(driver);
> vm = virDomainFindByUUID(&driver->domains, dom->uuid);
> + qemuDriverUnlock(driver);
>
> if (!vm) {
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> virUUIDFormat(dom->uuid, uuidstr);
> qemuReportError(VIR_ERR_NO_DOMAIN,
> _("no domain with matching uuid '%s'"),
uuidstr);
> - goto cleanup;
> + return -1;
> }
> if (!virDomainObjIsActive(vm)) {
> qemuReportError(VIR_ERR_OPERATION_INVALID,
> @@ -1354,16 +1373,12 @@ static int qemudDomainSuspend(virDomainPtr dom) {
> }
>
> endjob:
> - if (qemuDomainObjEndJob(vm) == 0)
> - vm = NULL;
> + qemuDomainObjEndJob(vm);
Ouch - this change means you are accessing
virDomainSaveStatus(driver->caps, driver->stateDir, vm) without holding
the driver lock. Is that safe? (I don't know that caps or stateDir
ever change after creation, so maybe it is safe after all). Likewise
Yes, not safe here. Since the semantics of
qemuDomainObjBeginJobWithDriver is changed, and many places call it, I
have to check every place and test the change.
In next version I plan to split the series into patches like this:
first a patch changes virDomainObj to inherit from virObject, second a
patch changes qemuDomainObjBeginJobWithDriver, then each patch for a
place that calls qemuDomainObjBeginJobWithDriver. Is this reasonable? Or
a better way?
you call qemuProcessStopCPUs(driver, vm), which used to assume
driver
was locked - is that safe?
As you have already noticed, the semantics of
qemuDomainObjBeginJobWithDriver and qemuEnterMonitorWithDriver(patch 4)
are changed, qemu driver lock have not to be hold before calling
them. So if this patch is applied, patch 4 should also be applied.
I think a lot of this file will have the same questions, so I won't
review the rest of qemu_domain.c very closely on this round.
> @@ -1090,8 +1087,8 @@ int qemuMigrationPerform(struct qemud_driver *driver,
> VIR_DOMAIN_EVENT_STOPPED_MIGRATED);
> if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) {
> virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm);
> - if (qemuDomainObjEndJob(vm) > 0)
> - virDomainRemoveInactive(&driver->domains, vm);
> + qemuDomainObjEndJob(vm);
> + virDomainRemoveInactive(&driver->domains, vm);
> vm = NULL;
Wonky spacing.
> @@ -644,7 +644,7 @@ qemuConnectMonitor(struct qemud_driver *driver, virDomainObjPtr
vm)
>
> /* Safe to ignore value since ref count was incremented above */
> if (priv->mon == NULL)
> - ignore_value(virDomainObjUnref(vm));
> + virDomainObjUnref(vm);
Another case where the comment is out of date once you delete the
ignore_value().
Will remove it.
> +++ b/src/vmware/vmware_conf.c
> @@ -205,7 +205,7 @@ cleanup:
> VIR_FREE(vmx);
> /* any non-NULL vm here has not been shared, so unref will return 0 */
> if (vm)
> - ignore_value(virDomainObjUnref(vm));
> + virDomainObjUnref(vm);
And another stale comment.
Will remove it.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org