[libvirt] [PATCH v3 0/3] rbd: improvements to actual disk-usage calculation

From: Jason Dillaman <dillaman@redhat.com> The RBD fast-diff feature can vastly reduce the amount of time needed to calculate actual disk usage of volumes, but it might still be a slow operation for large RBD pools or pools with large RBD images. Therefore, this feature should be able to be optionally disabled if needed. Additionally, the fast-diff feature can only be used if the fast-diff map isn't flagged as invalid. Otherwise, librbd will silently perform a costly block-by-block scan to calculate the disk usage. since v1: - Moved RBD-unique refresh volume allocation override to shared storage pool attribute sice v2: - Moved attribute under new "<refresh><volume allocation='...'/></refresh>" elements. Jason Dillaman (3): rbd: do not attempt to use fast-diff if it's marked invalid storage: optional 'refresh' elemement on pool rbd: optionally compute volume allocation from capacity docs/formatstorage.html.in | 27 ++++++++++++ docs/schemas/storagecommon.rng | 7 ++++ docs/schemas/storagepool.rng | 23 ++++++++++ src/conf/storage_conf.c | 27 ++++++++++++ src/conf/storage_conf.h | 9 ++++ src/storage/storage_backend_rbd.c | 42 +++++++++++++++++-- .../pool-rbd-refresh-volume-allocation.xml | 15 +++++++ .../pool-rbd-refresh-volume-allocation.xml | 18 ++++++++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 165 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml -- 2.20.1

From: Jason Dillaman <dillaman@redhat.com> The librbd API will transparently revert to a slow disk usage calculation method if the fast-diff map is marked as invalid. Signed-off-by: Jason Dillaman <dillaman@redhat.com> --- src/storage/storage_backend_rbd.c | 41 ++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 2b7af1db23..e67911f928 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -437,10 +437,29 @@ volStorageBackendRBDGetFeatures(rbd_image_t image, } #if LIBRBD_VERSION_CODE > 265 +static int +volStorageBackendRBDGetFlags(rbd_image_t image, + const char *volname, + uint64_t *flags) +{ + int r, ret = -1; + + if ((r = rbd_get_flags(image, flags)) < 0) { + virReportSystemError(-r, _("failed to get the flags of RBD image " + "%s"), volname); + goto cleanup; + } + ret = 0; + + cleanup: + return ret; +} + static bool -volStorageBackendRBDUseFastDiff(uint64_t features) +volStorageBackendRBDUseFastDiff(uint64_t features, uint64_t flags) { - return features & RBD_FEATURE_FAST_DIFF; + return (((features & RBD_FEATURE_FAST_DIFF) != 0ULL) && + ((flags & RBD_FLAG_FAST_DIFF_INVALID) == 0ULL)); } static int @@ -484,7 +503,17 @@ virStorageBackendRBDSetAllocation(virStorageVolDefPtr vol, #else static int -volStorageBackendRBDUseFastDiff(uint64_t features ATTRIBUTE_UNUSED) +volStorageBackendRBDGetFlags(rbd_image_t image, + const char *volname, + uint64_t *flags) +{ + *flags = 0; + return 0; +} + +static int +volStorageBackendRBDUseFastDiff(uint64_t features ATTRIBUTE_UNUSED, + uint64_t feature_flags ATTRIBUTE_UNUSED) { return false; } @@ -509,6 +538,7 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, rbd_image_t image = NULL; rbd_image_info_t info; uint64_t features; + uint64_t flags; if ((r = rbd_open_read_only(ptr->ioctx, vol->name, &image, NULL)) < 0) { ret = -r; @@ -527,11 +557,14 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, if (volStorageBackendRBDGetFeatures(image, vol->name, &features) < 0) goto cleanup; + if (volStorageBackendRBDGetFlags(image, vol->name, &flags) < 0) + goto cleanup; + vol->target.capacity = info.size; vol->type = VIR_STORAGE_VOL_NETWORK; vol->target.format = VIR_STORAGE_FILE_RAW; - if (volStorageBackendRBDUseFastDiff(features)) { + if (volStorageBackendRBDUseFastDiff(features, flags)) { VIR_DEBUG("RBD image %s/%s has fast-diff feature enabled. " "Querying for actual allocation", def->source.name, vol->name); -- 2.20.1

