Changeset 40466 in vbox for trunk/src/VBox

Timestamp:

Mar 14, 2012 2:34:15 PM (13 years ago)

Author:

vboxsync

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

76832

Message:

Main/Medium: another go at the medium locking related to queryInfo, since there was a remaining deadlock with the LockWrite method.

Location:

trunk/src/VBox/Main

Files:

• include/MediumImpl.h (modified) (1 diff)
• src-server/MediumImpl.cpp (modified) (13 diffs)

trunk/src/VBox/Main/include/MediumImpl.h

@@ -265 +265 @@
 
     HRESULT queryInfo(bool fSetImageId, bool fSetParentId);
-    HRESULT lockRead(MediumState_T *aState, bool fWithinQueryInfo);
-    HRESULT lockWrite(MediumState_T *aState, bool fWithinQueryInfo);
 
     HRESULT canClose();

trunk/src/VBox/Main/src-server/MediumImpl.cpp

@@ -2080 +2080 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    return lockRead(aState, false /* fWithinQueryInfo */);
+    /* Must not hold the object lock, as we need control over it below. */
+    Assert(!isWriteLockOnCurrentThread());
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    if (m->queryInfoRunning)
+    {
+        /* Must not hold the media tree lock, as Medium::queryInfo needs this
+         * lock and thus we would run into a deadlock here. */
+        Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+        while (m->queryInfoRunning)
+        {
+            alock.release();
+            {
+                AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+            }
+            alock.acquire();
+        }
+    }
+
+    /* return the current state before */
+    if (aState)
+        *aState = m->state;
+
+    HRESULT rc = S_OK;
+
+    switch (m->state)
+    {
+        case MediumState_Created:
+        case MediumState_Inaccessible:
+        case MediumState_LockedRead:
+        {
+            ++m->readers;
+
+            ComAssertMsgBreak(m->readers != 0, ("Counter overflow"), rc = E_FAIL);
+
+            /* Remember pre-lock state */
+            if (m->state != MediumState_LockedRead)
+                m->preLockState = m->state;
+
+            LogFlowThisFunc(("Okay - prev state=%d readers=%d\n", m->state, m->readers));
+            m->state = MediumState_LockedRead;
+
+            break;
+        }
+        default:
+        {
+            LogFlowThisFunc(("Failing - state=%d\n", m->state));
+            rc = setStateError();
+            break;
+        }
+    }
+
+    return rc;
 }
 
@@ -2143 +2196 @@
     if (FAILED(autoCaller.rc())) return autoCaller.rc();
 
