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

[John Ferlan]

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;
 
+    /* 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;
+
     if (!backend->buildVolFrom) {
         virReportError(VIR_ERR_NO_SUPPORT,
                        "%s", _("storage pool does not support"
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 4f44e05..d4e61ca 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -1835,6 +1835,7 @@ virStorageSourceCopy(const virStorageSource *src,
     ret->format = src->format;
     ret->capacity = src->capacity;
     ret->allocation = src->allocation;
+    ret->has_allocation = src->has_allocation;
     ret->physical = src->physical;
     ret->readonly = src->readonly;
     ret->shared = src->shared;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 17e1277..b88e715 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -260,6 +260,8 @@ struct _virStorageSource {
     unsigned long long capacity; /* in bytes, 0 if unknown */
     unsigned long long allocation; /* in bytes, 0 if unknown */
     unsigned long long physical; /* in bytes, 0 if unknown */
+    bool has_allocation; /* Set to true when provided in XML */
+
     size_t nseclabels;
     virSecurityDeviceLabelDefPtr *seclabels;
 
-- 
2.5.5