7:35 a.m.
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>
---
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"
--
2.5.5
8:51 a.m.
On 04/29/2016 08:35 AM, 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.
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>
---
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"
ACK
- Cole
9:02 a.m.
On 04/29/2016 08:35 AM, 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.
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>
---
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(-)
Jan pointed out something in the bz which I failed to read before
posting this patch (hazards of ignoring the bz email volume <sigh>).
There's no way (yet) to distinguish whether the XML has provided a "0"
allocation or not. If someone provides a 0 <allocation>, then it should
be honored and thus the code here which just copies capacity isn't quite
right.
I'm working through a possible solution for that instance now...
John
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"
10:34 a.m.
New subject: [libvirt] [PATCH 2/1] storage: Merge into 1/1
As it turns out, trusting that <allocation> being 0 means it wasn't
provided isn't such a good idea.
If someone provided a <capacity> of 10 and <allocation> of 0, then
we need to honor it.
So this patch which I'll merge into the previous patch will track
when the XML is read if the allocation was provided or not. That
way we can determine in this code that if allocation = 0, then
"overwrite" with the capacity value if it wasn't provided.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 1 +
src/storage/storage_driver.c | 11 ++++++-----
src/util/virstoragefile.c | 1 +
src/util/virstoragefile.h | 2 ++
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index daf8f99..a4708f6 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1364,6 +1364,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 4415a4c..1ebee5b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2043,11 +2043,12 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
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)
+ /* 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) {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 05ac254..73151bd 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 b98fe25..6edc789 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
6:20 a.m.
New subject: [libvirt] [PATCH 2/1] storage: Merge into 1/1
On 04/29/2016 11:34 AM, John Ferlan wrote:
As it turns out, trusting that <allocation> being 0 means it
wasn't
provided isn't such a good idea.
If someone provided a <capacity> of 10 and <allocation> of 0, then
we need to honor it.
So this patch which I'll merge into the previous patch will track
when the XML is read if the allocation was provided or not. That
way we can determine in this code that if allocation = 0, then
"overwrite" with the capacity value if it wasn't provided.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/storage_conf.c | 1 +
src/storage/storage_driver.c | 11 ++++++-----
src/util/virstoragefile.c | 1 +
src/util/virstoragefile.h | 2 ++
4 files changed, 10 insertions(+), 5 deletions(-)
ping - any thoughts regarding squashing this in as well?
Tks -
John
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index daf8f99..a4708f6 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -1364,6 +1364,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 4415a4c..1ebee5b 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -2043,11 +2043,12 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
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)
+ /* 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) {
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index 05ac254..73151bd 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 b98fe25..6edc789 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;
8:40 a.m.
New subject: [libvirt] [PATCH 2/1] storage: Merge into 1/1
On Tue, May 10, 2016 at 07:20:09 -0400, John Ferlan wrote:
On 04/29/2016 11:34 AM, John Ferlan wrote:
> As it turns out, trusting that <allocation> being 0 means it wasn't
> provided isn't such a good idea.
>
> If someone provided a <capacity> of 10 and <allocation> of 0, then
> we need to honor it.
>
> So this patch which I'll merge into the previous patch will track
> when the XML is read if the allocation was provided or not. That
> way we can determine in this code that if allocation = 0, then
> "overwrite" with the capacity value if it wasn't provided.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/conf/storage_conf.c | 1 +
> src/storage/storage_driver.c | 11 ++++++-----
> src/util/virstoragefile.c | 1 +
> src/util/virstoragefile.h | 2 ++
> 4 files changed, 10 insertions(+), 5 deletions(-)
>
ping - any thoughts regarding squashing this in as well?
I think it will be better if you repost the patch in the form you are
going to push it.
Peter
3118
days inactive
3129
days old
5 comments
3 participants
participants (3)
-
Cole Robinson -
John Ferlan -
Peter Krempa