Changeset 35162 in vbox

Timestamp:

Dec 15, 2010 7:39:54 PM (14 years ago)

Author:

vboxsync

Message:

BlkCache: Fix possible crash during suspend of the VM. The cache must be suspended before making the disk readonly to make sure all I/O completed

Location:

trunk

Files:

• include/VBox/pdmblkcache.h (modified) (1 diff)
• src/VBox/Devices/Storage/DrvVD.cpp (modified) (2 diffs)
• src/VBox/VMM/PDMBlkCache.cpp (modified) (11 diffs)
• src/VBox/VMM/PDMBlkCacheInternal.h (modified) (1 diff)

trunk/include/VBox/pdmblkcache.h

@@ -310 +310 @@
  */
 VMMR3DECL(void) PDMR3BlkCacheIoXferComplete(PPDMBLKCACHE pBlkCache, PPDMBLKCACHEIOXFER hIoXfer, int rcIoXfer);
+
+/**
+ * Suspends the block cache. The cache waits until all I/O transfers completed
+ * and stops to enqueue new requests after the call returned but will not accept
+ * reads, write or flushes either.
+ *
+ * @returns VBox status code.
+ * @param   pBlkCache       The cache instance.
+ */
+VMMR3DECL(int) PDMR3BlkCacheSuspend(PPDMBLKCACHE pBlkCache);
+
+/**
+ * Resumes operation of the block cache.
+ *
+ * @returns VBox status code.
+ * @param   pBlkCache       The cache instance.
+ */
+VMMR3DECL(int) PDMR3BlkCacheResume(PPDMBLKCACHE pBlkCache);
+
 /** @} */
 

trunk/src/VBox/Devices/Storage/DrvVD.cpp

@@ -1897 +1897 @@
     LogFlowFunc(("\n"));
     PVBOXDISK pThis = PDMINS_2_DATA(pDrvIns, PVBOXDISK);
+
     drvvdSetWritable(pThis);
     pThis->fErrorUseRuntime = true;
+
+    if (pThis->pBlkCache)
+    {
+        int rc = PDMR3BlkCacheResume(pThis->pBlkCache);
+        AssertRC(rc);
+    }
 }
 
@@ -1920 +1927 @@
     LogFlowFunc(("\n"));
     PVBOXDISK pThis = PDMINS_2_DATA(pDrvIns, PVBOXDISK);
+
+    if (pThis->pBlkCache)
+    {
+        int rc = PDMR3BlkCacheSuspend(pThis->pBlkCache);
+        AssertRC(rc);
+    }
+
     drvvdSetReadonly(pThis);
 }

trunk/src/VBox/VMM/PDMBlkCache.cpp

@@ -647 +647 @@
 {
     uint32_t cbCommitted = 0;
+
+    /* Return if the cache was suspended. */
+    if (pBlkCache->fSuspended)
+        return;
+
     RTSemRWRequestWrite(pBlkCache->SemRWEntries, RT_INDEFINITE_WAIT);
 
@@ -683 +688 @@
     AssertMsg(pBlkCache->pCache->cbDirty >= cbCommitted,
               ("Number of committed bytes exceeds number of dirty bytes\n"));
-    ASMAtomicSubU32(&pBlkCache->pCache->cbDirty, cbCommitted);
+    uint32_t cbDirtyOld = ASMAtomicSubU32(&pBlkCache->pCache->cbDirty, cbCommitted);
+
+    /* Reset the commit timer if we don't have any dirty bits. */
+    if (   !(cbDirtyOld - cbCommitted)
+        && pBlkCache->pCache->u32CommitTimeoutMs != 0)
+        TMTimerStop(pBlkCache->pCache->pTimerCommit);
 }
 
@@ -752 +762 @@
         /* Prevent committing if the VM was suspended. */
         if (RT_LIKELY(!ASMAtomicReadBool(&pCache->fIoErrorVmSuspended)))
