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

[Prerna Saxena]

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 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.

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