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@xxxxxxxxxx +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list