Re: [PATCHv3 08/10] threshold: scrape threshold data from QMP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]