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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list