On Mon, Jun 22, 2015 at 05:07:26PM +0530, Prerna Saxena wrote:
From: Prerna Saxena <prerna(a)linux.vnet.ibm.com>
Date: Thu, 18 Jun 2015 05:05:09 -0500
Libvirt periodically refreshes all volumes in a storage pool, including
the volumes being cloned.
While cloning a storage volume from parent, we drop pool locks. Subsequent
volume refresh sometimes changes allocation for an ongoing copy, and leads
to corrupt images.
Fix: Introduce a shadow volume that isolates the volume object under refresh
from the base which has a copy ongoing.
Signed-off-by: Prerna Saxena <prerna(a)linux.vnet.ibm.com>
---
src/storage/storage_driver.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 57060ab..4980546 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1898,7 +1898,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
{
virStoragePoolObjPtr pool, origpool = NULL;
virStorageBackendPtr backend;
- virStorageVolDefPtr origvol = NULL, newvol = NULL;
+ virStorageVolDefPtr origvol = NULL, newvol = NULL, shadowvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
unsigned long long allocation;
int buildret;
@@ -2010,6 +2010,17 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
if (backend->createVol(obj->conn, pool, newvol) < 0)
goto cleanup;
+ /* Make a shallow copy of the 'defined' volume definition, since the
+ * original allocation value will change as the user polls 'info',
+ * but we only need the initial requested values
+ */
+ if (VIR_ALLOC(shadowvol) < 0) {
+ newvol = NULL;
newvol has not been added to pool->volumes.objs yet, so we should free
it on the cleanup path. shadowvol should also be VIR_FREE'd.
+ goto cleanup;
+ }
+
+ memcpy(shadowvol, newvol, sizeof(*newvol));
+
pool->volumes.objs[pool->volumes.count++] = newvol;
volobj = virGetStorageVol(obj->conn, pool->def->name, newvol->name,
newvol->key, NULL, NULL);
@@ -2029,7 +2040,7 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStoragePoolObjUnlock(origpool);
}
- buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags);
+ buildret = backend->buildVolFrom(obj->conn, pool, shadowvol, origvol, flags);
A few lines below, there is one more usage of newvol after the pool has
been unlocked: newvol->target.allocation
If the parallel volume refresh happened when the volume was not fully
allocated, this might not contain the intended allocation.
Using shadowvol->target.allocation will behave the same regardless of a
parallel refresh (even though the buildVolFrom function might not honor
the allocation exactly).
Jan