Changeset 14143 in vbox for trunk/src/VBox/Main/HardDisk2Impl.cpp

Timestamp:

Nov 12, 2008 8:06:37 PM (16 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

39231

Message:

Main: Improved hard disk tree lock usage to avoid possible deadlocks when walking the hard disk tree upwards (from children to parents).

File:

• trunk/src/VBox/Main/HardDisk2Impl.cpp (modified) (42 diffs)

trunk/src/VBox/Main/HardDisk2Impl.cpp

@@ -176 +176 @@
 /**
  * Helper class for merge operations.
+ *
+ * @note It is assumed that when modifying methods of this class are called,
+ *       HardDisk2::treeLock() is held in read mode.
  */
 class HardDisk2::MergeChain : public HardDisk2::List,
@@ -571 +574 @@
  * @param aParent       Parent hard disk or NULL for a root hard disk.
  * @param aNode         <HardDisk> settings node.
+ *
+ * @note Locks VirtualBox lock for writing, treeLock() for writing.
  */
 HRESULT HardDisk2::init (VirtualBox *aVirtualBox, HardDisk2 *aParent,
@@ -587 +592 @@
     /* share VirtualBox and parent weakly */
     unconst (mVirtualBox) = aVirtualBox;
-    mParent = aParent;
 
     /* register with VirtualBox/parent early, since uninit() will
      * unconditionally unregister on failure */
-    if (!aParent)
+    if (aParent == NULL)
         aVirtualBox->addDependentChild (this);
-    else
+
+    {
+        /* we set mParent */
+        AutoWriteLock treeLock (this->treeLock());
+
+        mParent = aParent;
         aParent->addDependentChild (this);
+    }
 
     /* see below why we don't call queryInfo() (and therefore treat the medium
@@ -670 +680 @@
  * @note All children of this hard disk get uninitialized by calling their
  *       uninit() methods.
+ *
+ * @note Locks treeLock() for writing, VirtualBox for writing.
  */
 void HardDisk2::uninit()
@@ -683 +695 @@
          * Reparenting has already been done so don't touch it here (we are
          * now orphans and remoeDependentChild() will assert) */
+
+        Assert (mParent.isNull());
     }
     else
     {
+        /* we uninit children and reset mParent
+         * and VirtualBox::removeDependentChild() needs a write lock */
+        AutoMultiWriteLock2 alock (mVirtualBox->lockHandle(), this->treeLock());
+
         uninitDependentChildren();
 
-        if (mParent)
+        if (!mParent.isNull())
+        {
             mParent->removeDependentChild (this);
+            mParent.setNull();
+        }
         else
             mVirtualBox->removeDependentChild (this);
     }
 
-    mParent.setNull();
     unconst (mVirtualBox).setNull();
 }
@@ -752 +772 @@
         return S_OK;
     }
+
+    /* we access mParent & children() */
+    AutoReadLock treeLock (this->treeLock());
 
     /* cannot change the type of a differencing hard disk */
@@ -803 +826 @@
     CheckComRCReturnRC (autoCaller.rc());
 
-    AutoReadLock alock (this);
+    /* we access mParent */
+    AutoReadLock treeLock (this->treeLock());
 
     mParent.queryInterfaceTo (aParent);
@@ -818 +842 @@
     CheckComRCReturnRC (autoCaller.rc());
 
-    AutoReadLock alock (this);
+    /* we access children */
+    AutoReadLock treeLock (this->treeLock());
 
     SafeIfaceArray <IHardDisk2> children (this->children());
@@ -863 +888 @@
 
         AutoReadLock alock (this);
+
+        /* we access mParent */
+        AutoReadLock treeLock (this->treeLock());
 
         if (mParent.isNull())
@@ -1081 +1109 @@
  * @param aNewPath  New path (full).
  *
- * @note Locks this object and all children for writing.
+ * @note Locks treeLock() for reading, this object and all children for writing.
  */
 void HardDisk2::updatePaths (const char *aOldPath, const char *aNewPath)
@@ -1092 +1120 @@
 
     AutoWriteLock alock (this);
+
+    /* we access children() */
+    AutoReadLock treeLock (this->treeLock());
 
     updatePath (aOldPath, aNewPath);
@@ -1114 +1145 @@
  *                  (zero for the root), may be @c NULL.
  *
