Changeset 33259 in vbox

Timestamp:

Oct 20, 2010 12:49:55 PM (14 years ago)

Author:

vboxsync

Message:

Storage/VBoxHDD+Main/Snapshot: Implement backward merging of snapshots, needed for live deletion of last remaining snapshot. Required a few minor changes to existing code in paths which so far were untested, to get the details right.

Location:

trunk/src/VBox

Files:

• Devices/Storage/VBoxHDD.cpp (modified) (6 diffs)
• Main/SnapshotImpl.cpp (modified) (6 diffs)

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

@@ -166 +166 @@
      * The same as pBase if only one image is used. */
     PVDIMAGE            pLast;
+
+    /** If a merge to one of the parents is running this may be non-NULL
+     * to indicate to what image the writes should be additionally relayed. */
+    PVDIMAGE            pImageRelay;
 
     /** Flags representing the modification state. */
@@ -1565 +1569 @@
  * write optimizations.
  */
-static int vdWriteHelper(PVBOXHDD pDisk, PVDIMAGE pImage, PVDIMAGE pImageParentOverride,
-                         uint64_t uOffset, const void *pvBuf, size_t cbWrite,
+static int vdWriteHelper(PVBOXHDD pDisk, PVDIMAGE pImage,
+                         PVDIMAGE pImageParentOverride, uint64_t uOffset,
+                         const void *pvBuf, size_t cbWrite,
                          bool fUpdateCache)
 {
@@ -5290 +5295 @@
         {
             /*
-             * We may need to update the parent uuid of the child coming after the
-             * last image to be merged. We have to reopen it read/write.
+             * We may need to update the parent uuid of the child coming after
+             * the last image to be merged. We have to reopen it read/write.
              *
-             * This is done before we do the actual merge to prevent an incosistent
-             * chain if the mode change fails for some reason.
+             * This is done before we do the actual merge to prevent an
+             * inconsistent chain if the mode change fails for some reason.
              */
             if (pImageFrom->pNext)
@@ -5316 +5321 @@
                         break;
                 }
+
+                rc2 = vdThreadFinishWrite(pDisk);
+                AssertRC(rc2);
+                fLockWrite = false;
+            }
+
+            /* If the merge is from the last image we have to relay all writes
+             * to the merge destination as well, so that concurrent writes
+             * (in case of a live merge) are handled correctly. */
+            if (!pImageFrom->pNext)
+            {
+                /* Take the write lock. */
+                rc2 = vdThreadStartWrite(pDisk);
+                AssertRC(rc2);
+                fLockWrite = true;
+
+                pDisk->pImageRelay = pImageTo;
 
                 rc2 = vdThreadFinishWrite(pDisk);
@@ -5380 +5402 @@
                 }
             } while (uOffset < cbSize);
+
+            /* In case we set up a "write proxy" image above we must clear
+             * this again now to prevent stray writes. Failure or not. */
+            if (!pImageFrom->pNext)
+            {
+                /* Take the write lock. */
+                rc2 = vdThreadStartWrite(pDisk);
+                AssertRC(rc2);
+                fLockWrite = true;
+
+                pDisk->pImageRelay = NULL;
+
+                rc2 = vdThreadFinishWrite(pDisk);
+                AssertRC(rc2);
+                fLockWrite = false;
+            }
         }
 
@@ -6475 +6513 @@
         rc = vdWriteHelper(pDisk, pImage, NULL, uOffset, pvBuf, cbWrite,
                            true /* fUpdateCache */);
+        if (RT_FAILURE(rc))
+            break;
+
+        /* If there is a merge (in the direction towards a parent) running
+         * concurrently then we have to also "relay" the write to this parent,
+         * as the merge position might be already past the position where
+         * this write is going. The "context" of the write can come from the
+         * natural chain, since merging either already did or will take care
+         * of the "other" content which is might be needed to fill the block
+         * to a full allocation size. The cache doesn't need to be touched
+         * as this write is covered by the previous one. */
+        if (RT_UNLIKELY(pDisk->pImageRelay))
+            rc = vdWriteHelper(pDisk, pDisk->pImageRelay, NULL, uOffset,
+                               pvBuf, cbWrite, false /* fUpdateCache */);
     } while (0);
 