-            fDirtyBytesExceeded = (cbDirty >= pCache->cbCommitDirtyThreshold);
+            fDirtyBytesExceeded = (cbDirty + pEntry->cbData >= pCache->cbCommitDirtyThreshold);
+        else if (!cbDirty && pCache->u32CommitTimeoutMs > 0)
+        {
+            /* Arm the commit timer. */
+            TMTimerSetMillies(pCache->pTimerCommit, pCache->u32CommitTimeoutMs);
+        }
     }
 
@@ -788 +803 @@
         pdmBlkCacheCommitDirtyEntries(pCache);
 
-    TMTimerSetMillies(pTimer, pCache->u32CommitTimeoutMs);
     LogFlowFunc(("Entries committed, going to sleep\n"));
 }
@@ -1191 +1205 @@
             && pBlkCache->pszId)
         {
+            pBlkCache->fSuspended = false;
             pBlkCache->pCache = pBlkCacheGlobal;
             RTListInit(&pBlkCache->ListDirtyNotCommitted);
@@ -1203 +1218 @@
                     if (pBlkCache->pTree)
                     {
-                        /* Arm the timer if this is the first endpoint. */
-                        if (   !pBlkCacheGlobal->cRefs
-                            && pBlkCacheGlobal->u32CommitTimeoutMs > 0)
-                            rc = TMTimerSetMillies(pBlkCacheGlobal->pTimerCommit, pBlkCacheGlobal->u32CommitTimeoutMs);
-
-                        if (RT_SUCCESS(rc))
-                        {
 #ifdef VBOX_WITH_STATISTICS
-                            STAMR3RegisterF(pBlkCacheGlobal->pVM, &pBlkCache->StatWriteDeferred,
-                                           STAMTYPE_COUNTER, STAMVISIBILITY_ALWAYS,
-                                           STAMUNIT_COUNT, "Number of deferred writes",
-                                           "/PDM/BlkCache/%s/Cache/DeferredWrites", pBlkCache->pszId);
+                        STAMR3RegisterF(pBlkCacheGlobal->pVM, &pBlkCache->StatWriteDeferred,
+                                        STAMTYPE_COUNTER, STAMVISIBILITY_ALWAYS,
+                                        STAMUNIT_COUNT, "Number of deferred writes",
+                                        "/PDM/BlkCache/%s/Cache/DeferredWrites", pBlkCache->pszId);
 #endif
 
-                            /* Add to the list of users. */
-                            pBlkCacheGlobal->cRefs++;
-                            RTListAppend(&pBlkCacheGlobal->ListUsers, &pBlkCache->NodeCacheUser);
-                            pdmBlkCacheLockLeave(pBlkCacheGlobal);
-
-                            *ppBlkCache = pBlkCache;
-                            LogFlowFunc(("returns success\n"));
-                            return VINF_SUCCESS;
-                        }
+                        /* Add to the list of users. */
+                        pBlkCacheGlobal->cRefs++;
+                        RTListAppend(&pBlkCacheGlobal->ListUsers, &pBlkCache->NodeCacheUser);
+                        pdmBlkCacheLockLeave(pBlkCacheGlobal);
+
+                        *ppBlkCache = pBlkCache;
+                        LogFlowFunc(("returns success\n"));
+                        return VINF_SUCCESS;
                     }
                     else
@@ -1417 +1424 @@
     pCache->cRefs--;
     RTListNodeRemove(&pBlkCache->NodeCacheUser);
-
-    if (   !pCache->cRefs
-        && pCache->u32CommitTimeoutMs > 0)
-        TMTimerStop(pCache->pTimerCommit);
 
     pdmBlkCacheLockLeave(pCache);
@@ -1934 +1937 @@
     LogFlowFunc((": pBlkCache=%#p{%s} off=%llu pcSgBuf=%#p cbRead=%u pvUser=%#p\n",
                  pBlkCache, pBlkCache->pszId, off, pcSgBuf, cbRead, pvUser));
+
+    AssertPtrReturn(pBlkCache, VERR_INVALID_POINTER);
+    AssertReturn(!pBlkCache->fSuspended, VERR_INVALID_STATE);
 
     RTSGBUF SgBuf;
@@ -2149 +2155 @@
     LogFlowFunc((": pBlkCache=%#p{%s} off=%llu pcSgBuf=%#p cbWrite=%u pvUser=%#p\n",
                  pBlkCache, pBlkCache->pszId, off, pcSgBuf, cbWrite, pvUser));
+
+    AssertPtrReturn(pBlkCache, VERR_INVALID_POINTER);
+    AssertReturn(!pBlkCache->fSuspended, VERR_INVALID_STATE);
 
     RTSGBUF SgBuf;
@@ -2378 +2387 @@
     LogFlowFunc((": pBlkCache=%#p{%s}\n", pBlkCache, pBlkCache->pszId));
 
+    AssertPtrReturn(pBlkCache, VERR_INVALID_POINTER);
+    AssertReturn(!pBlkCache->fSuspended, VERR_INVALID_STATE);
+
     /* Commit dirty entries in the cache. */
     pdmBlkCacheCommit(pBlkCache);
@@ -2539 +2551 @@
 }
 
