[libvirt] [PATCH 0/3] Fix some storage issues
Patch 1 & 3 are bz based, while Patch 2 was determined while working on Patch 3. Details in each commit message John Ferlan (3): storage: Fix issues in storageVolResize storage: Need to update freeExtent at delete primary partition storage: Don't duplicate efforts of backend driver src/storage/storage_backend_disk.c | 9 +++-- src/storage/storage_driver.c | 67 ++++++++++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 17 deletions(-) -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1073305 When creating a volume in a pool, the creation allows the 'capacity' value to be larger than the available space in the pool. As long as the 'allocation' value will fit in the space, the volume will be created. However, resizing the volume checks were made with the new absolute capacity value against existing capacity + the available space without regard for whether the new absolute capacity was actually allocating space or not. For example, a pool with 75G of available space creates a volume of 10G using a capacity of 100G and allocation of 10G will succeed; however, if the allocation used a capacity of 10G instead and then tried to resize the allocation to 100G the code would fail to allow the backend to try the resize. Furthermore, when updating the pool "available" and "allocation" values, the resize code would just "blindly" adjust them regardless of whether space was "allocated" or just "capacity" was being adjusted. This left a scenario whereby a resize to 100G would fail; however, a resize to 50G followed by one to 100G would both succeed. Again, neither was adjusting the allocation value, just the "capacity" value. This patch adds more logic to the resize code to understand whether the new capacity value is actually "allocating" space as well and whether it shrinking or expanding. Since unsigned arithmatic is involved, the possibility that we adjust the pool size values incorrectly is probable. This patch also ensures that updates to the pool values only occur if we actually performed the allocation. NB: The storageVolDelete, storageVolCreateXML, and storageVolCreateXMLFrom each only updates the pool allocation/availability values by the target volume allocation value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b95506f..8d91879 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2139,7 +2139,7 @@ storageVolResize(virStorageVolPtr obj, virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; - unsigned long long abs_capacity; + unsigned long long abs_capacity, delta; int ret = -1; virCheckFlags(VIR_STORAGE_VOL_RESIZE_ALLOCATE | @@ -2184,13 +2184,24 @@ storageVolResize(virStorageVolPtr obj, !(flags & VIR_STORAGE_VOL_RESIZE_SHRINK)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("Can't shrink capacity below current " - "capacity with shrink flag explicitly specified")); + "capacity unless shrink flag explicitly specified")); goto cleanup; } - if (abs_capacity > vol->target.capacity + pool->def->available) { + if (flags & VIR_STORAGE_VOL_RESIZE_SHRINK) + delta = vol->target.allocation - abs_capacity; + else + delta = abs_capacity - vol->target.allocation; + + /* If the operation is going to increase the allocation value and not + * just the capacity value, then let's make sure there's enough space + * in the pool in order to perform that operation + */ + if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE && + !(flags & VIR_STORAGE_VOL_RESIZE_SHRINK) && + delta > pool->def->available) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Not enough space left on storage pool")); + _("Not enough space left in storage pool")); goto cleanup; } @@ -2205,12 +2216,22 @@ storageVolResize(virStorageVolPtr obj, goto cleanup; vol->target.capacity = abs_capacity; - if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) + /* Only update the allocation and pool values if we actually did the + * allocation; otherwise, this is akin to a create operation with a + * capacity value different and potentially much larger than available + */ + if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) { vol->target.allocation = abs_capacity; - /* Update pool metadata */ - pool->def->allocation += (abs_capacity - vol->target.capacity); - pool->def->available -= (abs_capacity - vol->target.capacity); + /* Update pool metadata */ + if (flags & VIR_STORAGE_VOL_RESIZE_SHRINK) { + pool->def->allocation -= delta; + pool->def->available += delta; + } else { + pool->def->allocation += delta; + pool->def->available -= delta; + } + } ret = 0; -- 2.1.0
Commit id '471e1c4e' only considered updating the pool if the extended partition was removed. As it turns out removing a primary partition would also need to update the freeExtent list otherwise the following sequence would fail (assuming a "fresh" disk pool for /dev/sde of 500M): $ virsh pool-info disk-pool ... Capacity: 509.88 MiB Allocation: 0.00 B Available: 509.84 MiB $ virsh vol-create-as disk-pool sde1 --capacity 300M $ virsh vol-delete --pool disk-pool sde1 $ virsh vol-create-as disk-pool sde1 --capacity 300M error: Failed to create vol sde1 error: internal error: no large enough free extent $ This patch will refresh the pool, rereading the partitions, and return Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_disk.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index ba928d8..17c6492 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -746,10 +746,13 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn, goto cleanup; } - /* If this was the extended partition, then all the logical partitions - * are then lost. Make it easy on ourselves and just refresh the pool + /* If this is not a logical partition, then either we've removed an + * extended partition or a primary partion - refresh the pool which + * includes resetting the [n]freeExtents data so a subsequent allocation + * might be able to use what was deleted. A logical partition is part + * of an extended partition and handled differently */ - if (vol->source.partType == VIR_STORAGE_VOL_DISK_TYPE_EXTENDED) { + if (vol->source.partType != VIR_STORAGE_VOL_DISK_TYPE_LOGICAL) { virStoragePoolObjClearVols(pool); if (virStorageBackendDiskRefreshPool(conn, pool) < 0) goto cleanup; -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1206521 If the backend driver updates the pool available and/or allocation values, then the storage_driver VolCreateXML, VolCreateXMLFrom, and VolDelete APIs should not change the value; otherwise, it will appear as if the values were "doubled" for each change. Additionally since unsigned arithmetic will be used depending on the size and operation, either or both values could be appear to be much larger than they should be (in the EiB range). Currently only the disk pool updates the values, but other pools could. Assume a "fresh" disk pool of 500 MiB using /dev/sde: $ virsh pool-info disk-pool ... Capacity: 509.88 MiB Allocation: 0.00 B Available: 509.84 MiB $ virsh vol-create-as disk-pool sde1 --capacity 300M $ virsh pool-info disk-pool ... Capacity: 509.88 MiB Allocation: 600.47 MiB Available: 16.00 EiB Following assumes disk backend updated to refresh the disk pool at deletion of primary partition as well as extended partition: $ virsh vol-delete --pool disk-pool sde1 Vol sde1 deleted $ virsh pool-info disk-pool ... Capacity: 509.88 MiB Allocation: 9.73 EiB Available: 6.27 EiB This patch will check if the backend updated the pool values and honor that update. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_driver.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8d91879..47486e2 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1499,6 +1499,7 @@ 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, @@ -1507,6 +1508,9 @@ 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; @@ -1514,8 +1518,10 @@ storageVolDeleteInternal(virStorageVolPtr obj, * in this module since the allocation/available weren't adjusted yet */ if (updateMeta) { - pool->def->allocation -= vol->target.allocation; - pool->def->available += vol->target.allocation; + 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; } for (i = 0; i < pool->volumes.count; i++) { @@ -1634,6 +1640,7 @@ 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); @@ -1681,6 +1688,9 @@ 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); @@ -1738,8 +1748,10 @@ storageVolCreateXML(virStoragePoolPtr obj, goto cleanup; /* Update pool metadata */ - pool->def->allocation += buildvoldef->target.allocation; - pool->def->available -= buildvoldef->target.allocation; + 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; VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); @@ -1767,6 +1779,7 @@ 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 | @@ -1869,6 +1882,9 @@ 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. */ @@ -1922,8 +1938,10 @@ storageVolCreateXMLFrom(virStoragePoolPtr obj, newvol = NULL; /* Updating pool metadata */ - pool->def->allocation += allocation; - pool->def->available -= allocation; + if (orig_pool_allocation == pool->def->allocation) + pool->def->allocation += allocation; + if (orig_pool_available == pool->def->available) + pool->def->available -= allocation; VIR_INFO("Creating volume '%s' in storage pool '%s'", volobj->name, pool->def->name); -- 2.1.0
On 03/27/2015 12:07 PM, John Ferlan wrote:
Patch 1 & 3 are bz based, while Patch 2 was determined while working on Patch 3. Details in each commit message
John Ferlan (3): storage: Fix issues in storageVolResize storage: Need to update freeExtent at delete primary partition storage: Don't duplicate efforts of backend driver
src/storage/storage_backend_disk.c | 9 +++-- src/storage/storage_driver.c | 67 ++++++++++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 17 deletions(-)
ping? Tks - John
On 27.03.2015 17:07, John Ferlan wrote:
Patch 1 & 3 are bz based, while Patch 2 was determined while working on Patch 3. Details in each commit message
John Ferlan (3): storage: Fix issues in storageVolResize storage: Need to update freeExtent at delete primary partition storage: Don't duplicate efforts of backend driver
src/storage/storage_backend_disk.c | 9 +++-- src/storage/storage_driver.c | 67 ++++++++++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 17 deletions(-)
ACK series Michal
participants (2)
-
John Ferlan -
Michal Privoznik