[libvirt] [PATCH v2] storage: Fix regression cloning volume into a logical pool

[John Ferlan]

On 05/11/2016 07:26 AM, Ján Tomko wrote:
> On Tue, May 10, 2016 at 01:28:48PM -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1318993
>>
>> Commit id 'dd519a294' caused a regression cloning a volume into a
>> logical pool by removing just the 'allocation' adjustment during
>> storageVolCreateXMLFrom. Combined with the change to not require the
>> new volume input XML to have a capacity listed (commit id 'e3f1d2a8')
>> left the possibility that a copy from a larger volume into a smaller
>> volume would create a thin/sparse logical volume. If a thin lv becomes
>> fully populated, then LVM will set the partition 'inactive' and the
>> subsequent fdatasync() would fail.
>>
>> In order to fix this in a backend agnostic manner, only adjust the
>> new capacity if not provided. Likewise, if the new allocation is not
>> provided, then use the capacity value as we document that if omitted,
>> the volume will be fully allocated at time of creation. In order to
>> ascertain that <allocation> == 0 was because it was not provided and
>> not because it was provided as a 0 value, add a has_allocation flag
>> which will be checked during in order to honor the possibility
>> that <capacity> was provided as some value and <allocation> was
>> provided as 0.
>>
>> For a logical backend, that creation time is 'createVol', while for a
>> file backend, creation doesn't set the size, but the 'createRaw' called
>> during buildVolFrom will decide whether the file is sparse or not based
>> on the provided capacity and allocation value.
>>
>> For volume clones that provide different allocation and capacity values
>> to allow for sparse files, there is no change.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> v1:
>> http://www.redhat.com/archives/libvir-list/2016-April/msg01994.html
>>
>>
>>  src/conf/storage_conf.c      |  1 +
>>  src/storage/storage_driver.c | 18 +++++++++++++++---
>>  src/util/virstoragefile.c    |  1 +
>>  src/util/virstoragefile.h    |  2 ++
>>  4 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
>> index 0b91956..6932195 100644
>> --- a/src/conf/storage_conf.c
>> +++ b/src/conf/storage_conf.c
>> @@ -1370,6 +1370,7 @@ virStorageVolDefParseXML(virStoragePoolDefPtr pool,
>>          unit = virXPathString("string(./allocation/@unit)", ctxt);
>>          if (virStorageSize(unit, allocation, &ret->target.allocation) < 0)
>>              goto error;
>> +        ret->target.has_allocation = true;
>>      } else {
>>          ret->target.allocation = ret->target.capacity;
>>      }
>> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
>> index 1d42f24..0afe522 100644
>> --- a/src/storage/storage_driver.c
>> +++ b/src/storage/storage_driver.c
>> @@ -2034,11 +2034,23 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
>>          goto cleanup;
>>      }
>>  
>> -    /* Use the original volume's capacity in case the new capacity
>> -     * is less than that, or it was omitted */
>> -    if (newvol->target.capacity < origvol->target.capacity)
>> +    /* Use the original volume's capacity if the new capacity was omitted.
>> +     * If the provided newvol capacity is less than original, we cannot just
>> +     * use the original since the subsequent createVol has "competing needs"
>> +     * for backends. A logical volume backend could then create a thin lv
>> +     * that has different characteristics when copying from the original
>> +     * volume than perhaps a raw disk file. */
>> +    if (newvol->target.capacity == 0)
>>          newvol->target.capacity = origvol->target.capacity;
>>  
> 
> I think this hunk can be dropped from the patch.
> 
> With the new code, we would clone a volume which would be missing data
> if someone provided a capacity lower than the original.
> 

Sure - understood... Perhaps I'm over thinking the what if someone
supplies a lower value.  I'll restore former code before pushing.

>> +    /* If the new allocation is 0 and the allocation was not provided in
>> +     * the XML, then use capacity as it's specifically documented "If
>> +     * omitted when creating a volume, the volume will be fully allocated
>> +     * at time of creation.". This is especially important for logical
>> +     * volume creation. */
>> +    if (newvol->target.allocation == 0 && !newvol->target.has_allocation)
>> +        newvol->target.allocation = newvol->target.capacity;
>> +
> 
> We should only decide based on the value of has_allocation here.
> As is, the condition would only match the case where neither capacity
> nor allocation is specified. But if the user specified capacity,
> the parser will set has_allocation to false and allocation to the value
> of capacity.
> 

yep -

Thanks for the review -

John