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

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. 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(a)redhat.com&gt; --- NOTE: Related bz's to previous changes: https://bugzilla.redhat.com/show_bug.cgi?id=1130739 https://bugzilla.redhat.com/show_bug.cgi?id=1298065 I have tested using 'virsh vol-clone' and 'virt-clone' with and without this patch applied. Both tests have the same results for the 'qemu-img info' on the cloned file. src/storage/storage_driver.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index fcc0991..4415a4c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2034,11 +2034,22 @@ 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; + /* If the new allocation is 0, 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.allocation = newvol->target.capacity; + if (!backend->buildVolFrom) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("storage pool does not support"