-    return lockWrite(aState, false /* fWithinQueryInfo */);
+    /* Must not hold the object lock, as we need control over it below. */
+    Assert(!isWriteLockOnCurrentThread());
+    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
+
+    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    if (m->queryInfoRunning)
+    {
+        /* Must not hold the media tree lock, as Medium::queryInfo needs this
+         * lock and thus we would run into a deadlock here. */
+        Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+        while (m->queryInfoRunning)
+        {
+            alock.release();
+            {
+                AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+            }
+            alock.acquire();
+        }
+    }
+
+    /* return the current state before */
+    if (aState)
+        *aState = m->state;
+
+    HRESULT rc = S_OK;
+
+    switch (m->state)
+    {
+        case MediumState_Created:
+        case MediumState_Inaccessible:
+        {
+            m->preLockState = m->state;
+
+            LogFlowThisFunc(("Okay - prev state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
+            m->state = MediumState_LockedWrite;
+            break;
+        }
+        default:
+        {
+            LogFlowThisFunc(("Failing - state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
+            rc = setStateError();
+            break;
+        }
+    }
+
+    return rc;
 }
 
@@ -5188 +5286 @@
 
 /**
- * @note @a aState may be NULL if the state value is not needed (only for
- *       in-process calls).
- */
-HRESULT Medium::lockRead(MediumState_T *aState, bool fWithinQueryInfo)
-{
-    /* Must not hold the object lock, as we need control over it below. */
+ * Queries information from the medium.
+ *
+ * As a result of this call, the accessibility state and data members such as
+ * size and description will be updated with the current information.
+ *
+ * @note This method may block during a system I/O call that checks storage
+ *       accessibility.
+ *
+ * @note Caller MUST NOT hold the media tree or medium lock.
+ *
+ * @note Locks mParent for reading. Locks this object for writing.
+ *
+ * @param fSetImageId Whether to reset the UUID contained in the image file to the UUID in the medium instance data (see SetIDs())
+ * @param fSetParentId Whether to reset the parent UUID contained in the image file to the parent UUID in the medium instance data (see SetIDs())
+ * @return
+ */
+HRESULT Medium::queryInfo(bool fSetImageId, bool fSetParentId)
+{
     Assert(!isWriteLockOnCurrentThread());
     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
 
-    /* Wait for a concurrently running Medium::queryInfo to complete. */
+    if (   m->state != MediumState_Created
+        && m->state != MediumState_Inaccessible
+        && m->state != MediumState_LockedRead)
+        return E_FAIL;
+
+    HRESULT rc = S_OK;
+
+    int vrc = VINF_SUCCESS;
+
+    /* check if a blocking queryInfo() call is in progress on some other thread,
+     * and wait for it to finish if so instead of querying data ourselves */
     if (m->queryInfoRunning)
     {
-        /* Must not hold the media tree lock, as Medium::queryInfo needs this
-         * lock and thus we would run into a deadlock here. */
-        if (!fWithinQueryInfo)
-            Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+        Assert(   m->state == MediumState_LockedRead
+               || m->state == MediumState_LockedWrite);
+
         while (m->queryInfoRunning)
         {
@@ -5212 +5331 @@
             alock.acquire();
         }
-    }
-
-    /* return the current state before */
-    if (aState)
-        *aState = m->state;
-
-    HRESULT rc = S_OK;
-
-    switch (m->state)
-    {
-        case MediumState_Created:
-        case MediumState_Inaccessible:
-        case MediumState_LockedRead:
-        {
-            ++m->readers;
-
-            ComAssertMsgBreak(m->readers != 0, ("Counter overflow"), rc = E_FAIL);
-
-            /* Remember pre-lock state */
-            if (m->state != MediumState_LockedRead)
-                m->preLockState = m->state;
-
-            LogFlowThisFunc(("Okay - prev state=%d readers=%d\n", m->state, m->readers));
-            m->state = MediumState_LockedRead;
-
-            break;
-        }
-        default:
-        {
-            LogFlowThisFunc(("Failing - state=%d\n", m->state));
-            rc = setStateError();
-            break;
-        }
-    }
-
-    return rc;
-}
-
-/**
- * @note @a aState may be NULL if the state value is not needed (only for
- *       in-process calls).
- */
-HRESULT Medium::lockWrite(MediumState_T *aState, bool fWithinQueryInfo)
-{
-    /* Must not hold the object lock, as we need control over it below. */
-    Assert(!isWriteLockOnCurrentThread());
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    /* Wait for a concurrently running Medium::queryInfo to complete. */
-    if (m->queryInfoRunning)
-    {
-        /* Must not hold the media tree lock, as Medium::queryInfo needs this
-         * lock and thus we would run into a deadlock here. */
-        if (!fWithinQueryInfo)
-            Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-        while (m->queryInfoRunning)
-        {
-            alock.release();
-            {
-                AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
-            }
-            alock.acquire();
-        }
-    }
-
-    /* return the current state before */
-    if (aState)
-        *aState = m->state;
-
-    HRESULT rc = S_OK;
-
-    switch (m->state)
-    {
-        case MediumState_Created:
-        case MediumState_Inaccessible:
-        {
-            m->preLockState = m->state;
-
-            LogFlowThisFunc(("Okay - prev state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
-            m->state = MediumState_LockedWrite;
-            break;
-        }
-        default:
-        {
-            LogFlowThisFunc(("Failing - state=%d locationFull=%s\n", m->state, getLocationFull().c_str()));
-            rc = setStateError();
-            break;
-        }
-    }
-
-    return rc;
-}
-
-/**
- * Queries information from the medium.
- *
- * As a result of this call, the accessibility state and data members such as
- * size and description will be updated with the current information.
- *
- * @note This method may block during a system I/O call that checks storage
- *       accessibility.
- *
- * @note Caller MUST NOT hold the media tree or medium lock.
- *
- * @note Locks mParent for reading. Locks this object for writing.
- *
- * @param fSetImageId Whether to reset the UUID contained in the image file to the UUID in the medium instance data (see SetIDs())
- * @param fSetParentId Whether to reset the parent UUID contained in the image file to the parent UUID in the medium instance data (see SetIDs())
- * @return
- */
-HRESULT Medium::queryInfo(bool fSetImageId, bool fSetParentId)
-{
-    Assert(!isWriteLockOnCurrentThread());
-    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
-
-    if (   m->state != MediumState_Created
-        && m->state != MediumState_Inaccessible
-        && m->state != MediumState_LockedRead)
-        return E_FAIL;
-
-    HRESULT rc = S_OK;
-
-    int vrc = VINF_SUCCESS;
-
-    /* check if a blocking queryInfo() call is in progress on some other thread,
-     * and wait for it to finish if so instead of querying data ourselves */
-    if (m->queryInfoRunning)
-    {
-        Assert(   m->state == MediumState_LockedRead
-               || m->state == MediumState_LockedWrite);
-
-        while (m->queryInfoRunning)
-        {
-            alock.release();
-            {
-                AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
-            }
-            alock.acquire();
-        }
 
         return S_OK;
     }
-
-    alock.release();
-    Assert(!isWriteLockOnCurrentThread());
-    Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
-    AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
-    alock.acquire();
 
     bool success = false;
@@ -5387 +5361 @@
     alock.release();
     if (uOpenFlags & (VD_OPEN_FLAGS_READONLY | VD_OPEN_FLAGS_SHAREABLE))
-        rc = lockRead(NULL, true /* fWithinQueryInfo */);
+        rc = LockRead(NULL);
     else
-        rc = lockWrite(NULL, true /* fWithinQueryInfo */);
+        rc = LockWrite(NULL);
     if (FAILED(rc)) return rc;
     alock.acquire();
@@ -5411 +5385 @@
     bool fRepairImageZeroParentUuid = false;
 
-    /* release the object lock before a lengthy operation */
+    /* release the object lock before a lengthy operation, and take the
+     * opportunity to have a media tree lock, too, which isn't held initially */
     m->queryInfoRunning = true;
     alock.release();
-    treeLock.release();
     Assert(!isWriteLockOnCurrentThread());
     Assert(!m->pVirtualBox->getMediaTreeLockHandle().isWriteLockOnCurrentThread());
+    AutoWriteLock treeLock(m->pVirtualBox->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
+    treeLock.release();
 
     /* Note that taking the queryInfoSem after leaving the object lock above
@@ -5422 +5398 @@
      * complete. This is unavoidable since the other order causes a lock order
      * violation: here it would be requesting the object lock (at the beginning
-     * of the method), then SemRW, and below the other way round. */
+     * of the method), then queryInfoSem, and below the other way round. */
     AutoWriteLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
+    alock.acquire();
 
     try
@@ -5562 +5539 @@
 
                     /* we set mParent & children() */
+                    alock.release();
                     treeLock.acquire();
 
@@ -5569 +5547 @@
 
                     treeLock.release();
+                    alock.acquire();
                 }
                 else
                 {
                     /* we access mParent */
+                    alock.release();
                     treeLock.acquire();
 
@@ -5596 +5576 @@
                                 location.c_str(),
                                 m->pVirtualBox->settingsFilePath().c_str());
+                        treeLock.release();
+                        alock.acquire();
                         throw S_OK;
 #endif /* 0 */
                     }
 
-                    AutoReadLock parentLock(m->pParent COMMA_LOCKVAL_SRC_POS);
-                    if (   !fRepairImageZeroParentUuid
-                        && m->pParent->getState() != MediumState_Inaccessible
-                        && m->pParent->getId() != parentId)
                     {
-                        /** @todo r=klaus this always refers to VirtualBox.xml as the medium registry, even for new VMs */
-                        lastAccessError = Utf8StrFmt(
-                                tr("Parent UUID {%RTuuid} of the medium '%s' does not match UUID {%RTuuid} of its parent medium stored in the media registry ('%s')"),
-                                &parentId, location.c_str(),
-                                m->pParent->getId().raw(),
-                                m->pVirtualBox->settingsFilePath().c_str());
-                        throw S_OK;
+                        AutoReadLock parentLock(m->pParent COMMA_LOCKVAL_SRC_POS);
+                        if (   !fRepairImageZeroParentUuid
+                            && m->pParent->getState() != MediumState_Inaccessible
+                            && m->pParent->getId() != parentId)
+                        {
+                            /** @todo r=klaus this always refers to VirtualBox.xml as the medium registry, even for new VMs */
+                            lastAccessError = Utf8StrFmt(
+                                    tr("Parent UUID {%RTuuid} of the medium '%s' does not match UUID {%RTuuid} of its parent medium stored in the media registry ('%s')"),
+                                    &parentId, location.c_str(),
+                                    m->pParent->getId().raw(),
+                                    m->pVirtualBox->settingsFilePath().c_str());
+                            parentLock.release();
+                            treeLock.release();
+                            alock.acquire();
+                            throw S_OK;
+                        }
                     }
 
@@ -5619 +5606 @@
 
                     treeLock.release();
+                    alock.acquire();
                 }
             }
@@ -5646 +5634 @@
     }
 
+    alock.release();
     treeLock.acquire();
     alock.acquire();
@@ -5730 +5719 @@
             rc = aRC;
         }
-
-        alock.acquire();
 
         rc = UnlockWrite(NULL);