[libvirt] [PATCH] rbd: Allow shrinking and deltas when resizing RBD volumes
Modify virCheckFlags that it accepts both VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK as valid flags for resizing RBD volumes. This still does not solve the problem where RBD volumes can't be shrinked using libvirt. The allocation and capacity of RBD volumes are equal for a RBD volume and this makes libvirt think the volume is fully allocated and therefor can't be shrinked. Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 80a1d33..88b613a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -1041,7 +1041,8 @@ static int virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, int ret = -1; int r = 0; - virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_VOL_RESIZE_DELTA | + VIR_STORAGE_VOL_RESIZE_SHRINK, -1); if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) goto cleanup; -- 1.9.1
Through the years the RBD storage pool code hasn't maintained the same or correct coding standard which applies to libvirt. This patch doesn't change any logic in the code, it only applies the proper coding standards to the code where possible without making large changes. This way the code style used in this storage pool is consistent throughout the whole file. Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 108 +++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 55 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 80a1d33..4a8d90d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -49,9 +49,10 @@ struct _virStorageBackendRBDState { typedef struct _virStorageBackendRBDState virStorageBackendRBDState; typedef virStorageBackendRBDState *virStorageBackendRBDStatePtr; -static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, - virConnectPtr conn, - virStoragePoolSourcePtr source) +static int +virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, + virConnectPtr conn, + virStoragePoolSourcePtr source) { int ret = -1; int r = 0; @@ -71,8 +72,8 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, if (authdef) { VIR_DEBUG("Using cephx authorization, username: %s", authdef->username); - r = rados_create(&ptr->cluster, authdef->username); - if (r < 0) { + + if ((r = rados_create(&ptr->cluster, authdef->username)) < 0) { virReportSystemError(-r, "%s", _("failed to initialize RADOS")); goto cleanup; } @@ -222,8 +223,7 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rados_conf_set(ptr->cluster, "rbd_default_format", rbd_default_format); ptr->starttime = time(0); - r = rados_connect(ptr->cluster); - if (r < 0) { + if ((r = rados_connect(ptr->cluster)) < 0) { virReportSystemError(-r, _("failed to connect to the RADOS monitor on: %s"), mon_buff); goto cleanup; @@ -242,7 +242,9 @@ static int virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, return ret; } -static int virStorageBackendRBDOpenIoCTX(virStorageBackendRBDStatePtr ptr, virStoragePoolObjPtr pool) +static int +virStorageBackendRBDOpenIoCTX(virStorageBackendRBDStatePtr ptr, + virStoragePoolObjPtr pool) { int r = rados_ioctx_create(ptr->cluster, pool->def->source.name, &ptr->ioctx); if (r < 0) { @@ -252,7 +254,8 @@ static int virStorageBackendRBDOpenIoCTX(virStorageBackendRBDStatePtr ptr, virSt return r; } -static int virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr) +static int +virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr) { int ret = 0; @@ -276,25 +279,24 @@ static int virStorageBackendRBDCloseRADOSConn(virStorageBackendRBDStatePtr ptr) return ret; } -static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, - virStoragePoolObjPtr pool, - virStorageBackendRBDStatePtr ptr) +static int +volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, + virStoragePoolObjPtr pool, + virStorageBackendRBDStatePtr ptr) { int ret = -1; int r = 0; rbd_image_t image = NULL; + rbd_image_info_t info; - r = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL); - if (r < 0) { + if ((r = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL)) < 0) { ret = -r; virReportSystemError(-r, _("failed to open the RBD image '%s'"), vol->name); goto cleanup; } - rbd_image_info_t info; - r = rbd_stat(image, &info, sizeof(info)); - if (r < 0) { + if ((r = rbd_stat(image, &info, sizeof(info))) < 0) { ret = -r; virReportSystemError(-r, _("failed to stat the RBD image '%s'"), vol->name); @@ -331,8 +333,9 @@ static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, return ret; } -static int virStorageBackendRBDRefreshPool(virConnectPtr conn, - virStoragePoolObjPtr pool) +static int +virStorageBackendRBDRefreshPool(virConnectPtr conn, + virStoragePoolObjPtr pool) { size_t max_size = 1024; int ret = -1; @@ -342,6 +345,8 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn, virStorageBackendRBDState ptr; ptr.cluster = NULL; ptr.ioctx = NULL; + struct rados_cluster_stat_t clusterstat; + struct rados_pool_stat_t poolstat; if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) goto cleanup; @@ -349,16 +354,12 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn, if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) goto cleanup; - struct rados_cluster_stat_t clusterstat; - r = rados_cluster_stat(ptr.cluster, &clusterstat); - if (r < 0) { + if ((r = rados_cluster_stat(ptr.cluster, &clusterstat)) < 0) { virReportSystemError(-r, "%s", _("failed to stat the RADOS cluster")); goto cleanup; } - struct rados_pool_stat_t poolstat; - r = rados_ioctx_pool_stat(ptr.ioctx, &poolstat); - if (r < 0) { + if ((r = rados_ioctx_pool_stat(ptr.ioctx, &poolstat)) < 0) { virReportSystemError(-r, _("failed to stat the RADOS pool '%s'"), pool->def->source.name); goto cleanup; @@ -440,9 +441,10 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn, return ret; } -static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, - virStoragePoolSourcePtr source, - virStorageVolDefPtr vol) +static int +virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, + virStoragePoolSourcePtr source, + virStorageVolDefPtr vol) { int ret = -1; int r = 0; @@ -452,8 +454,7 @@ static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, rbd_snap_info_t *snaps = NULL; rbd_image_t image = NULL; - r = rbd_open(ioctx, vol->name, &image, NULL); - if (r < 0) { + if ((r = rbd_open(ioctx, vol->name, &image, NULL)) < 0) { virReportSystemError(-r, _("failed to open the RBD image '%s'"), vol->name); goto cleanup; @@ -474,8 +475,7 @@ static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, if (snap_count > 0) { for (i = 0; i < snap_count; i++) { - r = rbd_snap_is_protected(image, snaps[i].name, &protected); - if (r < 0) { + if ((r = rbd_snap_is_protected(image, snaps[i].name, &protected)) < 0) { virReportSystemError(-r, _("failed to verify if snapshot '%s/%s@%s' is protected"), source->name, vol->name, snaps[i].name); @@ -487,8 +487,7 @@ static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, "unprotected", source->name, vol->name, snaps[i].name); - r = rbd_snap_unprotect(image, snaps[i].name); - if (r < 0) { + if ((r = rbd_snap_unprotect(image, snaps[i].name)) < 0) { virReportSystemError(-r, _("failed to unprotect snapshot '%s/%s@%s'"), source->name, vol->name, snaps[i].name); @@ -499,8 +498,7 @@ static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, VIR_DEBUG("Removing snapshot %s/%s@%s", source->name, vol->name, snaps[i].name); - r = rbd_snap_remove(image, snaps[i].name); - if (r < 0) { + if ((r = rbd_snap_remove(image, snaps[i].name)) < 0) { virReportSystemError(-r, _("failed to remove snapshot '%s/%s@%s'"), source->name, vol->name, snaps[i].name); @@ -523,10 +521,11 @@ static int virStorageBackendRBDCleanupSnapshots(rados_ioctx_t ioctx, return ret; } -static int virStorageBackendRBDDeleteVol(virConnectPtr conn, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - unsigned int flags) +static int +virStorageBackendRBDDeleteVol(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned int flags) { int ret = -1; int r = 0; @@ -648,9 +647,8 @@ virStorageBackendRBDBuildVol(virConnectPtr conn, goto cleanup; } - r = virStorageBackendRBDCreateImage(ptr.ioctx, vol->name, - vol->target.capacity); - if (r < 0) { + if ((r = virStorageBackendRBDCreateImage(ptr.ioctx, vol->name, + vol->target.capacity)) < 0) { virReportSystemError(-r, _("failed to create volume '%s/%s'"), pool->def->source.name, vol->name); @@ -1003,9 +1001,10 @@ virStorageBackendRBDBuildVolFrom(virConnectPtr conn, return ret; } -static int virStorageBackendRBDRefreshVol(virConnectPtr conn, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - virStorageVolDefPtr vol) +static int +virStorageBackendRBDRefreshVol(virConnectPtr conn, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol) { virStorageBackendRBDState ptr; ptr.cluster = NULL; @@ -1028,11 +1027,12 @@ static int virStorageBackendRBDRefreshVol(virConnectPtr conn, return ret; } -static int virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, - virStorageVolDefPtr vol, - unsigned long long capacity, - unsigned int flags) +static int +virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, + virStorageVolDefPtr vol, + unsigned long long capacity, + unsigned int flags) { virStorageBackendRBDState ptr; ptr.cluster = NULL; @@ -1049,15 +1049,13 @@ static int virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendRBDOpenIoCTX(&ptr, pool) < 0) goto cleanup; - r = rbd_open(ptr.ioctx, vol->name, &image, NULL); - if (r < 0) { + if ((r = rbd_open(ptr.ioctx, vol->name, &image, NULL)) < 0) { virReportSystemError(-r, _("failed to open the RBD image '%s'"), vol->name); goto cleanup; } - r = rbd_resize(image, capacity); - if (r < 0) { + if ((r = rbd_resize(image, capacity)) < 0) { virReportSystemError(-r, _("failed to resize the RBD image '%s'"), vol->name); goto cleanup; -- 1.9.1
On 01/30/2016 10:15 AM, Wido den Hollander wrote:
Through the years the RBD storage pool code hasn't maintained the same or correct coding standard which applies to libvirt.
This patch doesn't change any logic in the code, it only applies the proper coding standards to the code where possible without making large changes.
This way the code style used in this storage pool is consistent throughout the whole file.
Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 108 +++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 55 deletions(-)
ACK - pushed. John strange, this came as a reply-to the Allow shrinking patch... as did the subsequent 2/2
This was only used in debugging messages and not in any real code. Ceph/RBD uses uint64_t for sizes internally and they can be printed with %zu without any need for casting. Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 4a8d90d..5d73370 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -303,10 +303,9 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, goto cleanup; } - VIR_DEBUG("Refreshed RBD image %s/%s (size: %llu obj_size: %llu num_objs: %llu)", - pool->def->source.name, vol->name, (unsigned long long)info.size, - (unsigned long long)info.obj_size, - (unsigned long long)info.num_objs); + VIR_DEBUG("Refreshed RBD image %s/%s (size: %zu obj_size: %zu num_objs: %zu)", + pool->def->source.name, vol->name, info.size, info.obj_size, + info.num_objs); vol->target.capacity = info.size; vol->target.allocation = info.obj_size * info.num_objs; @@ -369,10 +368,9 @@ virStorageBackendRBDRefreshPool(virConnectPtr conn, pool->def->available = clusterstat.kb_avail * 1024; pool->def->allocation = poolstat.num_bytes; - VIR_DEBUG("Utilization of RBD pool %s: (kb: %llu kb_avail: %llu num_bytes: %llu)", - pool->def->source.name, (unsigned long long)clusterstat.kb, - (unsigned long long)clusterstat.kb_avail, - (unsigned long long)poolstat.num_bytes); + VIR_DEBUG("Utilization of RBD pool %s: (kb: %zu kb_avail: %zu num_bytes: %zu)", + pool->def->source.name, clusterstat.kb, clusterstat.kb_avail, + poolstat.num_bytes); while (true) { if (VIR_ALLOC_N(names, max_size) < 0) -- 1.9.1
On 01/30/2016 10:15 AM, Wido den Hollander wrote:
This was only used in debugging messages and not in any real code.
Ceph/RBD uses uint64_t for sizes internally and they can be printed with %zu without any need for casting.
Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
ACK, pushed. John
On Sat, Jan 30, 2016 at 04:15:32PM +0100, Wido den Hollander wrote:
Modify virCheckFlags that it accepts both VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK as valid flags for resizing RBD volumes.
This still does not solve the problem where RBD volumes can't be shrinked using libvirt. The allocation and capacity of RBD volumes are equal for a RBD volume and this makes libvirt think the volume is fully allocated and therefor can't be shrinked.
Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 80a1d33..88b613a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -1041,7 +1041,8 @@ static int virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, int ret = -1; int r = 0;
- virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_VOL_RESIZE_DELTA |
The storage driver calculates the new capacity in storageVolResize and masks off the VIR_STORAGE_VOL_RESIZE_DELTA flag. It should never be set here. Jan
On 01/30/2016 10:15 AM, Wido den Hollander wrote:
Modify virCheckFlags that it accepts both VIR_STORAGE_VOL_RESIZE_DELTA and VIR_STORAGE_VOL_RESIZE_SHRINK as valid flags for resizing RBD volumes.
This still does not solve the problem where RBD volumes can't be shrinked using libvirt. The allocation and capacity of RBD volumes are equal for a RBD volume and this makes libvirt think the volume is fully allocated and therefor can't be shrinked.
'shrinked' is just a strange word. Typically it's shrunk (US movie "Honey I shrunk the kids". Still the problem seems to be that shrinking an RBD volume ran into problems because allocation and capacity values are checked by the driver and perhaps some assumptions made about the underlying storage In any case - is what you're seeing because the storage_driver (storageVolResize) has some checks? Could you provide some context? Is it this check?: if (abs_capacity < vol->target.allocation) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("can't shrink capacity below " "existing allocation")); goto cleanup; } Since only _fs and _sheepdog support 'resizeVol' - maybe those checks are more _fs specific rather than general for all storage drivers.
Signed-off-by: Wido den Hollander <wido@widodh.nl> --- src/storage/storage_backend_rbd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 80a1d33..88b613a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -1041,7 +1041,8 @@ static int virStorageBackendRBDResizeVol(virConnectPtr conn ATTRIBUTE_UNUSED, int ret = -1; int r = 0;
- virCheckFlags(0, -1); + virCheckFlags(VIR_STORAGE_VOL_RESIZE_DELTA | + VIR_STORAGE_VOL_RESIZE_SHRINK, -1);
if (virStorageBackendRBDOpenRADOSConn(&ptr, conn, &pool->def->source) < 0) goto cleanup;
To add to Jan's comments, the virsh-volume.c tells us 'delta' is just a way to tell the storage driver that capacity is delta to current size rather than the new size. Perhaps a better explanation in virStorageVolResize to indicate that the flag is used only by the storage driver. Also, I would assume _rbd can increase (ALLOCATE) too, right? John
participants (3)
-
John Ferlan -
Ján Tomko -
Wido den Hollander