- * @note This method may return an uninitialized object if it happens in
- *       parallel with such an operation as the VirtualBox shutdown or hard disk
- *       merge.
- *
- * @note Locks this object and ancestors for reading. Neither this object nor
- *       its ancestors may be locked or have active callers on the current
- *       thread otherwise a deadlock or an endless loop will occur.
+ * @note Locks treeLock() for reading.
  */
 ComObjPtr <HardDisk2> HardDisk2::root (uint32_t *aLevel /*= NULL*/)
 {
-    /* Note: This method locks hard disks sequentially to avoid breaking the
-     * {parent,child} lock order which is impossible to follow here. As a
-     * result, any hard disk in the chain may become uninitialized before this
-     * method finishes walking (for example, as a result of the merge operation
-     * which removes hard disks from the chain). For this reason, we retry the
-     * walk from the beginnig each time it happens (because we must always
-     * return a non-null object).
-     *
-     * As the worst case, we end up having ourselves uninitialized (either
-     * because VirtualBox has initiated the shutdown procedure, or because we
-     * were closed/deleted/etc). In this case, we just return ourselves to avoid
-     * the endless loop.
-     */
-
     ComObjPtr <HardDisk2> root;
     uint32_t level;
 
-    do
-    {
-        root = this;
-        level = 0;
-
-        do
-        {
-            AutoCaller autoCaller (root);
-            if (!autoCaller.isOk())
-            {
-                /* break the endless loop, see above */
-                if (root == this)
-                    break;
-
-                /* cause to start over with this object */
-                root.setNull();
-                break;
-            }
-
-            AutoReadLock alock (root);
+    AutoCaller autoCaller (this);
+    AssertReturn (autoCaller.isOk(), root);
+
+    /* we access mParent */
+    AutoReadLock treeLock (this->treeLock());
+
+    root = this;
+    level = 0;
+
+    if (!mParent.isNull())
+    {
+        for (;;)
+        {
+            AutoCaller rootCaller (root);
+            AssertReturn (rootCaller.isOk(), root);
 
             if (root->mParent.isNull())
@@ -1168 +1174 @@
             ++ level;
         }
-        while (true);
-    }
-    while (root.isNull());
+    }
 
     if (aLevel != NULL)
@@ -1183 +1187 @@
  * type and posterity, not to the current media state.
  *
- * @note Locks this object for reading.
+ * @note Locks this object and treeLock() for reading.
  */
 bool HardDisk2::isReadOnly()
@@ -1191 +1195 @@
 
     AutoReadLock alock (this);
+
+    /* we access children */
+    AutoReadLock treeLock (this->treeLock());
 
     switch (mm.type)
@@ -1227 +1234 @@
  * @param aaParentNode  Parent <HardDisks> or <HardDisk> node.
  *
- * @note Locks this object for reading.
+ * @note Locks this object, treeLock() and children for reading.
  */
 HRESULT HardDisk2::saveSettings (settings::Key &aParentNode)
@@ -1239 +1246 @@
 
     AutoReadLock alock (this);
+
+    /* we access mParent */
+    AutoReadLock treeLock (this->treeLock());
 
     Key diskNode = aParentNode.appendKey ("HardDisk");
@@ -1402 +1412 @@
  *                      if no real merge is necessary).
  *
- * @note Locks the branch lock for reading. Locks this object, aTarget and all
+ * @note Locks treeLock() for reading. Locks this object, aTarget and all
  *       intermediate hard disks for writing.
  */
@@ -1413 +1423 @@
 
     AutoWriteLock alock (this);
+
+    /* we access mParent & children() */
+    AutoReadLock treeLock (this->treeLock());
 
     AssertReturn (mm.type == HardDiskType_Normal, E_FAIL);
@@ -1528 +1541 @@
  *                      no real merge takes place).
  *
- * @note Locks the branch lock for writing. Locks the hard disks from the chain
- *       for writing. Locks the machine object when the backward merge takes
- *       place.
+ * @note Locks the hard disks from the chain for writing. Locks the machine
+ *       object when the backward merge takes place. Locks treeLock() lock for
+ *       reading or writing.
  */
 HRESULT HardDisk2::discard (ComObjPtr <Progress> &aProgress, MergeChain *aChain)
@@ -1550 +1563 @@
         {
             AutoWriteLock alock (this);
+
+            /* we access mParent & children() */
+            AutoReadLock treeLock (this->treeLock());
 
             Assert (children().size() == 0);
@@ -1599 +1615 @@
  *                      no real merge takes place).
  *
- * @note Locks the hard disks from the chain for writing.
+ * @note Locks the hard disks from the chain for writing. Locks treeLock() for
+ *       reading.
  */
 void HardDisk2::cancelDiscard (MergeChain *aChain)
