Re: [PATCH v2 2/2] qemu: completely rework reference counting

Michal Privoznik <mprivozn@xxxxxxxxxx> · Tue, 16 Dec 2014 14:00:09 +0100

On 16.12.2014 11:15, Martin Kletzander wrote:
There is one problem that causes various errors in the daemon.  When
domain is waiting for a job, it is unlocked while waiting on the
condition.  However, if that domain is for example transient and being
removed in another API (e.g. cancelling incoming migration), it get's
unref'd.  If the first call, that was waiting, fails to get the job, it
unref's the domain object, and because it was the last reference, it
causes clearing of the whole domain object.  However, when finishing the
call, the domain must be unlocked, but there is no way for the API to
know whether it was cleaned or not (unless there is some ugly temporary
variable, but let's scratch that).

The root cause is that our APIs don't ref the objects they are using and
all use the implicit reference that the object has when it is in the
domain list.  That reference can be removed when the API is waiting for
a job.  And because each domain doesn't do its ref'ing, it results in
the ugly checking of the return value of virObjectUnref() that we have
everywhere.

This patch changes qemuDomObjFromDomain() to ref the domain (using
virDomainObjListFindByUUIDRef()) and adds qemuDomObjEndAPI() which
should be the only function in which the return value of
virObjectUnref() is checked.  This makes all reference counting
deterministic and makes the code a bit clearer.

Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
v2:
  - Comments from Peter and Daniel on v1 implemented, rather not
    listing them here as the list was pretty comprehensive and it would
    make the reviewer focus on that.

  src/qemu/THREADS.txt      |  40 ++-
  src/qemu/qemu_domain.c    |  49 ++--
  src/qemu/qemu_domain.h    |  12 +-
  src/qemu/qemu_driver.c    | 708 ++++++++++++++++------------------------------
  src/qemu/qemu_migration.c | 111 +++-----
  src/qemu/qemu_migration.h |  10 +-
  src/qemu/qemu_process.c   |  77 ++---
  7 files changed, 371 insertions(+), 636 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f652237..30ea9df 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c

@@ -4409,8 +4333,7 @@ static void qemuProcessEventHandler(void *data, void *opaque)
          break;
      }

-    if (virObjectUnref(vm))
-        virObjectUnlock(vm);
+    qemuDomObjEndAPI(&vm);

Interesting, why the heck have we Unref() here? I mean, in the calltrace 
I don't see where the @vm is Ref()-ed. And this should not use the 
reference obtained via qemuMonitorOpen() as we are connecting only once 
and receiving events multiple times. But this is pre-existing code.

      VIR_FREE(processEvent);
  }


diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index a19e71a..4d9ce09 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c

@@ -5491,6 +5470,7 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
                                virConnectPtr conn)
  {
      VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
+    virObjectRef(vm);
      return virCloseCallbacksSet(driver->closeCallbacks, vm, conn,
                                  qemuProcessAutoDestroy);

This function is called conditionally in qemuProcessStart() depending on 
VIR_QEMU_PROCESS_START_AUTODESTROY flag passed...

  }
@@ -5498,9 +5478,12 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
  int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver,
                                   virDomainObjPtr vm)
  {
+    int ret;
      VIR_DEBUG("vm=%s", vm->def->name);
-    return virCloseCallbacksUnset(driver->closeCallbacks, vm,
-                                  qemuProcessAutoDestroy);
+    ret = virCloseCallbacksUnset(driver->closeCallbacks, vm,
+                                 qemuProcessAutoDestroy);
+    virObjectUnref(vm);
+    return ret;

.. on the other hand, this function is called unconditionally in 
qemuProcessStop(). So after 'virsh start' and 'virsh destroy' the 
reference counts don't match. I've been able to debug this and I came up 
with this diff:

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4d9ce09..cbd0389 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5470,7 +5470,6 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
                               virConnectPtr conn)
 {
     VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
-    virObjectRef(vm);
     return virCloseCallbacksSet(driver->closeCallbacks, vm, conn,
                                 qemuProcessAutoDestroy);
 }
@@ -5482,7 +5481,6 @@ int qemuProcessAutoDestroyRemove(virQEMUDriverPtr 
driver,
     VIR_DEBUG("vm=%s", vm->def->name);
     ret = virCloseCallbacksUnset(driver->closeCallbacks, vm,
                                  qemuProcessAutoDestroy);
-    virObjectUnref(vm);
     return ret;
 }

diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
index 4f26172..4128057 100644
--- a/src/util/virclosecallbacks.c
+++ b/src/util/virclosecallbacks.c
@@ -138,6 +138,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr 
closeCallbacks,
             VIR_FREE(closeDef);
             goto cleanup;
         }
+        virObjectRef(vm);
     }

     ret = 0;
@@ -172,7 +173,11 @@ virCloseCallbacksUnset(virCloseCallbacksPtr 
closeCallbacks,
         goto cleanup;
     }

-    ret = virHashRemoveEntry(closeCallbacks->list, uuidstr);
+    if (virHashRemoveEntry(closeCallbacks->list, uuidstr) < 0)
+        goto cleanup;
+
+    virObjectUnref(vm);
+    ret = 0;
  cleanup:
     virObjectUnlock(closeCallbacks);
     return ret;


ACK with this (or equivalent) diff squashed in.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list







