[libvirt] [PATCH 3/6] use virObject to manage reference-count of virDomainObj
Eric Blake
eblake at redhat.com
Wed Apr 6 22:33:56 UTC 2011
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
considered a coding bug to ever have virObjectInit fail in the first
place, so we don't really have to worry about that.
> @@ -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?
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).
> @@ -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/
> @@ -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. :(
> @@ -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()?
> @@ -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.
> 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?
>
> 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;
> + }
> +
> + 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
you call qemuProcessStopCPUs(driver, vm), which used to assume driver
was locked - is that safe?
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().
> +++ 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.
--
Eric Blake eblake at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110406/3abf8931/attachment-0001.sig>
More information about the libvir-list
mailing list