[libvirt] [PATCH 1/2] Storage: Introduce shadow vol for refresh while the main vol builds.

[Ján Tomko]

On Mon, Jun 22, 2015 at 05:07:26PM +0530, Prerna Saxena wrote:
> From: Prerna Saxena <prerna at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150623/dd545de1/attachment-0001.sig>