On 3/19/19 2:42 PM, jdillama@redhat.com wrote:
From: Jason Dillaman <dillaman@redhat.com>
The librbd API will transparently revert to a slow disk usage calculation method if the fast-diff map is marked as invalid.
Signed-off-by: Jason Dillaman <dillaman@redhat.com> --- src/storage/storage_backend_rbd.c | 41 ++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 2b7af1db23..e67911f928 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -437,10 +437,29 @@ volStorageBackendRBDGetFeatures(rbd_image_t image, }
#if LIBRBD_VERSION_CODE > 265 +static int +volStorageBackendRBDGetFlags(rbd_image_t image, + const char *volname, + uint64_t *flags) +{ + int r, ret = -1; + + if ((r = rbd_get_flags(image, flags)) < 0) { + virReportSystemError(-r, _("failed to get the flags of RBD image " + "%s"), volname); + goto cleanup; + } + ret = 0; + + cleanup: + return ret; +}
I've simplified this a bit. Michal

From: Jason Dillaman <dillaman@redhat.com> The new 'refresh' element can override the default refresh operations for a storage pool. The only currently supported override is to set the volume allocation size to the volume capacity. This can be specified by adding the following snippet: <pool> ... <refresh> <volume allocation='capacity'/> </refresh> ... </pool> This is useful for certain backends where computing the actual allocation of a volume might be an expensive operation. Signed-off-by: Jason Dillaman <dillaman@redhat.com> --- docs/formatstorage.html.in | 27 +++++++++++++++++++ docs/schemas/storagecommon.rng | 7 +++++ docs/schemas/storagepool.rng | 23 ++++++++++++++++ src/conf/storage_conf.c | 27 +++++++++++++++++++ src/conf/storage_conf.h | 9 +++++++ .../pool-rbd-refresh-volume-allocation.xml | 15 +++++++++++ .../pool-rbd-refresh-volume-allocation.xml | 18 +++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 8 files changed, 127 insertions(+) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index 968651330f..680950ff20 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -508,6 +508,33 @@ device, measured in bytes. <span class="since">Since 0.4.1</span> </p> + <h3><a id="StoragePoolRefresh">Refresh overrides</a></h3> + + <p> + The optional <code>refresh</code> element can control how the pool and + associated volumes are refreshed (pool type <code>rbd</code>). The + <code>allocation</code> attribute of the <code>volume</code> child element + controls the method used for computing the allocation of a volume. The + valid attribute values are <code>default</code> to compute the actual + usage or <code>capacity</code> to use the logical capacity for cases where + computing the allocation is too expensive. The following XML snippet + shows the syntax: + <pre> +<pool type="rbd"> + <name>myrbdpool</name> +... + <source> +... + </source> +... + <refresh> + <volume allocation='capacity'/> + </refresh> +</pool> +...</pre> + <span class="since">Since 5.2.0</span> + </p> + <h3><a id="StoragePoolNamespaces">Storage Pool Namespaces</a></h3> <p> diff --git a/docs/schemas/storagecommon.rng b/docs/schemas/storagecommon.rng index 63b51470a0..d837f92bc7 100644 --- a/docs/schemas/storagecommon.rng +++ b/docs/schemas/storagecommon.rng @@ -236,4 +236,11 @@ </optional> </define> + <define name='refreshVolumeAllocation'> + <choice> + <value>default</value> + <value>capacity</value> + </choice> + </define> + </grammar> diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index 3907f70afe..3ca8e790ea 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -155,6 +155,7 @@ <ref name='commonMetadataNameOptional'/> <ref name='sizing'/> <ref name='sourcerbd'/> + <ref name='refresh'/> </interleave> <optional> <ref name='rbd_config_opts'/> @@ -691,6 +692,28 @@ </data> </define> + <define name='refresh'> + <optional> + <element name='refresh'> + <interleave> + <ref name='refreshVolume'/> + </interleave> + </element> + </optional> + </define> + + <define name='refreshVolume'> + <optional> + <element name='volume'> + <optional> + <attribute name='allocation'> + <ref name="refreshVolumeAllocation"/> + </attribute> + </optional> + </element> + </optional> + </define> + <!-- Optional storage pool extensions in their own namespace: "fs" or "netfs" diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index c7ab5b8802..d76ded2d02 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -94,6 +94,11 @@ VIR_ENUM_IMPL(virStorageVolFormatDisk, "extended", ); +VIR_ENUM_IMPL(virStorageVolRefreshAllocation, + VIR_STORAGE_VOL_REFRESH_ALLOCATION_LAST, + "default", "capacity", +); + VIR_ENUM_IMPL(virStoragePartedFs, VIR_STORAGE_PARTED_FS_TYPE_LAST, "ext2", "ext2", "fat16", @@ -799,6 +804,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) VIR_AUTOFREE(char *) type = NULL; VIR_AUTOFREE(char *) uuid = NULL; VIR_AUTOFREE(char *) target_path = NULL; + VIR_AUTOFREE(char *) refresh_volume_allocation = NULL; if (VIR_ALLOC(def) < 0) return NULL; @@ -931,6 +937,18 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) return NULL; } + refresh_volume_allocation = virXPathString("string(./refresh/volume/@allocation)", + ctxt); + if (refresh_volume_allocation) { + if ((def->refresh_volume_allocation = + virStorageVolRefreshAllocationTypeFromString(refresh_volume_allocation)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown storage pool volume refresh allocation type %s"), + refresh_volume_allocation); + return NULL; + } + } + /* Make a copy of all the callback pointers here for easier use, * especially during the virStoragePoolSourceClear method */ def->ns = options->ns; @@ -1163,6 +1181,15 @@ virStoragePoolDefFormatBuf(virBufferPtr buf, virBufferAddLit(buf, "</target>\n"); } + if (def->refresh_volume_allocation) { + virBufferAddLit(buf, "<refresh>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<volume allocation='%s'/>\n", + virStorageVolRefreshAllocationTypeToString(def->refresh_volume_allocation)); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</refresh>\n"); + } + if (def->namespaceData && def->ns.format) { if ((def->ns.format)(buf, def->namespaceData) < 0) return -1; diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index bfbebd15bd..f61d8f5a18 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -83,6 +83,13 @@ struct _virStorageVolSource { * backend for partition type creation */ }; +typedef enum { + VIR_STORAGE_VOL_REFRESH_ALLOCATION_DEFAULT, /* compute actual allocation */ + VIR_STORAGE_VOL_REFRESH_ALLOCATION_CAPACITY, /* use logical capacity */ + VIR_STORAGE_VOL_REFRESH_ALLOCATION_LAST, +} virStorageVolRefreshAllocationType; + +VIR_ENUM_DECL(virStorageVolRefreshAllocation); typedef struct _virStorageVolDef virStorageVolDef; typedef virStorageVolDef *virStorageVolDefPtr; @@ -243,6 +250,8 @@ struct _virStoragePoolDef { unsigned char uuid[VIR_UUID_BUFLEN]; int type; /* virStoragePoolType */ + int refresh_volume_allocation; /* virStorageVolRefreshAllocationType */ + unsigned long long allocation; /* bytes */ unsigned long long capacity; /* bytes */ unsigned long long available; /* bytes */ diff --git a/tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml b/tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml new file mode 100644 index 0000000000..e888b348b9 --- /dev/null +++ b/tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml @@ -0,0 +1,15 @@ +<pool type='rbd'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <source> + <name>rbd</name> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <auth username='admin' type='ceph'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> + <refresh> + <volume allocation='capacity'/> + </refresh> +</pool> diff --git a/tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml b/tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml new file mode 100644 index 0000000000..9c9f48acaa --- /dev/null +++ b/tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml @@ -0,0 +1,18 @@ +<pool type='rbd'> + <name>ceph</name> + <uuid>47c1faee-0207-e741-f5ae-d9b019b98fe2</uuid> + <capacity unit='bytes'>0</capacity> + <allocation unit='bytes'>0</allocation> + <available unit='bytes'>0</available> + <source> + <host name='localhost' port='6789'/> + <host name='localhost' port='6790'/> + <name>rbd</name> + <auth type='ceph' username='admin'> + <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/> + </auth> + </source> + <refresh> + <volume allocation='capacity'/> + </refresh> +</pool> diff --git a/tests/storagepoolxml2xmltest.c b/tests/storagepoolxml2xmltest.c index bd3408e8b8..2ae514f346 100644 --- a/tests/storagepoolxml2xmltest.c +++ b/tests/storagepoolxml2xmltest.c @@ -95,6 +95,7 @@ mymain(void) DO_TEST("pool-zfs-sourcedev"); DO_TEST("pool-rbd"); #ifdef WITH_STORAGE_RBD + DO_TEST("pool-rbd-refresh-volume-allocation"); DO_TEST("pool-rbd-ns-configopts"); #endif DO_TEST("pool-vstorage"); -- 2.20.1

On 3/19/19 2:42 PM, jdillama@redhat.com wrote:
From: Jason Dillaman <dillaman@redhat.com>
The new 'refresh' element can override the default refresh operations for a storage pool. The only currently supported override is to set the volume allocation size to the volume capacity. This can be specified by adding the following snippet:
<pool> ... <refresh> <volume allocation='capacity'/> </refresh> ... </pool>
This is useful for certain backends where computing the actual allocation of a volume might be an expensive operation.
Signed-off-by: Jason Dillaman <dillaman@redhat.com> --- docs/formatstorage.html.in | 27 +++++++++++++++++++ docs/schemas/storagecommon.rng | 7 +++++ docs/schemas/storagepool.rng | 23 ++++++++++++++++ src/conf/storage_conf.c | 27 +++++++++++++++++++ src/conf/storage_conf.h | 9 +++++++ .../pool-rbd-refresh-volume-allocation.xml | 15 +++++++++++ .../pool-rbd-refresh-volume-allocation.xml | 18 +++++++++++++ tests/storagepoolxml2xmltest.c | 1 + 8 files changed, 127 insertions(+) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index bfbebd15bd..f61d8f5a18 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -83,6 +83,13 @@ struct _virStorageVolSource { * backend for partition type creation */ };
+typedef enum { + VIR_STORAGE_VOL_REFRESH_ALLOCATION_DEFAULT, /* compute actual allocation */ + VIR_STORAGE_VOL_REFRESH_ALLOCATION_CAPACITY, /* use logical capacity */ + VIR_STORAGE_VOL_REFRESH_ALLOCATION_LAST, +} virStorageVolRefreshAllocationType; + +VIR_ENUM_DECL(virStorageVolRefreshAllocation);
typedef struct _virStorageVolDef virStorageVolDef; typedef virStorageVolDef *virStorageVolDefPtr; @@ -243,6 +250,8 @@ struct _virStoragePoolDef { unsigned char uuid[VIR_UUID_BUFLEN]; int type; /* virStoragePoolType */
+ int refresh_volume_allocation; /* virStorageVolRefreshAllocationType */
We want our structures to be as close to XML as possible. I'm introducing virStoragePoolDefRefresh and virStorageVolDefRefresh structs, parser and formatter funcs and renaming the enum as a result of that. I was also playing with the placement of <refresh/> element within <pool/> XML. At first I though it should go a bit higher - somewhere close to <allocation/>, <capacity/> and <available/>. My reasoning was that this tunes how those values are computed. But then I realized, for storage pool XML we keep config knobs (e.g. namespace data) at the end. So I'm keeping the element at the place you're introcing it - it's correct after all. Michal

From: Jason Dillaman <dillaman@redhat.com> Use the new refresh volume allocation pool override to skip computing the actual volume usage if disabled. Signed-off-by: Jason Dillaman <dillaman@redhat.com> --- 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 e67911f928..1b01fbb3a8 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -564,7 +564,8 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, vol->type = VIR_STORAGE_VOL_NETWORK; vol->target.format = VIR_STORAGE_FILE_RAW; - if (volStorageBackendRBDUseFastDiff(features, flags)) { + if (def->refresh_volume_allocation == VIR_STORAGE_VOL_REFRESH_ALLOCATION_DEFAULT && + volStorageBackendRBDUseFastDiff(features, flags)) { VIR_DEBUG("RBD image %s/%s has fast-diff feature enabled. " "Querying for actual allocation", def->source.name, vol->name); -- 2.20.1

On 3/19/19 2:42 PM, jdillama@redhat.com wrote:
From: Jason Dillaman <dillaman@redhat.com>
Use the new refresh volume allocation pool override to skip computing the actual volume usage if disabled.
Signed-off-by: Jason Dillaman <dillaman@redhat.com> --- 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 e67911f928..1b01fbb3a8 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -564,7 +564,8 @@ volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, vol->type = VIR_STORAGE_VOL_NETWORK; vol->target.format = VIR_STORAGE_FILE_RAW;
- if (volStorageBackendRBDUseFastDiff(features, flags)) { + if (def->refresh_volume_allocation == VIR_STORAGE_VOL_REFRESH_ALLOCATION_DEFAULT && + volStorageBackendRBDUseFastDiff(features, flags)) { VIR_DEBUG("RBD image %s/%s has fast-diff feature enabled. " "Querying for actual allocation", def->source.name, vol->name);
I'm updating this to reflect changes I made in 2/3. Michal

On 3/19/19 2:42 PM, jdillama@redhat.com wrote:
From: Jason Dillaman <dillaman@redhat.com>
The RBD fast-diff feature can vastly reduce the amount of time needed to calculate actual disk usage of volumes, but it might still be a slow operation for large RBD pools or pools with large RBD images. Therefore, this feature should be able to be optionally disabled if needed.
Additionally, the fast-diff feature can only be used if the fast-diff map isn't flagged as invalid. Otherwise, librbd will silently perform a costly block-by-block scan to calculate the disk usage.
since v1: - Moved RBD-unique refresh volume allocation override to shared storage pool attribute
sice v2: - Moved attribute under new "<refresh><volume allocation='...'/></refresh>" elements.
Jason Dillaman (3): rbd: do not attempt to use fast-diff if it's marked invalid storage: optional 'refresh' elemement on pool rbd: optionally compute volume allocation from capacity
docs/formatstorage.html.in | 27 ++++++++++++ docs/schemas/storagecommon.rng | 7 ++++ docs/schemas/storagepool.rng | 23 ++++++++++ src/conf/storage_conf.c | 27 ++++++++++++ src/conf/storage_conf.h | 9 ++++ src/storage/storage_backend_rbd.c | 42 +++++++++++++++++-- .../pool-rbd-refresh-volume-allocation.xml | 15 +++++++ .../pool-rbd-refresh-volume-allocation.xml | 18 ++++++++ tests/storagepoolxml2xmltest.c | 1 + 9 files changed, 165 insertions(+), 4 deletions(-) create mode 100644 tests/storagepoolxml2xmlin/pool-rbd-refresh-volume-allocation.xml create mode 100644 tests/storagepoolxml2xmlout/pool-rbd-refresh-volume-allocation.xml
I'm doing some tweaks (nothing serious), ACKing and pushing. Congratulations on your first libvirt contribution! Michal
participants (2)
-
jdillama@redhat.com
-
Michal Privoznik