@@ -1609 +1626 @@
     {
         AutoWriteLock alock (this);
+
+        /* we access mParent & children() */
+        AutoReadLock treeLock (this->treeLock());
 
         Assert (children().size() == 0);
@@ -1656 +1676 @@
  *                      an asynchronous thread.
  *
- * @note Locks mVirtualBox, mParent and this object for writing.
+ * @note Locks mVirtualBox and this object for writing. Locks treeLock() for
+ *       writing.
  */
 HRESULT HardDisk2::deleteStorage (ComObjPtr <Progress> *aProgress, bool aWait)
 {
     AssertReturn (aProgress != NULL || aWait == true, E_FAIL);
-
-    AutoCaller autoCaller (this);
-    CheckComRCReturnRC (autoCaller.rc());
 
     /* unregisterWithVirtualBox() needs a write lock. We want to unregister
@@ -1669 +1687 @@
      * sure that we don't do that after another thread has done
      * VirtualBox::findHardDisk2() but before it starts using us (provided that
-     * it holds a mVirtualBox lock too of course).
-     *
-     * mParent->removeDependentChild() needs a write lock too. */
-
-    AutoMultiWriteLock3 alock (mVirtualBox, mParent, this);
+     * it holds a mVirtualBox lock too of course). */
+
+    AutoWriteLock vboxLock (mVirtualBox);
+
+    AutoWriteLock alock (this);
 
     switch (m.state)
@@ -1691 +1709 @@
     CheckComRCReturnRC (rc);
 
+    /* go to Deleting state before leaving the lock */
+    m.state = MediaState_Deleting;
+
+    /* we need to leave this object's write lock now because of
+     * unregisterWithVirtualBox() that locks treeLock() for writing */
+    alock.leave();
+
     /* try to remove from the list of known hard disks before performing actual
      * deletion (we favor the consistency of the media registry in the first
@@ -1697 +1722 @@
 
     rc = unregisterWithVirtualBox();
+
+    alock.enter();
+
+    /* restore the state because we may fail below; we will set it later again*/
+    m.state = MediaState_Created;
+
     CheckComRCReturnRC (rc);
 
@@ -1916 +1947 @@
  *                              hard disk is attached to any VM.
  *
- * @note Locks the branch lock for reading. Locks this object, aTarget and all
+ * @note Locks treeLock() for reading. Locks this object, aTarget and all
  *       intermediate hard disks for writing.
  */
@@ -1928 +1959 @@
     AssertComRCReturnRC (autoCaller.rc());
 
-    AutoCaller targetCaller (this);
+    AutoCaller targetCaller (aTarget);
     AssertComRCReturnRC (targetCaller.rc());
 
     aChain = NULL;
 
-    /* tree lock is always the first */
+    /* we walk the tree */
     AutoReadLock treeLock (this->treeLock());
 
@@ -1956 +1987 @@
             {
                 Bstr tgtLoc;
-                rc = aTarget->COMGETTER(Location) (tgtLoc.asOutParam());
-                CheckComRCThrowRC (rc);
+                {
+                    AutoReadLock alock (this);
+                    tgtLoc = aTarget->locationFull();
+                }
 
                 AutoReadLock alock (this);
@@ -2248 +2281 @@
  *       accessibility.
  *
- * @note Locks this object for writing.
+ * @note Locks treeLock() for reading and writing (for new diff media checked
+ *       for the first time). Locks mParent for reading. Locks this object for
+ *       writing.
  */
 HRESULT HardDisk2::queryInfo()
@@ -2413 +2448 @@
                     }
 
-                    /* associate with parent, deassociate from VirtualBox */
+                    /* deassociate from VirtualBox, associate with parent */
+
+                    mVirtualBox->removeDependentChild (this);
+
+                    /* we set mParent & children() */
+                    AutoWriteLock treeLock (this->treeLock());
+
                     Assert (mParent.isNull());
                     mParent = parent;
                     mParent->addDependentChild (this);
-                    mVirtualBox->removeDependentChild (this);
                 }
                 else
                 {
+                    /* we access mParent */
+                    AutoReadLock treeLock (this->treeLock());
+
                     /* check that parent UUIDs match. Note that there's no need
                      * for the parent's AutoCaller (our lifetime is bound to
@@ -2514 +2557 @@
  * @note Called from this object's AutoMayUninitSpan and from under mVirtualBox
  *       write lock.
+ *
+ * @note Locks treeLock() for reading.
  */
 HRESULT HardDisk2::canClose()
 {
+    /* we access children */
+    AutoReadLock treeLock (this->treeLock());
+
     if (children().size() != 0)
         return setError (E_FAIL,
@@ -2544 +2592 @@
  *       from under mVirtualBox write lock.
  *
- * @note Locks mParent for writing.
+ * @note Locks treeLock() for writing.
  */
 HRESULT HardDisk2::unregisterWithVirtualBox()
@@ -2551 +2599 @@
      * unregisterHardDisk2() properly save the registry */
 
+    /* we modify mParent and access children */
+    AutoWriteLock treeLock (this->treeLock());
+
     const ComObjPtr <HardDisk2, ComWeakRef> parent = mParent;
-
-    /* Lock parent to make the try atomic WRT to mParent->COMGETTER(Children) */
-    AutoWriteLock parentLock (parent);
 
     AssertReturn (children().size() == 0, E_FAIL);
@@ -2880 +2928 @@
             if (SUCCEEDED (rc))
             {
-                /* mVirtualBox->registerHardDisk2() needs a write lock */
-                AutoWriteLock vboxLock (that->mVirtualBox);
-                thatLock.enter();
-
-                target->m.size = size;
-                target->mm.logicalSize = logicalSize;
+                /* we set mParent & children() (note that thatLock is released
+                 * here), but lock VirtualBox first to follow the rule */
+                AutoMultiWriteLock2 alock (that->mVirtualBox->lockHandle(),
+                                           that->treeLock());
 
                 Assert (target->mParent.isNull());
@@ -2900 +2946 @@
                 rc = that->mVirtualBox->registerHardDisk2 (target);
 
-                if (SUCCEEDED (rc))
-                {
-                    target->m.state = MediaState_Created;
-                }
-                else
+                if (FAILED (rc))
                 {
                     /* break the parent association on failure to register */
@@ -2913 +2955 @@
             }
 
-            if (FAILED (rc))
+            thatLock.maybeEnter();
+
+            if (SUCCEEDED (rc))
             {
-                thatLock.maybeEnter();
-
+                target->m.state = MediaState_Created;
+
+                target->m.size = size;
+                target->mm.logicalSize = logicalSize;
+            }
+            else
+            {
                 /* back to NotCreated on failiure */
                 target->m.state = MediaState_NotCreated;
@@ -2980 +3029 @@
                         /* complex sanity (sane complexity) */
                         Assert ((chain->isForward() &&
-                                 (*it != chain->back() &&
-                                  (*it)->m.state == MediaState_Deleting) ||
-                                 (*it == chain->back() &&
-                                  (*it)->m.state == MediaState_LockedWrite)) ||
+                                 ((*it != chain->back() &&
+                                   (*it)->m.state == MediaState_Deleting) ||
+                                  (*it == chain->back() &&
+                                   (*it)->m.state == MediaState_LockedWrite))) ||
                                 (!chain->isForward() &&
-                                 (*it != chain->front() &&
-                                  (*it)->m.state == MediaState_Deleting) ||
-                                 (*it == chain->front() &&
-                                  (*it)->m.state == MediaState_LockedWrite)));
+                                 ((*it != chain->front() &&
+                                   (*it)->m.state == MediaState_Deleting) ||
+                                  (*it == chain->front() &&
+                                   (*it)->m.state == MediaState_LockedWrite))));
 
                         Assert (*it == chain->target() ||
@@ -3088 +3137 @@
                  * VDMerge; reparent the last one and uninitialize deleted */
 
-                /* mVirtualBox->(un)registerHardDisk2() needs a write lock */
-                AutoWriteLock vboxLock (that->mVirtualBox);
-
-                /* we will reparent */
-                AutoWriteLock treeLock (that->mVirtualBox->hardDiskTreeHandle());
+                /* we set mParent & children() (note that thatLock is released
+                 * here), but lock VirtualBox first to follow the rule */
+                AutoMultiWriteLock2 alock (that->mVirtualBox->lockHandle(),
+                                           that->treeLock());
 
                 HardDisk2 *source = chain->source();
@@ -3104 +3152 @@
                         unregisterHardDisk2 (target, false /* aSaveSettings */);
                     AssertComRC (rc2);
-
-                    /* obey {parent,child} lock order (add/remove methods below
-                     * do locking) */
-                    AutoWriteLock parentLock (chain->parent());
-                    AutoWriteLock sourceLock (source);
-                    AutoWriteLock targetLock (target);
 
                     /* then, reparent it and disconnect the deleted branch at
@@ -3134 +3176 @@
                 else
                 {
-                    /* obey {parent,child} lock order (add/remove methods below
-                     * do locking) */
-                    AutoWriteLock targetLock (target);
-
                     Assert (target->children().size() == 1);
                     HardDisk2 *targetChild = target->children().front();
-
-                    AutoWriteLock targetChildLock (target);
 
                     /* disconnect the deleted branch at the elder end */