10:29 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1224018
v1 here:
http://www.redhat.com/archives/libvir-list/2015-May/msg00865.html
This series adjusts the patches in order to first revert patches for
https://bugzilla.redhat.com/show_bug.cgi?id=1206521
and rework the patch to be specific to/for the disk backend to not update
the allocation and available pool values.
That removes the need to add 1 for the extended partition leaving only
the need to check for whether it's a refresh or create when zeroing out
the allocation value while processing the partition data.
If this is accepted, bz 1206521 will be moved back to a state to allow
qe to recheck the change.
John Ferlan (3):
Revert "storage: Don't duplicate efforts of backend driver"
storage: Don't adjust pool alloc/avail values for disk backend
storage: Fix problem with disk backend pool allocation calculation
src/storage/storage_backend_disk.c | 7 ++++++-
src/storage/storage_driver.c | 35 ++++++++++++++---------------------
2 files changed, 20 insertions(+), 22 deletions(-)
--
2.1.0
10:29 a.m.
New subject: [libvirt] [PATCH v2 1/3] Revert "storage: Don't duplicate efforts of backend driver"
This reverts commit 2ac0e647bdd33d93a374e7ef3eadf2a253c7bf79.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 30 ++++++------------------------
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ac4a74a..30e0969 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1634,7 +1634,6 @@ storageVolDeleteInternal(virStorageVolPtr obj,
{
size_t i;
int ret = -1;
- unsigned long long orig_pool_available, orig_pool_allocation;
if (!backend->deleteVol) {
virReportError(VIR_ERR_NO_SUPPORT,
@@ -1643,9 +1642,6 @@ storageVolDeleteInternal(virStorageVolPtr obj,
goto cleanup;
}
- orig_pool_available = pool->def->available;
- orig_pool_allocation = pool->def->allocation;
-
if (backend->deleteVol(obj->conn, pool, vol, flags) < 0)
goto cleanup;
@@ -1653,10 +1649,8 @@ storageVolDeleteInternal(virStorageVolPtr obj,
* in this module since the allocation/available weren't adjusted yet
*/
if (updateMeta) {
- if (orig_pool_allocation == pool->def->allocation)
- pool->def->allocation -= vol->target.allocation;
- if (orig_pool_available == pool->def->available)
- pool->def->available += vol->target.allocation;
+ pool->def->allocation -= vol->target.allocation;
+ pool->def->available += vol->target.allocation;
}
for (i = 0; i < pool->volumes.count; i++) {
@@ -1775,7 +1769,6 @@ storageVolCreateXML(virStoragePoolPtr obj,
virStorageVolDefPtr voldef = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
virStorageVolDefPtr buildvoldef = NULL;
- unsigned long long orig_pool_available, orig_pool_allocation;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL);
@@ -1823,9 +1816,6 @@ storageVolCreateXML(virStoragePoolPtr obj,
goto cleanup;
}
- orig_pool_available = pool->def->available;
- orig_pool_allocation = pool->def->allocation;
-
/* Wipe any key the user may have suggested, as volume creation
* will generate the canonical key. */
VIR_FREE(voldef->key);
@@ -1883,10 +1873,8 @@ storageVolCreateXML(virStoragePoolPtr obj,
goto cleanup;
/* Update pool metadata */
- if (orig_pool_allocation == pool->def->allocation)
- pool->def->allocation += buildvoldef->target.allocation;
- if (orig_pool_available == pool->def->available)
- pool->def->available -= buildvoldef->target.allocation;
+ pool->def->allocation += buildvoldef->target.allocation;
+ pool->def->available -= buildvoldef->target.allocation;
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
@@ -1914,7 +1902,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
virStorageVolDefPtr origvol = NULL, newvol = NULL;
virStorageVolPtr ret = NULL, volobj = NULL;
unsigned long long allocation;
- unsigned long long orig_pool_available, orig_pool_allocation;
int buildret;
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA |
@@ -2017,9 +2004,6 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
pool->volumes.count+1) < 0)
goto cleanup;
- orig_pool_available = pool->def->available;
- orig_pool_allocation = pool->def->allocation;
-
/* 'Define' the new volume so we get async progress reporting.
* Wipe any key the user may have suggested, as volume creation
* will generate the canonical key. */
@@ -2073,10 +2057,8 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
newvol = NULL;
/* Updating pool metadata */
- if (orig_pool_allocation == pool->def->allocation)
- pool->def->allocation += allocation;
- if (orig_pool_available == pool->def->available)
- pool->def->available -= allocation;
+ pool->def->allocation += allocation;
+ pool->def->available -= allocation;
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
--
2.1.0
10:29 a.m.
New subject: [libvirt] [PATCH v2 2/3] storage: Don't adjust pool alloc/avail values for disk backend
Commit id '2ac0e647' for https://bugzilla.redhat.com/show_bug.cgi?id=1206521
was meant to be a generic check for the CreateVol, CreateVolFrom, and
DeleteVol paths to check if the storage backend's changed the pool's view
of allocation or available values.
Unfortunately as it turns out this caused a side effect when the disk backend
created an extended partition there would be no actual storage removed from
the pool, thus the changes would not find any change in allocation or
available and incorrectly update the pool values using the size of the
extended partition. A subsequent refresh of the pool would reset the
values appropriately.
This patch modifies those checks in order to specifically not update the
pool allocation and available for only the disk backend rather than be
generic before and after checks.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_driver.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 30e0969..a9f66d1 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1646,11 +1646,14 @@ storageVolDeleteInternal(virStorageVolPtr obj,
goto cleanup;
/* Update pool metadata - don't update meta data from error paths
- * in this module since the allocation/available weren't adjusted yet
+ * in this module since the allocation/available weren't adjusted yet.
+ * Ignore the disk backend since it updates the pool values.
*/
if (updateMeta) {
- pool->def->allocation -= vol->target.allocation;
- pool->def->available += vol->target.allocation;
+ if (pool->def->type != VIR_STORAGE_POOL_DISK) {
+ pool->def->allocation -= vol->target.allocation;
+ pool->def->available += vol->target.allocation;
+ }
}
for (i = 0; i < pool->volumes.count; i++) {
@@ -1872,9 +1875,13 @@ storageVolCreateXML(virStoragePoolPtr obj,
backend->refreshVol(obj->conn, pool, voldef) < 0)
goto cleanup;
- /* Update pool metadata */
- pool->def->allocation += buildvoldef->target.allocation;
- pool->def->available -= buildvoldef->target.allocation;
+ /* Update pool metadata ignoring the disk backend since
+ * it updates the pool values.
+ */
+ if (pool->def->type != VIR_STORAGE_POOL_DISK) {
+ pool->def->allocation += buildvoldef->target.allocation;
+ pool->def->available -= buildvoldef->target.allocation;
+ }
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
@@ -2056,9 +2063,13 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj,
}
newvol = NULL;
- /* Updating pool metadata */
- pool->def->allocation += allocation;
- pool->def->available -= allocation;
+ /* Updating pool metadata ignoring the disk backend since
+ * it updates the pool values
+ */
+ if (pool->def->type != VIR_STORAGE_POOL_DISK) {
+ pool->def->allocation += allocation;
+ pool->def->available -= allocation;
+ }
VIR_INFO("Creating volume '%s' in storage pool '%s'",
volobj->name, pool->def->name);
--
2.1.0
10:29 a.m.
New subject: [libvirt] [PATCH v2 3/3] storage: Fix problem with disk backend pool allocation calculation
https://bugzilla.redhat.com/show_bug.cgi?id=1224018
The disk pool recalculates the pool allocation, capacity, and available
values each time through processing a newly created disk partition. This
created an issue with the allocation setting since the code used is shared
with the refresh path. Each path calls virStorageBackendDiskReadPartitions
which initializes the pool values and then processes the partition table
from the 'libvirt_parthelper' utility output with the only difference being
create passes a specific volume to be processed while refresh pass a NULL
indicating to process all volumes. That passed volume is check during the
virStorageBackendDiskMakeVol call to see if the current partition described
by the volume key already exists. If it exists, then no adjustments are
made to the allocation and the next entry in the output is checked.
For the create path this resulted in only the most recently created
partition size would be accounted for in the 'allocation' setting. This
patch thus checks whether the incoming volume is NULL before clearing
the pool allocation value.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend_disk.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
index 4dc63d7..c4bd6fe 100644
--- a/src/storage/storage_backend_disk.c
+++ b/src/storage/storage_backend_disk.c
@@ -309,7 +309,12 @@ virStorageBackendDiskReadPartitions(virStoragePoolObjPtr pool,
pool->def->source.devices[0].path,
NULL);
- pool->def->allocation = pool->def->capacity = pool->def->available
= 0;
+ /* If a volume is passed, virStorageBackendDiskMakeVol only updates the
+ * pool allocation for that single volume.
+ */
+ if (!vol)
+ pool->def->allocation = 0;
+ pool->def->capacity = pool->def->available = 0;
ret = virCommandRunNul(cmd,
6,
--
2.1.0
7:47 a.m.
On Tue, May 26, 2015 at 11:29:16AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1224018
v1 here:
http://www.redhat.com/archives/libvir-list/2015-May/msg00865.html
This series adjusts the patches in order to first revert patches for
https://bugzilla.redhat.com/show_bug.cgi?id=1206521
and rework the patch to be specific to/for the disk backend to not update
the allocation and available pool values.
That removes the need to add 1 for the extended partition leaving only
the need to check for whether it's a refresh or create when zeroing out
the allocation value while processing the partition data.
If this is accepted, bz 1206521 will be moved back to a state to allow
qe to recheck the change.
John Ferlan (3):
Revert "storage: Don't duplicate efforts of backend driver"
storage: Don't adjust pool alloc/avail values for disk backend
storage: Fix problem with disk backend pool allocation calculation
ACK series
Jan
3466
days inactive
3468
days old
4 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko