Hi Jan, Thanks for the review comments. On Tuesday 23 June 2015 06:21 PM, Ján Tomko wrote: > On Mon, Jun 22, 2015 at 05:07:26PM +0530, Prerna Saxena wrote: >> From: Prerna Saxena <prerna@xxxxxxxxxxxxxxxxxx> >> 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@xxxxxxxxxxxxxxxxxx> >> --- >> 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. Thanks, I'd missed this. Will be addressed in subsequent patch. >> + 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). > > Right. The buildVolFrom() call completes an fsync and then returns. In my experimental runs, a parallel refresh would always happen by the time I got to this point; so this had missed me. But ofcourse we can never say that for sure. Will fix in the next patch. -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list