trunk/src/VBox/Main/SnapshotImpl.cpp

@@ -2579 +2579 @@
                 if (it->mfNeedsOnlineMerge)
                 {
-/// @todo VBoxHDD cannot handle backward merges where source==active disk yet
-                    if (!it->mfMergeForward && it->mChildrenToReparent.size() == 0)
-                        throw setError(E_NOTIMPL,
-                                       tr("Snapshot '%s' of the machine '%ls' cannot be deleted while a VM is running, as this case is not implemented yet. You can delete the snapshot when the VM is powered off"),
-                                       aTask.pSnapshot->getName().c_str(),
-                                       mUserData->s.strName.c_str());
-
                     // online medium merge, in the direction decided earlier
                     rc = onlineMergeMedium(it->mpOnlineMediumAttachment,
@@ -2682 +2675 @@
                     childSnapshotId = pChildSnapshot->getId();
                 }
-                pAtt = findAttachment(pMachine->mMediaData->mAttachments, it->mpSource);
-                // If no attachment is found do not change anything. The source
-                // medium might not have been attached to the snapshot.
-                if (pAtt)
+                // If this is an online deletion the attachment was updated
+                // already as it is required to continue execution immediately.
+                // Needs a bit of special treatment due to this difference.
+                if (it->mfNeedsOnlineMerge)
                 {
-                    AutoWriteLock attLock(pAtt COMMA_LOCKVAL_SRC_POS);
-                    pAtt->updateMedium(it->mpTarget);
                     it->mpTarget->addBackReference(pMachine->mData->mUuid, childSnapshotId);
+                }
+                else
+                {
+                    pAtt = findAttachment(pMachine->mMediaData->mAttachments, it->mpSource);
+                    // If no attachment is found do not change anything. Maybe
+                    // the source medium was not attached to the snapshot.
+                    if (pAtt)
+                    {
+                        AutoWriteLock attLock(pAtt COMMA_LOCKVAL_SRC_POS);
+                        pAtt->updateMedium(it->mpTarget);
+                        it->mpTarget->addBackReference(pMachine->mData->mUuid, childSnapshotId);
+                    }
                 }
             }
@@ -3260 +3263 @@
     AutoWriteLock treeLock(mParent->getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
 
+    // Declare this here to make sure the object does not get uninitialized
+    // before this method completes. Would normally happen as halfway through
+    // we delete the last reference to the no longer existing medium object.
+    ComObjPtr<Medium> targetChild;
+
     if (aMergeForward)
     {
@@ -3281 +3289 @@
     {
         Assert(pTarget->getChildren().size() == 1);
-        Medium *targetChild = pTarget->getChildren().front();
+        targetChild = pTarget->getChildren().front();
 
         // disconnect the deleted branch at the elder end
@@ -3320 +3328 @@
     /* unregister and uninitialize all hard disks removed by the merge */
     MediumLockList *pMediumLockList = NULL;
-    rc = mData->mSession.mLockedMedia.Get(static_cast<MediumAttachment *>(aMediumAttachment),
-                                          pMediumLockList);
+    MediumAttachment *pMediumAttachment = static_cast<MediumAttachment *>(aMediumAttachment);
+    rc = mData->mSession.mLockedMedia.Get(pMediumAttachment, pMediumLockList);
     const ComObjPtr<Medium> &pLast = aMergeForward ? pTarget : pSource;
     AssertReturn(SUCCEEDED(rc) && pMediumLockList, E_FAIL);
@@ -3396 +3404 @@
     }
 
+    /* If this is a backwards merge update the medium associated with the
+     * attachment, as the previously associated one is now deleted. */
+    if (!aMergeForward)
+    {
+        AutoWriteLock attLock(pMediumAttachment COMMA_LOCKVAL_SRC_POS);
+        pMediumAttachment->updateMedium(pTarget);
+    }
 
     return S_OK;