[libvirt] [RFC: PATCH 0/2] Display allocation during dumpxml
I'm still working on code to populate the latest numbers for each disk of a domain, including getting numbers for offline domains, but have confirmed that with these two patches alone I'm able to see <capacity> and <allocation> numbers for block volumes of live domains (thanks to how we populate backing chain information). So while there are more patches to come, I'd like to get review started on my proposed API addition. Eric Blake (2): dumpxml: add flag to virDomainGetXMLDesc dumpxml: prepare to output block info docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 16 +++++++++++++++- src/libvirt.c | 15 +++++++++++---- src/util/virstoragefile.h | 3 ++- tools/virsh-domain.c | 6 ++++++ tools/virsh.pod | 6 ++++-- 7 files changed, 61 insertions(+), 8 deletions(-) -- 1.9.3
The information in virDomainGetBlockInfo() is important for clients that use qcow2 format on LVM block devices - by tracking the allocation in relation to the physical size, management can tell if a disk needs to be expanded before the guest (file system contents) and/or qemu (copy-on-write differing more from a backing file) runs out of space. Normally, only the active layer matters, but during a block commit operation, the allocation of the backing file ALSO grows, and management would like to track that growth. Right now, virDomainGetBlockInfo() can only convey information about the active layer of a disk, via a single API call per disk. It can also be easily extended to support "vda[1]" notation that we recently added for blockcommit and friends, to get similar information about a backing element; but that still implies one call per layer, which adds up to a lot of overhead. This API addition will make it possible to grab this information for ALL guest disks in a single call, by letting the user request additional information about each disk in the backing chain to be output as part of the domain XML. My ultimate goal is to have this flag and virStorageVolGetXMLDesc() expose the same bits of information about a given storage volume (there are slight incompatiblities in the XML naming that we'll have to keep for back-compat sake, but the idea is to get all the information out there). * include/libvirt/libvirt.h.in (virDomainXMLFlags): Add new flag. * src/libvirt.c (virDomainGetXMLDesc): Document it. (virDomainSaveImageGetXMLDesc, virDomainSnapshotGetXMLDesc): For now, mention the flag won't help here. * tools/virsh-domain.c (cmdDumpXML): Add new flag. * tools/virsh.pod (dumpxml): Document it. Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 15 +++++++++++---- tools/virsh-domain.c | 6 ++++++ tools/virsh.pod | 6 ++++-- 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c2f9d26..40f4e0c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -2082,6 +2082,7 @@ typedef enum { VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */ VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements according to host CPU */ VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */ + VIR_DOMAIN_XML_BLOCK_INFO = (1 << 4), /* include storage volume information about each disk source */ } virDomainXMLFlags; char * virDomainGetXMLDesc (virDomainPtr domain, diff --git a/src/libvirt.c b/src/libvirt.c index f7e5a37..1020cc5 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -2806,8 +2806,9 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml, * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * connections. For this API, @flags should not contain + * VIR_DOMAIN_XML_INACTIVE, VIR_DOMAIN_XML_UPDATE_CPU, or + * VIR_DOMAIN_XML_BLOCK_INFO. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of * error. The caller must free() the returned value. @@ -4354,6 +4355,11 @@ virDomainGetControlInfo(virDomainPtr domain, * describing CPU capabilities is modified to match actual * capabilities of the host. * + * If @flags contains VIR_DOMAIN_XML_BLOCK_INFO, the listing for each + * <disk> device will contain additional information such as capacity + * and allocation, similar to what is displayed by virStorageVolGetXMLDesc(), + * and avoiding the need to call virDomainGetBlockInfo() separately. + * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. */ @@ -18507,8 +18513,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain, * * No security-sensitive data will be included unless @flags contains * VIR_DOMAIN_XML_SECURE; this flag is rejected on read-only - * connections. For this API, @flags should not contain either - * VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_XML_UPDATE_CPU. + * connections. For this API, @flags should not contain + * VIR_DOMAIN_XML_INACTIVE, VIR_DOMAIN_XML_UPDATE_CPU, or + * VIR_DOMAIN_XML_BLOCK_INFO. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 435d045..8f97c48 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8838,6 +8838,10 @@ static const vshCmdOptDef opts_dumpxml[] = { .type = VSH_OT_BOOL, .help = N_("provide XML suitable for migrations") }, + {.name = "block-info", + .type = VSH_OT_BOOL, + .help = N_("include additional block info for each <disk> in XML dump") + }, {.name = NULL} }; @@ -8861,6 +8865,8 @@ cmdDumpXML(vshControl *ctl, const vshCmd *cmd) flags |= VIR_DOMAIN_XML_UPDATE_CPU; if (migratable) flags |= VIR_DOMAIN_XML_MIGRATABLE; + if (vshCommandOptBool(cmd, "block-info")) + flags |= VIR_DOMAIN_XML_BLOCK_INFO; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; diff --git a/tools/virsh.pod b/tools/virsh.pod index b139715..fc45f3c 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1220,7 +1220,7 @@ NOTE: Some hypervisors may require the user to manually ensure proper permissions on file and path specified by argument I<corefilepath>. =item B<dumpxml> I<domain> [I<--inactive>] [I<--security-info>] -[I<--update-cpu>] [I<--migratable>] +[I<--update-cpu>] [I<--migratable>] [I<--block-info>] Output the domain information as an XML dump to stdout, this format can be used by the B<create> command. Additional options affecting the XML dump may be @@ -1231,7 +1231,9 @@ in the XML dump. I<--update-cpu> updates domain CPU requirements according to host CPU. With I<--migratable> one can request an XML that is suitable for migrations, i.e., compatible with older libvirt releases and possibly amended with internal run-time options. This option may automatically enable other -options (I<--update-cpu>, I<--security-info>, ...) as necessary. +options (I<--update-cpu>, I<--security-info>, ...) as necessary. Using +I<--block-info> will add additional information about each <disk> source, +comparable to what B<vol-dumpxml> or B<domblkinfo> would show. =item B<edit> I<domain> -- 1.9.3
On 17/09/14 08:43 -0600, Eric Blake wrote:
+ * If @flags contains VIR_DOMAIN_XML_BLOCK_INFO, the listing for each + * <disk> device will contain additional information such as capacity + * and allocation, similar to what is displayed by virStorageVolGetXMLDesc(), + * and avoiding the need to call virDomainGetBlockInfo() separately. + *
Eric: We decided to go with this extra flag since gathering the block info was seen as a potentially expensive operation which users would want to selectively enable. Do you have any thoughts on how expensive this would be in practice? We are considering enabling it unconditionally in oVirt and am weighing a decision between A) Simplicity and Cacheability vs. B) Performance. Thanks. -- Adam Litke
On 10/23/2014 07:52 AM, Adam Litke wrote:
On 17/09/14 08:43 -0600, Eric Blake wrote:
+ * If @flags contains VIR_DOMAIN_XML_BLOCK_INFO, the listing for each + * <disk> device will contain additional information such as capacity + * and allocation, similar to what is displayed by virStorageVolGetXMLDesc(), + * and avoiding the need to call virDomainGetBlockInfo() separately. + *
Eric: We decided to go with this extra flag since gathering the block info was seen as a potentially expensive operation which users would want to selectively enable. Do you have any thoughts on how expensive this would be in practice? We are considering enabling it unconditionally in oVirt and am weighing a decision between A) Simplicity and Cacheability vs. B) Performance.
I just posted a v2 patch. The extra expense is that I have to obtain a job lock in order to call into the qemu monitor. Thankfully, it's only a read lock (so it can be done even during a long-running operation like a migration), but obtaining that lock is not instant. It's still probably not noticeable in the long run, but avoiding the flag avoids the need for the job lock in the first place. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds the common code for outputting drive sizing information, when a user has requested the new API flag from the previous patch. * src/util/virstoragefile.h (_virStorageSource): Add physical, to mirror virDomainBlockInfo. * docs/schemas/domaincommon.rng (storageSourceExtra): New define. * src/conf/domain_conf.c (virDomainDiskDefFormat): Output sizing when flag is set. (DUMPXML_FLAGS): Add new flag. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/schemas/domaincommon.rng | 22 ++++++++++++++++++++++ src/conf/domain_conf.c | 16 +++++++++++++++- src/util/virstoragefile.h | 3 ++- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c600f22..dd874fc 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1292,6 +1292,28 @@ </element> </define> + <define name='storageSourceExtra' combine='choice'> + <!-- Override of storagecommon.rng to allow domain disks to list + extras without impacting snapshot disks --> + <interleave> + <optional> + <element name="capacity"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="allocation"> + <ref name="scaledInteger"/> + </element> + </optional> + <optional> + <element name="physical"> + <ref name="scaledInteger"/> + </element> + </optional> + </interleave> + </define> + <define name="diskBackingChain"> <choice> <ref name="diskBackingStore"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ccec1c..3b54619 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -108,7 +108,8 @@ typedef enum { (VIR_DOMAIN_XML_SECURE | \ VIR_DOMAIN_XML_INACTIVE | \ VIR_DOMAIN_XML_UPDATE_CPU | \ - VIR_DOMAIN_XML_MIGRATABLE) + VIR_DOMAIN_XML_MIGRATABLE | \ + VIR_DOMAIN_XML_BLOCK_INFO) verify(((VIR_DOMAIN_XML_INTERNAL_STATUS | VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET | @@ -15887,6 +15888,19 @@ virDomainDiskDefFormat(virBufferPtr buf, flags) < 0) return -1; + if (flags & VIR_DOMAIN_XML_BLOCK_INFO) { + if (def->src->capacity) + virBufferAsprintf(buf, "<capacity unit='bytes'>%llu</capacity>\n", + def->src->capacity); + if (def->src->allocation) + virBufferAsprintf(buf, + "<allocation unit='bytes'>%llu</allocation>\n", + def->src->allocation); + if (def->src->physical) + virBufferAsprintf(buf, "<physical unit='bytes'>%llu</physical>\n", + def->src->physical); + } + /* Don't format backingStore to inactive XMLs until the code for * persistent storage of backing chains is ready. */ if (!(flags & VIR_DOMAIN_XML_INACTIVE) && diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2583e10..681e50a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -252,8 +252,9 @@ struct _virStorageSource { virStoragePermsPtr perms; virStorageTimestampsPtr timestamps; - unsigned long long allocation; /* in bytes, 0 if unknown */ 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 */ size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; -- 1.9.3
participants (2)
-
Adam Litke -
Eric Blake