On Mon, Jun 22, 2015 at 17:06:44 -0600, Eric Blake wrote:
> Expose threshold information by collecting the information from
> QMP commands. qemu 2.3 has a way to get threshold information
> on all elements of a block chain, but only when node names are
> used - what's worse, the threshold information is only provided
> by 'query-named-block-nodes', but the mapping between devices
> and nodes is only provided by 'query-blockstats'. Rather than
> manage node names ourselves, we rely on qemu 2.4 doing auto
> naming; then when collecting the stats, we make a pass through
> both query functions.
>
> I chose to go with the naive O(n^2) mapping algorithm; if it turns
> out to be slow in practice on a guest with lots of nodes, we can
> enhance it via a sorted list or hash table lookup.
>
> * src/qemu/qemu_monitor.h (qemuMonitorBlockStatsUpdateThreshold):
> New prototype.
> * src/qemu/qemu_monitor.c (qemuMonitorBlockStatsUpdateThreshold):
> New function.
> * src/qemu/qemu_monitor_json.h
> (qemuMonitorJSONBlockStatsUpdateThreshold): New prototype.
> * src/qemu/qemu_monitor_json.c (qemuMonitorJSONDevGetBlockExtent):
> Populate node name.
> (qemuMonitorJSONBlockStatsUpdateThreshold)
> (qemuMonitorJSONBlockStatsUpdateOneThreshold): Use it to populate
> threshold data.
> (qemuMonitorJSONGetOneBlockStatsInfo)
> (qemuMonitorJSONGetBlockExtent): Update callers.
> * src/qemu/qemu_driver.c (qemuDomainGetStatsOneBlock): Expose
> threshold data.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 12 ++++-
> src/qemu/qemu_monitor.c | 13 ++++++
> src/qemu/qemu_monitor.h | 4 ++
> src/qemu/qemu_monitor_json.c | 102 ++++++++++++++++++++++++++++++++++++++-----
> src/qemu/qemu_monitor_json.h | 2 +
> 5 files changed, 121 insertions(+), 12 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 25bc76d..72e256b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19282,6 +19282,12 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver,
> QEMU_ADD_BLOCK_PARAM_LL(record, maxparams, block_idx,
> "fl.times", entry->flush_total_times);
>
> + /* TODO: Until we can set thresholds on backing elements, we only
> + * report the threshold on the active layer. */
While this is okay in this intermediate part I am not okay with merging
this series without the full support for non-top layers.
If we add a partial impl we will force the users to check whether given
libvirt supports it.
Since I know of only one consumer of this feature which is oVirt and
they are interrested in both data we would increase the complexity of
usefullnes of this feature.
> + if (!backing_idx)
> + QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
> + "write-threshold", entry->write_threshold);
> +
> QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx,
> "allocation", entry->wr_highest_offset);
>
> @@ -19323,9 +19329,13 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
> qemuDomainObjEnterMonitor(driver, dom);
> rc = qemuMonitorGetAllBlockStatsInfo(priv->mon, &stats,
> visitBacking);
> - if (rc >= 0)
> + if (rc >= 0) {
> ignore_value(qemuMonitorBlockStatsUpdateCapacity(priv->mon, stats,
> visitBacking));
> + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_AUTO_NODE_NAMES))
> + ignore_value(qemuMonitorBlockStatsUpdateThreshold(priv->mon,
> + stats));
> + }
> if (qemuDomainObjExitMonitor(driver, dom) < 0)
> goto cleanup;
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 86dc4be..a3ad740 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1838,6 +1838,19 @@ qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
> }
>
>
> +/* Updates "stats" to fill write threshold of the image */
> +int
> +qemuMonitorBlockStatsUpdateThreshold(qemuMonitorPtr mon,
> + virHashTablePtr stats)
> +{
> + VIR_DEBUG("stats=%p", stats);
> +
> + QEMU_CHECK_MONITOR_JSON(mon);
> +
> + return qemuMonitorJSONBlockStatsUpdateThreshold(mon, stats);
> +}
> +
> +
> int
> qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
> const char *dev_name,
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index c0ea2ee..541f774 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -395,6 +395,10 @@ int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
> bool backingChain)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>
> +int qemuMonitorBlockStatsUpdateThreshold(qemuMonitorPtr mon,
> + virHashTablePtr stats)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> int qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
> const char *dev_name,
> unsigned long long *extent);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 32b2719..885b6f4 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -1668,9 +1668,13 @@ typedef enum {
> } qemuMonitorBlockExtentError;
>
>
> +/* Get the highest written extent. Additionally, if NODE is not null,
> + * and a node name is associated with the extent, then return that
> + * name; but failure to get a node name is not fatal. */
> static int
> qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev,
> - unsigned long long *extent)
> + unsigned long long *extent,
> + char **node)
I'll send a series today that will remove this function.
qemuMonitorJSONGetOneBlockStatsInfo will need to do this operation so
that we don't duplicate calls to query-blockstats.
> {
> virJSONValuePtr stats;
> virJSONValuePtr parent;
> @@ -1678,6 +1682,11 @@ qemuMonitorJSONDevGetBlockExtent(virJSONValuePtr dev,
> if ((parent = virJSONValueObjectGetObject(dev, "parent")) == NULL)
> return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOPARENT;
>
> + if (node) {
> + const char *name = virJSONValueObjectGetString(parent, "node-name");
> + ignore_value(VIR_STRDUP_QUIET(*node, name));
> + }
> +
> if ((stats = virJSONValueObjectGetObject(parent, "stats")) == NULL)
> return QEMU_MONITOR_BLOCK_EXTENT_ERROR_NOSTATS;
>
> @@ -1725,18 +1734,23 @@ qemuMonitorJSONGetOneBlockStatsInfo(virJSONValuePtr dev,
> goto cleanup; \
> } \
> }
> - QEMU_MONITOR_BLOCK_STAT_GET("rd_bytes", bstats->rd_bytes, true);
> - QEMU_MONITOR_BLOCK_STAT_GET("wr_bytes", bstats->wr_bytes, true);
> - QEMU_MONITOR_BLOCK_STAT_GET("rd_operations", bstats->rd_req, true);
> - QEMU_MONITOR_BLOCK_STAT_GET("wr_operations", bstats->wr_req, true);
> - QEMU_MONITOR_BLOCK_STAT_GET("rd_total_time_ns", bstats->rd_total_times, false);
> - QEMU_MONITOR_BLOCK_STAT_GET("wr_total_time_ns", bstats->wr_total_times, false);
> - QEMU_MONITOR_BLOCK_STAT_GET("flush_operations", bstats->flush_req, false);
> - QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns", bstats->flush_total_times, false);
> + QEMU_MONITOR_BLOCK_STAT_GET("rd_bytes", bstats->rd_bytes, true);
> + QEMU_MONITOR_BLOCK_STAT_GET("wr_bytes", bstats->wr_bytes, true);
> + QEMU_MONITOR_BLOCK_STAT_GET("rd_operations", bstats->rd_req, true);
> + QEMU_MONITOR_BLOCK_STAT_GET("wr_operations", bstats->wr_req, true);
> + QEMU_MONITOR_BLOCK_STAT_GET("rd_total_time_ns", bstats->rd_total_times,
> + false);
> + QEMU_MONITOR_BLOCK_STAT_GET("wr_total_time_ns", bstats->wr_total_times,
> + false);
> + QEMU_MONITOR_BLOCK_STAT_GET("flush_operations", bstats->flush_req,
> + false);
> + QEMU_MONITOR_BLOCK_STAT_GET("flush_total_time_ns",
> + bstats->flush_total_times, false);
Spurious whitespace change.
> #undef QEMU_MONITOR_BLOCK_STAT_GET
>
> /* it's ok to not have this information here. Just skip silently. */
> - qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset);
> + qemuMonitorJSONDevGetBlockExtent(dev, &bstats->wr_highest_offset,
> + &bstats->allocation_node);
>
> if (virHashAddEntry(hash, entry_name, bstats) < 0)
> goto cleanup;
> @@ -1937,6 +1951,72 @@ qemuMonitorJSONBlockStatsUpdateCapacity(qemuMonitorPtr mon,
> }
>
>
> +/* Hash table callback: Best effort routine to update write-threshold
> + * of a given qemuBlockStatsPtr (payload) using the QMP results for
> + * all nodes (opaque). */
> +static void
> +qemuMonitorJSONBlockStatsUpdateOneThreshold(void *payload,
> + const void *entry ATTRIBUTE_UNUSED,
> + void *opaque)
> +{
> + qemuBlockStatsPtr data = payload;
> + virJSONValuePtr nodes = opaque;
> + size_t i;
> +
> + if (!data->allocation_node)
> + return;
> + for (i = 0; i < virJSONValueArraySize(nodes); i++) {
> + virJSONValuePtr node = virJSONValueArrayGet(nodes, i);
> + const char *name = virJSONValueObjectGetString(node, "node-name");
> +
> + if (STREQ_NULLABLE(data->allocation_node, name) &&
> + virJSONValueObjectGetNumberUlong(node, "write_threshold",
> + &data->write_threshold) == 0)
> + break;
> + }
> +}
> +
> +
> +int
> +qemuMonitorJSONBlockStatsUpdateThreshold(qemuMonitorPtr mon,
> + virHashTablePtr stats)
> +{
> + int ret = -1;
> + int rc;
> + virJSONValuePtr cmd;
> + virJSONValuePtr reply = NULL;
> + virJSONValuePtr devices;
> +
> + if (!(cmd = qemuMonitorJSONMakeCommand("query-named-block-nodes", NULL)))
> + return -1;
> + if ((rc = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
> + goto cleanup;
> + if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> + goto cleanup;
> +
> + if (!(devices = virJSONValueObjectGetArray(reply, "return"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("query-named-block-nodes reply was missing list"));
> + goto cleanup;
> + }
> + /* O(n^2) collection pass because we visit the entire nodes list
> + * for each stats entry. Hopefully there aren't a boat-load of
> + * nodes to make this noticeably slower. If we need, we can do a
> + * pre-processing pass over devices to reach O(n log n) (via
> + * sorted list) or amortized O(n) (via a hash table) layout where
I think in the future it will be necessary to have the data from
query-named-blocks available for use so I think that this helper should
similarly to qemuMonitorJSONGetAllBlockStatsInfo return a hash of the
useful data, in this case keyed by the node name, and the caller would
do the disk->node matching when it requires the data if it requires it.
> + * node name lookup is more efficient. */
> + if (virJSONValueArraySize(devices) > 0)
> + virHashForEach(stats, qemuMonitorJSONBlockStatsUpdateOneThreshold,
> + devices);
> + ret = 0;
> +
> + cleanup:
> + virJSONValueFree(cmd);
> + virJSONValueFree(reply);
> + return ret;
> +}
> +
> +
> static int
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list