+/**
+ * Callback for the AVL do with all routine. Waits for a cachen entry to finish any pending I/O.
+ *
+ * @returns IPRT status code.
+ * @param    pNode     The node to destroy.
+ * @param    pvUser    Opaque user data.
+ */
+static int pdmBlkCacheEntryQuiesce(PAVLRU64NODECORE pNode, void *pvUser)
+{
+    PPDMBLKCACHEENTRY  pEntry = (PPDMBLKCACHEENTRY)pNode;
+    PPDMBLKCACHE pBlkCache = pEntry->pBlkCache;
+
+    while (ASMAtomicReadU32(&pEntry->fFlags) & PDMBLKCACHE_ENTRY_IO_IN_PROGRESS)
+    {
+        /* Leave the locks to let the I/O thread make progress but reference the entry to prevent eviction. */
+        pdmBlkCacheEntryRef(pEntry);
+        RTSemRWReleaseWrite(pBlkCache->SemRWEntries);
+
+        RTThreadSleep(1);
+
+        /* Re-enter all locks and drop the reference. */
+        RTSemRWRequestWrite(pBlkCache->SemRWEntries, RT_INDEFINITE_WAIT);
+        pdmBlkCacheEntryRelease(pEntry);
+    }
+
+    AssertMsg(!(pEntry->fFlags & PDMBLKCACHE_ENTRY_IO_IN_PROGRESS),
+                ("Entry is dirty and/or still in progress fFlags=%#x\n", pEntry->fFlags));
+
+    return VINF_SUCCESS;
+}
+
+VMMR3DECL(int) PDMR3BlkCacheSuspend(PPDMBLKCACHE pBlkCache)
+{
+    int rc = VINF_SUCCESS;
+    LogFlowFunc(("pBlkCache=%#p\n", pBlkCache));
+
+    AssertPtrReturn(pBlkCache, VERR_INVALID_POINTER);
+
+    ASMAtomicXchgBool(&pBlkCache->fSuspended, true);
+
+    /* Wait for all I/O to complete. */
+    RTSemRWRequestWrite(pBlkCache->SemRWEntries, RT_INDEFINITE_WAIT);
+    rc = RTAvlrU64DoWithAll(pBlkCache->pTree, true, pdmBlkCacheEntryQuiesce, NULL);
+    AssertRC(rc);
+    RTSemRWReleaseWrite(pBlkCache->SemRWEntries);
+
+    return rc;
+}
+
+VMMR3DECL(int) PDMR3BlkCacheResume(PPDMBLKCACHE pBlkCache)
+{
+    LogFlowFunc(("pBlkCache=%#p\n", pBlkCache));
+
+    AssertPtrReturn(pBlkCache, VERR_INVALID_POINTER);
+
+    ASMAtomicXchgBool(&pBlkCache->fSuspended, false);
+
+    return VINF_SUCCESS;
+}
+

trunk/src/VBox/VMM/PDMBlkCacheInternal.h

@@ -247 +247 @@
     STAMCOUNTER StatWriteDeferred;
 #endif
+
+    /** Flag whether the cache was suspended. */
+    volatile bool                 fSuspended;
+
 } PDMBLKCACHE, *PPDMBLKCACHE;
 #ifdef VBOX_WITH_STATISTICS