On Mon, Jun 22, 2015 at 17:06:43 -0600, Eric Blake wrote:
> Set up functions to make it easy to map between libvirt disk
> names and qemu node names. Although we won't expose the
> information in XML, it is still nicer to cache the information
> than to have to grab a job lock, so that we are less likely
> to need to re-query the monitor when dealing with a qemu
> monitor event. We need the information in two places: one in
> the hash table used to collect QMP stats (as the QMP design
> requires using 'query-blockstats' to learn node names,
> followed by 'query-named-block-nodes' to learn statistics
> about those nodes), and the other in virStorageSourcePtr (the
> disk information tracked by libvirt). Making sure this
> doesn't leak memory was interesting; in particular, in
> qemu_driver.c:qemUDomainBlocksStatsGather(), I made sure that
> the returned result does not set an allocation name, so that
> the callers of that function can continue to use VIR_FREE.
>
> * src/util/virstoragefile.h (_virStorageSource): Add a field.
> * src/util/virstoragefile.c (virStorageSourceBackingStoreClear):
> Clear it.
> (virStorageSourceCopy): Document that it is not deep-copied.
> * src/qemu/qemu_domain.c (qemuDomainDiskGetAllocationNode)
> (qemuDomainDiskResolveAllocationNode): New functions, to use the
> field.
> (qemuDomainDiskSupportsAllocationNode): New helper.
> * src/qemu/qemu_domain.h: Declare new functions.
> * src/qemu/qemu_monitor.h (_qemuBlockStats): Add fields.
> (qemuMonitorCleanBlockStats): New declaration.
> * src/qemu/qemu_monitor.c (qemuMonitorCleanBlockStats): New
> function.
> (qemuMonitorGetAllBlockStatsInfo): Use it to avoid leak.
> * src/qemu/qemu_migration.c (qemuMigrationCookieAddNBD):
> Likewise.
> * src/qemu/qemu_driver.c (qemuDomainBlocksStatsGather): Don't leak
> memory.
> * tests/qemumonitortest.c (testBlockInfoData): Update test.
> ---
> src/qemu/qemu_domain.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_domain.h | 10 ++++-
> src/qemu/qemu_driver.c | 1 +
> src/qemu/qemu_migration.c | 2 +-
> src/qemu/qemu_monitor.c | 12 +++++-
> src/qemu/qemu_monitor.h | 4 ++
> src/util/virstoragefile.c | 4 +-
> src/util/virstoragefile.h | 3 +-
> tests/qemumonitortest.c | 13 +++---
> 9 files changed, 143 insertions(+), 11 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6213fd9..d3ce7db 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2906,6 +2906,111 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
> }
>
>
> +static bool
> +qemuDomainDiskSupportsAllocationNode(virDomainDiskDefPtr disk)
> +{
> + /* For now, only qcow2 images backed by block devices are supported. */
> + if (disk->src->type != VIR_STORAGE_TYPE_BLOCK)
> + return false;
> + if (disk->src->format != VIR_STORAGE_FILE_QCOW2)
> + return false;
> + return true;
> +}
> +
> +
> +/* Determine the node name that qemu associates with allocation
> + * tracking. Cache the value to minimize queries. Return NULL if the
> + * storage source is not a qcow2 formatted block device (as those are
> + * the only devices where live allocation tracking is useful), or if
> + * qemu cannot determine a node name. Requires a live domain, and
> + * assumes that the job lock is held, as this may require monitor
> + * interaction. */
> +const char *
> +qemuDomainDiskGetAllocationNode(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainDiskDefPtr disk)
> +{
> + virStorageSourcePtr src = disk->src;
> + char *name = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + if (!qemuDomainDiskSupportsAllocationNode(disk))
> + return NULL;
> +
> + if (src->allocation_node)
> + goto cleanup;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + /* TODO: allow lookup of node name of backing images */
> + name = qemuMonitorNodeNameLookup(priv->mon, disk->info.alias);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> +
> + src->allocation_node = name;
> + name = NULL;
> +
> + cleanup:
> + VIR_FREE(name);
> + return src->allocation_node;
> +}
> +
> +
> +/* Determine the disk name that matches a node name returned by qemu
> + * in a threshold event. In the common case, no job is needed: the
> + * node name will be cached from when the threshold was
> + * registered. But if the cache is lost (such as over a libvirtd
> + * restart), JOB_OKAY states whether it is safe to rebuild the cache
> + * (requires a live domain and monitor interaction), instead of
> + * failing. */
> +virDomainDiskDefPtr
> +qemuDomainDiskResolveAllocationNode(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + const char *node,
> + bool job_okay)
> +{
> + size_t i;
> + virDomainDiskDefPtr disk;
> + virDomainDiskDefPtr ret = NULL;
> + char *name = NULL;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + /* First pass - see if the node name is cached. */
> + for (i = 0; i < vm->def->ndisks; i++) {
> + disk = vm->def->disks[i];
> + if (!qemuDomainDiskSupportsAllocationNode(disk))
> + continue;
> + if (STREQ_NULLABLE(disk->src->allocation_node, node))
> + return disk;
> + }
> +
> + /* Second pass - query the monitor. */
> + if (!job_okay)
> + return NULL;
> + for (i = 0; i < vm->def->ndisks; i++) {
> + disk = vm->def->disks[i];
> + if (!qemuDomainDiskSupportsAllocationNode(disk) ||
> + disk->src->allocation_node)
> + continue;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + name = qemuMonitorNodeNameLookup(priv->mon, disk->info.alias);
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> +
> + disk->src->allocation_node = name;
> + name = NULL;
> + if (STREQ_NULLABLE(disk->src->allocation_node, node)) {
> + ret = disk;
> + break;
> + }
> + }
> +
> + cleanup:
> + VIR_FREE(name);
> + return ret;
> +}
Rather than doing the ad-hoc back and forth mapping I think we should
cache all node names for all disks when reconnecting to the monitor and
when we complete a snapshot operation. This will simplfiy the event code
quite a bit and additionally it will be more future proof.
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index c1373de..25bc76d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10967,6 +10967,7 @@ qemuDomainBlocksStatsGather(virQEMUDriverPtr driver,
> _("cannot find statistics for device '%s'"), diskAlias);
> goto cleanup;
> }
> + VIR_FREE(stats->allocation_node);
The callers should use qemuMonitorCleanBlockStats rather than deleting
it here.
>
> **retstats = *stats;
> } else {
...
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 5612491..86dc4be 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1763,6 +1763,16 @@ qemuMonitorBlockInfoLookup(virHashTablePtr blockInfo,
> }
>
>
> +/* Callback for use in cleaning up a hash used by
> + * qemuMonitorGetAllBlockStatsInfo. */
> +void
> +qemuMonitorCleanBlockStats(void *opaque, const void *name ATTRIBUTE_UNUSED)
We use either Clear or Free. I'd go with qemuMonitorBlockStatsFree.
> +{
> + qemuBlockStatsPtr stats = opaque;
> + VIR_FREE(stats->allocation_node);
> + VIR_FREE(stats);
> +}
> +
> /**
> * qemuMonitorGetAllBlockStatsInfo:
> * @mon: monitor object
...
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 826835b..c0ea2ee 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -379,8 +379,12 @@ struct _qemuBlockStats {
> unsigned long long capacity;
> unsigned long long physical;
> unsigned long long wr_highest_offset;
> + char *allocation_node;
See below ...
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index aa17a00..8b2f491 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -1,7 +1,7 @@
> /*
> * virstoragefile.h: file utility functions for FS storage backend
> *
> - * Copyright (C) 2007-2009, 2012-2014 Red Hat, Inc.
> + * Copyright (C) 2007-2009, 2012-2015 Red Hat, Inc.
> * Copyright (C) 2007-2008 Daniel P. Berrange
> *
> * This library is free software; you can redistribute it and/or
> @@ -260,6 +260,7 @@ struct _virStorageSource {
> unsigned long long capacity; /* in bytes, 0 if unknown */
> unsigned long long allocation; /* in bytes, 0 if unknown */
> unsigned long long physical; /* in bytes, 0 if unknown */
> + char *allocation_node; /* cache of node name for tracking allocation */
The node name will be useful for other operations too so I'd choose a
more universal name.
> size_t nseclabels;
> virSecurityDeviceLabelDefPtr *seclabels;
>
Peter
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list