[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/6] use virObject to manage reference-count of virDomainObj



On Wed, Apr 06, 2011 at 03:19:51PM +0800, 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.
> ---
>  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(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 90a1317..fc76a00 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -47,6 +47,7 @@
>  #include "storage_file.h"
>  #include "files.h"
>  #include "bitmap.h"
> +#include "virobject.h"
>  
>  #define VIR_FROM_THIS VIR_FROM_DOMAIN
>  
> @@ -395,9 +396,7 @@ static void
>  virDomainObjListDataFree(void *payload, const void *name ATTRIBUTE_UNUSED)
>  {
>      virDomainObjPtr obj = payload;
> -    virDomainObjLock(obj);
> -    if (virDomainObjUnref(obj) > 0)
> -        virDomainObjUnlock(obj);
> +    virDomainObjUnref(obj);
>  }
>  
>  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);
>      return obj;
>  }
>  
> @@ -452,7 +451,7 @@ virDomainObjPtr virDomainFindByUUID(const virDomainObjListPtr doms,
>  
>      obj = virHashLookup(doms->objs, uuidstr);
>      if (obj)
> -        virDomainObjLock(obj);
> +        virDomainObjRef(obj);
>      return obj;
>  }
>  
> @@ -476,7 +475,7 @@ virDomainObjPtr virDomainFindByName(const virDomainObjListPtr doms,
>      virDomainObjPtr obj;
>      obj = virHashSearch(doms->objs, virDomainObjListSearchName, name);
>      if (obj)
> -        virDomainObjLock(obj);
> +        virDomainObjRef(obj);
>      return obj;
>  }

This is a major change in semantics, which makes pretty much
every single caller non-thread safe, unless the callers are
all changed todo virDomainObjLock immediately after calling
this. So I don't really see the point in this - it just means
more code duplication.

>  
> @@ -967,6 +966,12 @@ static void virDomainObjFree(virDomainObjPtr dom)
>  {
>      if (!dom)
>          return;
> +    virDomainObjUnref(dom);
> +}
> +
> +static void doDomainObjFree(virObjectPtr obj)
> +{
> +    virDomainObjPtr dom = (virDomainObjPtr)obj;
>  
>      VIR_DEBUG("obj=%p", dom);
>      virDomainDefFree(dom->def);
> @@ -984,21 +989,13 @@ static void virDomainObjFree(virDomainObjPtr dom)
>  
>  void virDomainObjRef(virDomainObjPtr dom)
>  {
> -    dom->refs++;
> -    VIR_DEBUG("obj=%p refs=%d", dom, dom->refs);
> +    virObjectRef(&dom->obj);
>  }
>  
>  
> -int virDomainObjUnref(virDomainObjPtr dom)
> +void virDomainObjUnref(virDomainObjPtr dom)
>  {
> -    dom->refs--;
> -    VIR_DEBUG("obj=%p refs=%d", dom, dom->refs);
> -    if (dom->refs == 0) {
> -        virDomainObjUnlock(dom);
> -        virDomainObjFree(dom);
> -        return 0;
> -    }
> -    return dom->refs;
> +    virObjectUnref(&dom->obj);
>  }
>  
>  static virDomainObjPtr virDomainObjNew(virCapsPtr caps)
> @@ -1010,6 +1007,11 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps)
>          return NULL;
>      }
>  
> +    if (virObjectInit(&domain->obj, doDomainObjFree)) {
> +        VIR_FREE(domain);
> +        return NULL;
> +    }
> +
>      if (caps->privateDataAllocFunc &&
>          !(domain->privateData = (caps->privateDataAllocFunc)())) {
>          virReportOOMError();
> @@ -1027,9 +1029,7 @@ static virDomainObjPtr virDomainObjNew(virCapsPtr caps)
>          return NULL;
>      }
>  
> -    virDomainObjLock(domain);
>      domain->state = VIR_DOMAIN_SHUTOFF;
> -    domain->refs = 1;
>  
>      virDomainSnapshotObjListInit(&domain->snapshots);
>  
> @@ -1075,8 +1075,10 @@ virDomainObjPtr virDomainAssignDef(virCapsPtr caps,
>      domain->def = def;
>  
>      virUUIDFormat(def->uuid, uuidstr);
> +    virDomainObjRef(domain);
>      if (virHashAddEntry(doms->objs, uuidstr, domain) < 0) {
> -        VIR_FREE(domain);
> +        virDomainObjUnref(domain);
> +        virDomainObjFree(domain);
>          return NULL;
>      }

Simiarly here, you're now requiring all callers to manually obtain
a lock.

> @@ -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;
> +    }
>  
> -    if (qemudCanonicalizeMachine(driver, def) < 0)
> +    if (qemudCanonicalizeMachine(driver, def) < 0) {
> +        qemuDriverUnlock(driver);
>          goto cleanup;
> +    }
>  
> -    if (qemuDomainAssignPCIAddresses(def) < 0)
> +    if (qemuDomainAssignPCIAddresses(def) < 0) {
> +        qemuDriverUnlock(driver);
>          goto cleanup;
> +    }
>  
>      if (!(vm = virDomainAssignDef(driver->caps,
>                                    &driver->domains,
> -                                  def, false)))
> +                                  def, false))) {
> +        qemuDriverUnlock(driver);
>          goto cleanup;
> +    }
> +
> +    qemuDriverUnlock(driver);

driver is now unlocked....

>  
>      def = NULL;
>  
> -    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> +    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0) {
> +        virDomainObjUnref(vm);

...but this method *requires* driver to be locked.

>          goto cleanup; /* XXXX free the 'vm' we created ? */
> +    }
>  
>      if (qemuProcessStart(conn, driver, vm, NULL,
>                           (flags & VIR_DOMAIN_START_PAUSED) != 0,
>                           -1, NULL, VIR_VM_OP_CREATE) < 0) {
>          qemuAuditDomainStart(vm, "booted", false);

...and this method writes to 'driver', so it is now unsafe.

> -        if (qemuDomainObjEndJob(vm) > 0)
> -            virDomainRemoveInactive(&driver->domains,
> -                                    vm);
> -        vm = NULL;
> +        qemuDomainObjEndJob(vm);
> +        qemuDriverLock(driver);
> +        virDomainRemoveInactive(&driver->domains,
> +                                vm);
> +        qemuDriverUnlock(driver);
> +        virDomainObjUnref(vm);
>          goto cleanup;
>      }
>  
> @@ -1283,17 +1305,13 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml,
>      dom = virGetDomain(conn, vm->def->name, vm->def->uuid);
>      if (dom) dom->id = vm->def->id;
>  
> -    if (vm &&
> -        qemuDomainObjEndJob(vm) == 0)
> -        vm = NULL;
> +    qemuDomainObjEndJob(vm);
> +    virDomainObjUnref(vm);
>  
>  cleanup:
>      virDomainDefFree(def);
> -    if (vm)
> -        virDomainObjUnlock(vm);
>      if (event)
>          qemuDomainEventQueue(driver, event);
> -    qemuDriverUnlock(driver);
>      return dom;
>  }


And all the usage of 'vm' in this method is now completely
unlocked and unsafe.


>  
> @@ -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);
>  
>  cleanup:
> -    if (vm)
> -        virDomainObjUnlock(vm);
> -
> +    virDomainObjUnref(vm);
>      if (event)
>          qemuDomainEventQueue(driver, event);
> -    qemuDriverUnlock(driver);
>      return ret;
>  }

Also now completely unsafe since 'vm' is never locked while
making changes to it.

[cut rest of patch]

I don't see how any of this patch is threadsafe now that virtually
no methods are acquiring the 'vm' lock.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]