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.
> + 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list