Re: [libvirt] [PATCHv3 07/10] threshold: track an allocation node name for a storage source

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; +}

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);

**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)

+{ + 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;

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 */

size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels;