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

Re: [libvirt] [PATCH 1/2] qemu: Turn closeCallbacks into virObjectLockable



On Mon, Feb 18, 2013 at 05:07:44PM +0100, Jiri Denemark wrote:
> To avoid having to hold the qemu driver lock while iterating through
> close callbacks and calling them. This fixes a real deadlock when a
> domain which is being migrated from another host gets autodestoyed as a
> result of broken connection to the other host.

Since you're turning this into a full object, I think it would be
nice to move it to a standalone file, so it can be reused by the
LXC driver, which currently re-invents this same kind of code.

> ---
>  src/qemu/qemu_conf.c      | 155 ++++++++++++++++++++++++++++++----------------
>  src/qemu/qemu_conf.h      |  39 ++++++------
>  src/qemu/qemu_driver.c    |  12 ++--
>  src/qemu/qemu_migration.c |   6 +-
>  src/qemu/qemu_process.c   |  10 +--
>  5 files changed, 136 insertions(+), 86 deletions(-)
> 
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index 4ff912d..edadf36 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -57,8 +57,25 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_QEMU
>  
> +typedef struct _qemuDriverCloseDef qemuDriverCloseDef;
> +typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
> +struct _qemuDriverCloseDef {
> +    virConnectPtr conn;
> +    virQEMUCloseCallback cb;
> +};
> +
> +struct _virQEMUCloseCallbacks {
> +    virObjectLockable parent;
> +
> +    /* UUID string to qemuDriverCloseDef mapping */
> +    virHashTablePtr list;
> +};
> +
> +
>  static virClassPtr virQEMUDriverConfigClass;
> +static virClassPtr virQEMUCloseCallbacksClass;
>  static void virQEMUDriverConfigDispose(void *obj);
> +static void virQEMUCloseCallbacksDispose(void *obj);
>  
>  static int virQEMUConfigOnceInit(void)
>  {
> @@ -71,13 +88,21 @@ static int virQEMUConfigOnceInit(void)
>      return 0;
>  }
>  
> -VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
> +static int
> +virQEMUCloseCallbacksOnceInit(void)
> +{
> +    virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(),
> +                                             "virQEMUCloseCallbacks",
> +                                             sizeof(virQEMUCloseCallbacks),
> +                                             virQEMUCloseCallbacksDispose);
> +    if (!virQEMUCloseCallbacksClass)
> +        return -1;
> +    return 0;
> +}
>  
> +VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
> +VIR_ONCE_GLOBAL_INIT(virQEMUCloseCallbacks)

No need for two initializers, just make the virQEMUConfigOnceInit
method create both classes.

> @@ -639,44 +664,59 @@ virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver,
>  
>  
>  static void
> -qemuDriverCloseCallbackFree(void *payload,
> -                            const void *name ATTRIBUTE_UNUSED)
> +virQEMUCloseCallbacksFreeData(void *payload,
> +                              const void *name ATTRIBUTE_UNUSED)
>  {
>      VIR_FREE(payload);
>  }
>  
> -int
> -qemuDriverCloseCallbackInit(virQEMUDriverPtr driver)
> +virQEMUCloseCallbacksPtr
> +virQEMUCloseCallbacksNew(void)
>  {
> -    driver->closeCallbacks = virHashCreate(5, qemuDriverCloseCallbackFree);
> -    if (!driver->closeCallbacks)
> -        return -1;
> +    virQEMUCloseCallbacksPtr closeCallbacks;
>  
> -    return 0;
> +    if (virQEMUCloseCallbacksInitialize() < 0)
> +        return NULL;
> +
> +    if (!(closeCallbacks = virObjectLockableNew(virQEMUCloseCallbacksClass)))
> +        return NULL;
> +
> +    closeCallbacks->list = virHashCreate(5, virQEMUCloseCallbacksFreeData);
> +    if (!closeCallbacks->list) {
> +        virObjectUnref(closeCallbacks);
> +        return NULL;
> +    }
> +
> +    return closeCallbacks;
>  }
>  
> -void
> -qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver)
> +static void
> +virQEMUCloseCallbacksDispose(void *obj)
>  {
> -    virHashFree(driver->closeCallbacks);
> +    virQEMUCloseCallbacksPtr closeCallbacks = obj;
> +
> +    virHashFree(closeCallbacks->list);
>  }
>  
>  int
> -qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
> -                           virDomainObjPtr vm,
> -                           virConnectPtr conn,
> -                           qemuDriverCloseCallback cb)
> +virQEMUCloseCallbacksSet(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         virConnectPtr conn,
> +                         virQEMUCloseCallback cb)
>  {
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      qemuDriverCloseDefPtr closeDef;
> +    virHashTablePtr list;
>      int ret = -1;
>  
> -    qemuDriverLock(driver);
>      virUUIDFormat(vm->def->uuid, uuidstr);
>      VIR_DEBUG("vm=%s, uuid=%s, conn=%p, cb=%p",
>                vm->def->name, uuidstr, conn, cb);
>  
> -    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
> +    virObjectLock(driver->closeCallbacks);
> +
> +    list = driver->closeCallbacks->list;
> +    closeDef = virHashLookup(list, uuidstr);
>      if (closeDef) {
>          if (closeDef->conn != conn) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -701,7 +741,7 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
>  
>          closeDef->conn = conn;
>          closeDef->cb = cb;
> -        if (virHashAddEntry(driver->closeCallbacks, uuidstr, closeDef) < 0) {
> +        if (virHashAddEntry(list, uuidstr, closeDef) < 0) {
>              VIR_FREE(closeDef);
>              goto cleanup;
>          }
> @@ -709,25 +749,28 @@ qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
>  
>      ret = 0;
>  cleanup:
> -    qemuDriverUnlock(driver);
> +    virObjectUnlock(driver->closeCallbacks);
>      return ret;
>  }
>  
>  int
> -qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
> -                             virDomainObjPtr vm,
> -                             qemuDriverCloseCallback cb)
> +virQEMUCloseCallbacksUnset(virQEMUDriverPtr driver,
> +                           virDomainObjPtr vm,
> +                           virQEMUCloseCallback cb)
>  {
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      qemuDriverCloseDefPtr closeDef;
> +    virHashTablePtr list;
>      int ret = -1;
>  
> -    qemuDriverLock(driver);
>      virUUIDFormat(vm->def->uuid, uuidstr);
>      VIR_DEBUG("vm=%s, uuid=%s, cb=%p",
>                vm->def->name, uuidstr, cb);
>  
> -    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
> +    virObjectLock(driver->closeCallbacks);
> +
> +    list = driver->closeCallbacks->list;
> +    closeDef = virHashLookup(list, uuidstr);
>      if (!closeDef)
>          goto cleanup;
>  
> @@ -738,46 +781,49 @@ qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
>          goto cleanup;
>      }
>  
> -    ret = virHashRemoveEntry(driver->closeCallbacks, uuidstr);
> +    ret = virHashRemoveEntry(list, uuidstr);
>  cleanup:
> -    qemuDriverUnlock(driver);
> +    virObjectUnlock(driver->closeCallbacks);
>      return ret;
>  }
>  
> -qemuDriverCloseCallback
> -qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
> -                           virDomainObjPtr vm,
> -                           virConnectPtr conn)
> +virQEMUCloseCallback
> +virQEMUCloseCallbacksGet(virQEMUDriverPtr driver,
> +                         virDomainObjPtr vm,
> +                         virConnectPtr conn)
>  {
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
>      qemuDriverCloseDefPtr closeDef;
> -    qemuDriverCloseCallback cb = NULL;
> +    virQEMUCloseCallback cb = NULL;
>  
> -    qemuDriverLock(driver);
>      virUUIDFormat(vm->def->uuid, uuidstr);
>      VIR_DEBUG("vm=%s, uuid=%s, conn=%p",
>                vm->def->name, uuidstr, conn);
>  
> -    closeDef = virHashLookup(driver->closeCallbacks, uuidstr);
> +    virObjectLock(driver->closeCallbacks);
> +
> +    closeDef = virHashLookup(driver->closeCallbacks->list, uuidstr);
>      if (closeDef && (!conn || closeDef->conn == conn))
>          cb = closeDef->cb;
>  
> +    virObjectUnlock(driver->closeCallbacks);
> +
>      VIR_DEBUG("cb=%p", cb);
> -    qemuDriverUnlock(driver);
>      return cb;
>  }
>  
> -struct qemuDriverCloseCallbackData {
> +struct virQEMUCloseCallbacksData {
> +    virHashTablePtr list;
>      virQEMUDriverPtr driver;
>      virConnectPtr conn;
>  };
>  
>  static void
> -qemuDriverCloseCallbackRun(void *payload,
> -                           const void *name,
> -                           void *opaque)
> +virQEMUCloseCallbacksRunOne(void *payload,
> +                            const void *name,
> +                            void *opaque)
>  {
> -    struct qemuDriverCloseCallbackData *data = opaque;
> +    struct virQEMUCloseCallbacksData *data = opaque;
>      qemuDriverCloseDefPtr closeDef = payload;
>      unsigned char uuid[VIR_UUID_BUFLEN];
>      char uuidstr[VIR_UUID_STRING_BUFLEN];
> @@ -808,20 +854,25 @@ qemuDriverCloseCallbackRun(void *payload,
>      if (dom)
>          virObjectUnlock(dom);
>  
> -    virHashRemoveEntry(data->driver->closeCallbacks, uuidstr);
> +    virHashRemoveEntry(data->list, uuidstr);
>  }
>  
>  void
> -qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
> -                              virConnectPtr conn)
> +virQEMUCloseCallbacksRun(virQEMUDriverPtr driver,
> +                         virConnectPtr conn)
>  {
> -    struct qemuDriverCloseCallbackData data = {
> -        driver, conn
> +    struct virQEMUCloseCallbacksData data = {
> +        .driver = driver,
> +        .conn = conn
>      };
> +
>      VIR_DEBUG("conn=%p", conn);
> -    qemuDriverLock(driver);
> -    virHashForEach(driver->closeCallbacks, qemuDriverCloseCallbackRun, &data);
> -    qemuDriverUnlock(driver);
> +    virObjectLock(driver->closeCallbacks);
> +
> +    data.list = driver->closeCallbacks->list;
> +    virHashForEach(data.list, virQEMUCloseCallbacksRunOne, &data);
> +
> +    virObjectUnlock(driver->closeCallbacks);
>  }
>  
>  /* Construct the hash key for sharedDisks as "major:minor" */
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index 14f1427..72380dc 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -47,8 +47,8 @@
>  
>  # define QEMUD_CPUMASK_LEN CPU_SETSIZE
>  
> -typedef struct _qemuDriverCloseDef qemuDriverCloseDef;
> -typedef qemuDriverCloseDef *qemuDriverCloseDefPtr;
> +typedef struct _virQEMUCloseCallbacks virQEMUCloseCallbacks;
> +typedef virQEMUCloseCallbacks *virQEMUCloseCallbacksPtr;
>  
>  typedef struct _virQEMUDriver virQEMUDriver;
>  typedef virQEMUDriver *virQEMUDriverPtr;
> @@ -216,8 +216,8 @@ struct _virQEMUDriver {
>      /* Immutable pointer. lockless access */
>      virLockManagerPluginPtr lockManager;
>  
> -    /* Immutable pointer. Unsafe APIs. XXX */
> -    virHashTablePtr closeCallbacks;
> +    /* Immutable pointer, self-clocking APIs */
> +    virQEMUCloseCallbacksPtr closeCallbacks;
>  };
>  
>  typedef struct _qemuDomainCmdlineDef qemuDomainCmdlineDef;
> @@ -254,23 +254,22 @@ struct qemuDomainDiskInfo {
>      int io_status;
>  };
>  
> -typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver,
> -                                                   virDomainObjPtr vm,
> -                                                   virConnectPtr conn);
> -int qemuDriverCloseCallbackInit(virQEMUDriverPtr driver);
> -void qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver);
> -int qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
> +typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver,
> +                                                virDomainObjPtr vm,
> +                                                virConnectPtr conn);
> +virQEMUCloseCallbacksPtr virQEMUCloseCallbacksNew(void);
> +int virQEMUCloseCallbacksSet(virQEMUDriverPtr driver,

I was rather expecting to see the virQEMUDriverPtr replacd
with a virQEMUCloseCallbacksPtr in all the methods now it
becomes a full object.

> +                             virDomainObjPtr vm,
> +                             virConnectPtr conn,
> +                             virQEMUCloseCallback cb);
> +int virQEMUCloseCallbacksUnset(virQEMUDriverPtr driver,
>                                 virDomainObjPtr vm,
> -                               virConnectPtr conn,
> -                               qemuDriverCloseCallback cb);
> -int qemuDriverCloseCallbackUnset(virQEMUDriverPtr driver,
> -                                 virDomainObjPtr vm,
> -                                 qemuDriverCloseCallback cb);
> -qemuDriverCloseCallback qemuDriverCloseCallbackGet(virQEMUDriverPtr driver,
> -                                                   virDomainObjPtr vm,
> -                                                   virConnectPtr conn);
> -void qemuDriverCloseCallbackRunAll(virQEMUDriverPtr driver,
> -                                   virConnectPtr conn);
> +                               virQEMUCloseCallback cb);
> +virQEMUCloseCallback virQEMUCloseCallbacksGet(virQEMUDriverPtr driver,
> +                                              virDomainObjPtr vm,
> +                                              virConnectPtr conn);
> +void virQEMUCloseCallbacksRun(virQEMUDriverPtr driver,
> +                              virConnectPtr conn);
>  
>  int qemuAddSharedDisk(virQEMUDriverPtr driver,
>                        const char *disk_path)

ACK to the general approach, but I think there's a few tweaks needed
still.

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]