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

Re: [libvirt] [PATCH 03/14] Fix crash when deleting monitor while a command is in progress



Hi Daniel,
I'm giving it some heavy testing and seems to be rock solid! Thanks a
lot for Your effort!
I'm going to add few tens of guests to torture libvirt with multiple simultaneous
accesses even more...
regards
nik


On Mon, Dec 07, 2009 at 02:18:44PM +0000, Daniel P. Berrange wrote:
> On Thu, Dec 03, 2009 at 11:12:40AM +0000, Daniel P. Berrange wrote:
> > On Thu, Dec 03, 2009 at 12:47:02AM +0100, Matthias Bolte wrote:
> > > 2009/11/26 Daniel P. Berrange <berrange redhat com>:
> > > > If QEMU shuts down while we're in the middle of processing a
> > > > monitor command, the monitor will be freed, and upon cleaning
> > > > up we attempt to do  qemuMonitorUnlock(priv->mon) when priv->mon
> > > > is NULL.
> > > >
> > > > To address this we introduce proper reference counting into
> > > > the qemuMonitorPtr object, and hold an extra reference whenever
> > > > executing a command.
> > > >
> > > > * src/qemu/qemu_driver.c: Hold a reference on the monitor while
> > > >  executing commands, and only NULL-ify the priv->mon field when
> > > >  the last reference is released
> > > > * src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Add reference
> > > >  counting to handle safe deletion of monitor objects
> > > 
> > > The locking pattern below results in destroying a locked mutex. It
> > > this intended?
> > 
> > No, that's a bug, the qemuMonitorUnref() call itself should have
> > called unlock if the ref count hit 0, before destroying it.
> > 
> > > qemuMonitorLock(mon);
> > > [...]
> > > if (qemuMonitorUnref(mon) > 0)
> > >     qemuMonitorUnlock(mon);
> > > 
> > > Well, this patch makes the TCK deadlock for me, seems to be a lock
> > > ordering issue combined with a race condition; it doesn't happen every
> > > run. I don't understand all details of the locking and refcounting
> > > scheme of the QEMU monitor yet, it's quite complex and gets even more
> > > complex.
> > 
> > Yes, that problem shown by valgrind will indeed deadlock. I'll fix
> > this. The domain object lock must never be acquired if the thread
> > holds the monitor lock already. We must have strict ordering from
> > top to bottom (driver -> domain object -> qemu monitor)
> 
> 
> This is the patch I'm proposing instead. It removes the ref count adjust
> of virDomainObjPtr from qemuMonitor, since that requires that you have
> the bad lock ordering. Instead that extra ref count is added/removed by
> the qemu driver code. This means the qemuMonitor never has to touch any
> locks on virDomainObjPtr which complies with our strict top->bottom
> ordering requirement.
> 
> 
> commit c2b32f964b0eed861dea11e5037993e3a3c3fb3d
> Author: Daniel P. Berrange <berrange redhat com>
> Date:   Thu Nov 26 13:29:29 2009 +0000
> 
>     Fix crash when deleting monitor while a command is in progress
>     
>     If QEMU shuts down while we're in the middle of processing a
>     monitor command, the monitor will be freed, and upon cleaning
>     up we attempt to do  qemuMonitorUnlock(priv->mon) when priv->mon
>     is NULL.
>     
>     To address this we introduce proper reference counting into
>     the qemuMonitorPtr object, and hold an extra reference whenever
>     executing a command.
>     
>     * src/qemu/qemu_driver.c: Hold a reference on the monitor while
>       executing commands, and only NULL-ify the priv->mon field when
>       the last reference is released
>     * src/qemu/qemu_monitor.h, src/qemu/qemu_monitor.c: Add reference
>       counting to handle safe deletion of monitor objects
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 43d20ea..fd47dc0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -272,6 +272,7 @@ static void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
>      qemuDomainObjPrivatePtr priv = obj->privateData;
>  
>      qemuMonitorLock(priv->mon);
> +    qemuMonitorRef(priv->mon);
>      virDomainObjUnlock(obj);
>  }
>  
> @@ -283,9 +284,16 @@ static void qemuDomainObjEnterMonitor(virDomainObjPtr obj)
>  static void qemuDomainObjExitMonitor(virDomainObjPtr obj)
>  {
>      qemuDomainObjPrivatePtr priv = obj->privateData;
> +    int refs;
>  
> +    refs = qemuMonitorUnref(priv->mon);
>      qemuMonitorUnlock(priv->mon);
>      virDomainObjLock(obj);
> +
> +    if (refs == 0) {
> +        virDomainObjUnref(obj);
> +        priv->mon = NULL;
> +    }
>  }
>  
>  
> @@ -302,6 +310,7 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir
>      qemuDomainObjPrivatePtr priv = obj->privateData;
>  
>      qemuMonitorLock(priv->mon);
> +    qemuMonitorRef(priv->mon);
>      virDomainObjUnlock(obj);
>      qemuDriverUnlock(driver);
>  }
> @@ -315,10 +324,17 @@ static void qemuDomainObjEnterMonitorWithDriver(struct qemud_driver *driver, vir
>  static void qemuDomainObjExitMonitorWithDriver(struct qemud_driver *driver, virDomainObjPtr obj)
>  {
>      qemuDomainObjPrivatePtr priv = obj->privateData;
> +    int refs;
>  
> +    refs = qemuMonitorUnref(priv->mon);
>      qemuMonitorUnlock(priv->mon);
>      qemuDriverLock(driver);
>      virDomainObjLock(obj);
> +
> +    if (refs == 0) {
> +        virDomainObjUnref(obj);
> +        priv->mon = NULL;
> +    }
>  }
>  
>  
> @@ -653,6 +669,10 @@ qemuConnectMonitor(virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>  
> +    /* Hold an extra reference because we can't allow 'vm' to be
> +     * deleted while the monitor is active */
> +    virDomainObjRef(vm);
> +
>      if ((priv->mon = qemuMonitorOpen(vm, qemuHandleMonitorEOF)) == NULL) {
>          VIR_ERROR(_("Failed to connect monitor for %s\n"), vm->def->name);
>          return -1;
> @@ -2400,8 +2420,9 @@ static void qemudShutdownVMDaemon(virConnectPtr conn,
>                               _("Failed to send SIGTERM to %s (%d)"),
>                               vm->def->name, vm->pid);
>  
> -    if (priv->mon) {
> -        qemuMonitorClose(priv->mon);
> +    if (priv->mon &&
> +        qemuMonitorClose(priv->mon) == 0) {
> +        virDomainObjUnref(vm);
>          priv->mon = NULL;
>      }
>  
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index f0ef81b..a2e10bc 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -42,7 +42,7 @@ struct _qemuMonitor {
>      virMutex lock;
>      virCond notify;
>  
> -    virDomainObjPtr dom;
> +    int refs;
>  
>      int fd;
>      int watch;
> @@ -67,12 +67,14 @@ struct _qemuMonitor {
>       * the next monitor msg */
>      int lastErrno;
>  
> -    /* If the monitor callback is currently active */
> +    /* If the monitor EOF callback is currently active (stops more commands being run) */
>      unsigned eofcb: 1;
> -    /* If the monitor callback should free the closed monitor */
> +    /* If the monitor is in process of shutting down */
>      unsigned closed: 1;
> +
>  };
>  
> +
>  void qemuMonitorLock(qemuMonitorPtr mon)
>  {
>      virMutexLock(&mon->lock);
> @@ -84,21 +86,33 @@ void qemuMonitorUnlock(qemuMonitorPtr mon)
>  }
>  
>  
> -static void qemuMonitorFree(qemuMonitorPtr mon, int lockDomain)
> +static void qemuMonitorFree(qemuMonitorPtr mon)
>  {
> -    VIR_DEBUG("mon=%p, lockDomain=%d", mon, lockDomain);
> -    if (mon->vm) {
> -        if (lockDomain)
> -            virDomainObjLock(mon->vm);
> -        if (!virDomainObjUnref(mon->vm) && lockDomain)
> -            virDomainObjUnlock(mon->vm);
> -    }
> +    VIR_DEBUG("mon=%p", mon);
>      if (virCondDestroy(&mon->notify) < 0)
>      {}
>      virMutexDestroy(&mon->lock);
>      VIR_FREE(mon);
>  }
>  
> +int qemuMonitorRef(qemuMonitorPtr mon)
> +{
> +    mon->refs++;
> +    return mon->refs;
> +}
> +
> +int qemuMonitorUnref(qemuMonitorPtr mon)
> +{
> +    mon->refs--;
> +
> +    if (mon->refs == 0) {
> +        qemuMonitorUnlock(mon);
> +        qemuMonitorFree(mon);
> +    }
> +
> +    return mon->refs;
> +}
> +
>  
>  static int
>  qemuMonitorOpenUnix(const char *monitor)
> @@ -348,6 +362,7 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) {
>      int quit = 0, failed = 0;
>  
>      qemuMonitorLock(mon);
> +    qemuMonitorRef(mon);
>      VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, fd, events);
>  
>      if (mon->fd != fd || mon->watch != watch) {
> @@ -407,23 +422,17 @@ qemuMonitorIO(int watch, int fd, int events, void *opaque) {
>       * but is this safe ?  I think it is, because the callback
>       * will try to acquire the virDomainObjPtr mutex next */
>      if (failed || quit) {
> +        qemuMonitorEOFNotify eofCB = mon->eofCB;
> +        virDomainObjPtr vm = mon->vm;
>          /* Make sure anyone waiting wakes up now */
>          virCondSignal(&mon->notify);
> -        mon->eofcb = 1;
> -        qemuMonitorUnlock(mon);
> -        VIR_DEBUG("Triggering EOF callback error? %d", failed);
> -        mon->eofCB(mon, mon->vm, failed);
> -
> -        qemuMonitorLock(mon);
> -        if (mon->closed) {
> +        if (qemuMonitorUnref(mon) > 0)
>              qemuMonitorUnlock(mon);
> -            VIR_DEBUG("Delayed free of monitor %p", mon);
> -            qemuMonitorFree(mon, 1);
> -        } else {
> -            qemuMonitorUnlock(mon);
> -        }
> +        VIR_DEBUG("Triggering EOF callback error? %d", failed);
> +        (eofCB)(mon, vm, failed);
>      } else {
> -        qemuMonitorUnlock(mon);
> +        if (qemuMonitorUnref(mon) > 0)
> +            qemuMonitorUnlock(mon);
>      }
>  }
>  
> @@ -453,10 +462,10 @@ qemuMonitorOpen(virDomainObjPtr vm,
>          return NULL;
>      }
>      mon->fd = -1;
> +    mon->refs = 1;
>      mon->vm = vm;
>      mon->eofCB = eofCB;
>      qemuMonitorLock(mon);
> -    virDomainObjRef(vm);
>  
>      switch (vm->monitor_chr->type) {
>      case VIR_DOMAIN_CHR_TYPE_UNIX:
> @@ -512,10 +521,14 @@ cleanup:
>  }
>  
>  
> -void qemuMonitorClose(qemuMonitorPtr mon)
> +int qemuMonitorClose(qemuMonitorPtr mon)
>  {
> +    int refs;
> +
>      if (!mon)
> -        return;
> +        return 0;
> +
> +    VIR_DEBUG("mon=%p", mon);
>  
>      qemuMonitorLock(mon);
>      if (!mon->closed) {
> @@ -523,18 +536,17 @@ void qemuMonitorClose(qemuMonitorPtr mon)
>              virEventRemoveHandle(mon->watch);
>          if (mon->fd != -1)
>              close(mon->fd);
> -        /* NB: don't reset  fd / watch fields, since active
> -         * callback may still want them */
> +        /* NB: ordinarily one might immediately set mon->watch to -1
> +         * and mon->fd to -1, but there may be a callback active
> +         * that is still relying on these fields being valid. So
> +         * we merely close them, but not clear their values and
> +         * use this explicit 'closed' flag to track this state */
>          mon->closed = 1;
>      }
>  
> -    if (mon->eofcb) {
> -        VIR_DEBUG("Mark monitor to be deleted %p", mon);
> +    if ((refs = qemuMonitorUnref(mon)) > 0)
>          qemuMonitorUnlock(mon);
> -    } else {
> -        VIR_DEBUG("Delete monitor now %p", mon);
> -        qemuMonitorFree(mon, 0);
> -    }
> +    return refs;
>  }
>  
>  
> @@ -552,7 +564,6 @@ int qemuMonitorSend(qemuMonitorPtr mon,
>  
>      if (mon->eofcb) {
>          msg->lastErrno = EIO;
> -        qemuMonitorUnlock(mon);
>          return -1;
>      }
>  
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 71688cb..27b43b7 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -78,11 +78,14 @@ typedef int (*qemuMonitorDiskSecretLookup)(qemuMonitorPtr mon,
>  qemuMonitorPtr qemuMonitorOpen(virDomainObjPtr vm,
>                                 qemuMonitorEOFNotify eofCB);
>  
> -void qemuMonitorClose(qemuMonitorPtr mon);
> +int qemuMonitorClose(qemuMonitorPtr mon);
>  
>  void qemuMonitorLock(qemuMonitorPtr mon);
>  void qemuMonitorUnlock(qemuMonitorPtr mon);
>  
> +int qemuMonitorRef(qemuMonitorPtr mon);
> +int qemuMonitorUnref(qemuMonitorPtr mon);
> +
>  void qemuMonitorRegisterDiskSecretLookup(qemuMonitorPtr mon,
>                                           qemuMonitorDiskSecretLookup secretCB);
>  
> 
> -- 
> |: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
> |: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
> 
> --
> Libvir-list mailing list
> Libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

-- 
-------------------------------------
Nikola CIPRICH
LinuxBox.cz, s.r.o.
28. rijna 168, 709 01 Ostrava

tel.:   +420 596 603 142
fax:    +420 596 621 273
mobil:  +420 777 093 799

www.linuxbox.cz

mobil servis: +420 737 238 656
email servis: servis linuxbox cz
-------------------------------------


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