[libvirt] [PATCH 0/3] volume resize fixes
First two patches fix bugs and are applicable for the freeze. Ján Tomko (3): Simplify allocation check in storageVolResize Fix shrinking volumes with the delta flag virsh: make negative values with vol-resize more convenient src/storage/storage_driver.c | 26 +++++++++----------------- tools/virsh-volume.c | 15 ++++++++------- 2 files changed, 17 insertions(+), 24 deletions(-) -- 2.3.6
The volume cannot be shrinked below existing allocation, thus a successful resize with VOL_RESIZE_ALLOCATE will never increase the pool's available value. Even with the SHRINK flag it is possible to extend the current allocation or even the capacity. Remove the overflow when computing delta with this flag and do the check even if the flag was specified. https://bugzilla.redhat.com/show_bug.cgi?id=1073305 --- src/storage/storage_driver.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ac4a74a..fbb8050 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2292,7 +2292,7 @@ storageVolResize(virStorageVolPtr obj, virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; - unsigned long long abs_capacity, delta; + unsigned long long abs_capacity, delta = 0; int ret = -1; virCheckFlags(VIR_STORAGE_VOL_RESIZE_ALLOCATE | @@ -2341,18 +2341,14 @@ storageVolResize(virStorageVolPtr obj, goto cleanup; } - if (flags & VIR_STORAGE_VOL_RESIZE_SHRINK) - delta = vol->target.allocation - abs_capacity; - else + if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) 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) { + if (delta > pool->def->available) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Not enough space left in storage pool")); goto cleanup; @@ -2375,15 +2371,8 @@ storageVolResize(virStorageVolPtr obj, */ if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) { vol->target.allocation = abs_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; - } + pool->def->allocation += delta; + pool->def->available -= delta; } ret = 0; -- 2.3.6
On 05/27/2015 11:08 AM, Ján Tomko wrote:
The volume cannot be shrinked below existing allocation, thus a successful resize with VOL_RESIZE_ALLOCATE will never increase the pool's available value.
Since shrinking a volume below existing allocation is not allowed, it is not possible for a successful resize with VOL_RESIZE_ALLOCATE to increase the pool's available value.
Even with the SHRINK flag it is possible to extend the current allocation or even the capacity. Remove the overflow when computing delta with this flag and do the check even if the flag was specified.
Editorial comment: This bz should go back to POST...
--- src/storage/storage_driver.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ac4a74a..fbb8050 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2292,7 +2292,7 @@ storageVolResize(virStorageVolPtr obj, virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; - unsigned long long abs_capacity, delta; + unsigned long long abs_capacity, delta = 0; int ret = -1;
virCheckFlags(VIR_STORAGE_VOL_RESIZE_ALLOCATE | @@ -2341,18 +2341,14 @@ storageVolResize(virStorageVolPtr obj, goto cleanup; }
- if (flags & VIR_STORAGE_VOL_RESIZE_SHRINK) - delta = vol->target.allocation - abs_capacity; - else + if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) 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 */
The comment won't make sense any more as well. ACK I would think safe for freeze John
- if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE && - !(flags & VIR_STORAGE_VOL_RESIZE_SHRINK) && - delta > pool->def->available) { + if (delta > pool->def->available) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Not enough space left in storage pool")); goto cleanup; @@ -2375,15 +2371,8 @@ storageVolResize(virStorageVolPtr obj, */ if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) { vol->target.allocation = abs_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; - } + pool->def->allocation += delta; + pool->def->available -= delta; }
ret = 0;
On Wed, May 27, 2015 at 03:03:04PM -0400, John Ferlan wrote:
On 05/27/2015 11:08 AM, Ján Tomko wrote:
The volume cannot be shrinked below existing allocation, thus a successful resize with VOL_RESIZE_ALLOCATE will never increase the pool's available value.
Since shrinking a volume below existing allocation is not allowed, it is not possible for a successful resize with VOL_RESIZE_ALLOCATE to increase the pool's available value.
Thanks, that's much clearer.
Even with the SHRINK flag it is possible to extend the current allocation or even the capacity. Remove the overflow when computing delta with this flag and do the check even if the flag was specified.
Editorial comment:
This bz should go back to POST...
--- src/storage/storage_driver.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ac4a74a..fbb8050 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2292,7 +2292,7 @@ storageVolResize(virStorageVolPtr obj, virStorageBackendPtr backend; virStoragePoolObjPtr pool = NULL; virStorageVolDefPtr vol = NULL; - unsigned long long abs_capacity, delta; + unsigned long long abs_capacity, delta = 0; int ret = -1;
virCheckFlags(VIR_STORAGE_VOL_RESIZE_ALLOCATE | @@ -2341,18 +2341,14 @@ storageVolResize(virStorageVolPtr obj, goto cleanup; }
- if (flags & VIR_STORAGE_VOL_RESIZE_SHRINK) - delta = vol->target.allocation - abs_capacity; - else + if (flags & VIR_STORAGE_VOL_RESIZE_ALLOCATE) 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 */
The comment won't make sense any more as well.
ACK
I removed the commend and pushed the first two patches. The third one does not fix a bug and can wait for the next release. Jan
I would think safe for freeze
John
This never worked. In 0.9.10 when this API was introduced, it was intended that the SHRINK flag combined with DELTA would shrink the volume by the specified capacity (to avoid passing negative numbers). See commit 055bbf4. When the SHRINK flag was finally implemented for the first backend in 1.2.13 (commit aa9aa6a), it was only implemented for the absolute values and with the delta flag the volume is always extended, regardless of the SHRINK flag. Treat the SHRINK flag as a minus sign when used together with DELTA, to allow shrinking volumes as was documented in the API since 0.9.10. https://bugzilla.redhat.com/show_bug.cgi?id=1220213 --- src/storage/storage_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index fbb8050..1ba6828 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2320,7 +2320,10 @@ storageVolResize(virStorageVolPtr obj, } if (flags & VIR_STORAGE_VOL_RESIZE_DELTA) { - abs_capacity = vol->target.capacity + capacity; + if (flags & VIR_STORAGE_VOL_RESIZE_SHRINK) + abs_capacity = vol->target.capacity - MIN(capacity, vol->target.capacity); + else + abs_capacity = vol->target.capacity + capacity; flags &= ~VIR_STORAGE_VOL_RESIZE_DELTA; } else { abs_capacity = capacity; -- 2.3.6
On 05/27/2015 11:08 AM, Ján Tomko wrote:
This never worked.
In 0.9.10 when this API was introduced, it was intended that the SHRINK flag combined with DELTA would shrink the volume by the specified capacity (to avoid passing negative numbers). See commit 055bbf4.
When the SHRINK flag was finally implemented for the first backend in 1.2.13 (commit aa9aa6a), it was only implemented for the absolute values and with the delta flag the volume is always extended, regardless of the SHRINK flag.
Treat the SHRINK flag as a minus sign when used together with DELTA, to allow shrinking volumes as was documented in the API since 0.9.10.
https://bugzilla.redhat.com/show_bug.cgi?id=1220213 --- src/storage/storage_driver.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
ACK (seemingly safe too) John
When shrinking a volume by a certain size, instead of typing vol-resize volume 1G --delta --shrink we allow the convience of specifying a negative value: vol-resize volume -1G --delta --shrink getting the same results with one more character. A negative value only makes sense as a delta. Imply the --delta parameter if the value is negative. Still require --shrink, because the operation is potentially destructive. --- tools/virsh-volume.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/tools/virsh-volume.c b/tools/virsh-volume.c index 5d6cc6a..989ce87 100644 --- a/tools/virsh-volume.c +++ b/tools/virsh-volume.c @@ -1124,14 +1124,10 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd) unsigned long long capacity = 0; unsigned int flags = 0; bool ret = false; - bool delta = false; + bool delta = vshCommandOptBool(cmd, "delta"); if (vshCommandOptBool(cmd, "allocate")) flags |= VIR_STORAGE_VOL_RESIZE_ALLOCATE; - if (vshCommandOptBool(cmd, "delta")) { - delta = true; - flags |= VIR_STORAGE_VOL_RESIZE_DELTA; - } if (vshCommandOptBool(cmd, "shrink")) flags |= VIR_STORAGE_VOL_RESIZE_SHRINK; @@ -1144,14 +1140,19 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd) if (*capacityStr == '-') { /* The API always requires a positive value; but we allow a * negative value for convenience. */ - if (delta && vshCommandOptBool(cmd, "shrink")) { + if (vshCommandOptBool(cmd, "shrink")) { capacityStr++; + delta = true; } else { vshError(ctl, "%s", - _("negative size requires --delta and --shrink")); + _("negative size requires --shrink")); goto cleanup; } } + + if (delta) + flags |= VIR_STORAGE_VOL_RESIZE_DELTA; + if (vshVolSize(capacityStr, &capacity) < 0) { vshError(ctl, _("Malformed size %s"), capacityStr); goto cleanup; -- 2.3.6
On 05/27/2015 11:08 AM, Ján Tomko wrote:
When shrinking a volume by a certain size, instead of typing vol-resize volume 1G --delta --shrink we allow the convience of specifying a negative value: vol-resize volume -1G --delta --shrink getting the same results with one more character.
A negative value only makes sense as a delta. Imply the --delta parameter if the value is negative.
Still require --shrink, because the operation is potentially destructive. --- tools/virsh-volume.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
Should virsh.pod be updated as well to indicate that --shrink with negative number implies --delta (not required, but the text in virsh.pod around this is lacking... ACK (seemingly safe too) John
participants (2)
-
John Ferlan -
Ján Tomko