[RFC patch 00/17] qemu: Unlock VM object while accessing storage (in certain cases)
If a thread is blocked while holding the VM lock, other operations become impossible. This is due to the fact that both listing domains of a daemon requires locking the objects (for accessing the name/uuid/id, but also for filtering). Similarly even if a user has a virDomainPtr instace already looked up invoking an API results into trying to fetch a locked VM object instance which will simply block. This patchset is a path for being able to identify stuff which colud block and similarly to how monitor is accessed unlocking @vm while doing such operation. To show how it's used, we use it when probing disk size data. The patchset obviously needed to refactor stuff first for this to happen. Peter Krempa (17): storage_source: Don't modify @src in 'virStorageSourceUpdatePhysicalSize' virStorageSourceGetSize: Return also 'allocation' value if needed virStorageFileProbeFormatFromBuf: Allow NULL 'path' argument virStorageFileProbeGetMetadata: Allow NULL 'meta->path' virStorageSourceGetMetadataFromBuf: Refactor cleanup virStorageSourceUpdateCapacity: Factor out size probing to 'virStorageSourceProbeCapacityFromBuf' qemu: domain: Introduce 'qemuDomainStorageSourceProbeSize' qemuDomainGetBlockInfo: Rewrite to 'qemuDomainStorageSourceProbeSize' qemuDomainGetStatsOneBlock: Rewrite to 'qemuDomainStorageSourceProbeSize' qemuMigrationDstPrepareStorage: Rewrite to 'qemuDomainStorageSourceProbeSize' qemuDomainBlockResize: Rewrite to 'qemuDomainStorageSourceProbeSize' qemu: domain: Remove 'qemuDomainStorageUpdatePhysical' qemuDomainGetStatsOneBlockFallback: Rewrite using 'qemuDomainStorageSourceProbeSize' qemuDomainGetBlockInfo: Rewrite using 'qemuDomainStorageSourceProbeSize' qemu: domain: Fix coding style of domain object locking helpers qemu: domain: Introduce APIs for unlocking domain object during potentially long running operations qemuDomainStorageSourceProbeSize: Yield domain object while accessing storage src/libvirt_private.syms | 3 +- src/qemu/qemu_domain.c | 140 ++++++++++++++++++++++--- src/qemu/qemu_domain.h | 19 +++- src/qemu/qemu_driver.c | 143 +++++++------------------- src/qemu/qemu_migration.c | 5 +- src/storage_file/storage_file_probe.c | 20 +++- src/storage_file/storage_source.c | 98 +++++++++++++----- src/storage_file/storage_source.h | 16 ++- 8 files changed, 279 insertions(+), 165 deletions(-) -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Upcoming code will want to modify the call semantics of the function to unlock the domain object around it thus we ought to not modify @src directly. Rename the function to 'virStorageSourceGetSize' and return the detected size via argument instead of directly modifying it. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 +- src/storage_file/storage_source.c | 22 +++++++++++++++------- src/storage_file/storage_source.h | 8 +++++--- 4 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2391f01bc7..a7ce33b428 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1927,6 +1927,7 @@ virStorageSourceGetMetadata; virStorageSourceGetMetadataFromBuf; virStorageSourceGetMetadataFromFD; virStorageSourceGetRelativeBackingPath; +virStorageSourceGetSize; virStorageSourceInit; virStorageSourceInitAs; virStorageSourceNewFromBacking; @@ -1942,7 +1943,6 @@ virStorageSourceSupportsSecurityDriver; virStorageSourceUnlink; virStorageSourceUpdateBackingSizes; virStorageSourceUpdateCapacity; -virStorageSourceUpdatePhysicalSize; # util/viracpi.c diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 84c8645259..ef79fc72aa 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11651,7 +11651,7 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverConfig *cfg, return -1; } - ret = virStorageSourceUpdatePhysicalSize(src, fd, &sb); + ret = virStorageSourceGetSize(src, fd, &sb, &src->physical); qemuDomainStorageCloseStat(src, &fd); diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index e886433bb4..cc2d3a3198 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -585,39 +585,47 @@ virStorageSourceNewFromDataFile(virStorageSource *parent) /** + * virStorageSourceGetPhysicalSize: * @src: disk source definition structure * @fd: file descriptor * @sb: stat buffer + * @physical: filled with the physical size of @src * - * Updates src->physical depending on the actual type of storage being used. + * Fetches size information about @src depending on the actual type of storage + * being used. * To be called for domain storage source reporting as the volume code does * not set/use the 'type' field for the voldef->source.target * * Returns 0 on success, -1 on error. No libvirt errors are reported. */ int -virStorageSourceUpdatePhysicalSize(virStorageSource *src, - int fd, - struct stat const *sb) +virStorageSourceGetSize(virStorageSource *src, + int fd, + struct stat const *sb, + unsigned long long *physical) { off_t end; virStorageType actual_type = virStorageSourceGetActualType(src); + unsigned long long dummy; + + if (!physical) + physical = &dummy; switch (actual_type) { case VIR_STORAGE_TYPE_FILE: case VIR_STORAGE_TYPE_NETWORK: - src->physical = sb->st_size; + *physical = sb->st_size; break; case VIR_STORAGE_TYPE_BLOCK: if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) return -1; - src->physical = end; + *physical = end; break; case VIR_STORAGE_TYPE_DIR: - src->physical = 0; + *physical = 0; break; /* We shouldn't get VOLUME, but the switch requires all cases */ diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 63fefb6919..fd58509eb3 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -54,9 +54,11 @@ virStorageSourceChainLookupBySource(virStorageSource *chain, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int -virStorageSourceUpdatePhysicalSize(virStorageSource *src, - int fd, - struct stat const *sb); +virStorageSourceGetSize(virStorageSource *src, + int fd, + struct stat const *sb, + unsigned long long *physical) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); int virStorageSourceUpdateBackingSizes(virStorageSource *src, -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Lift the calculation from 'virStorageSourceUpdateBackingSizes'. This will be used to have a single place for these values for use in VM drivers where 'type' does reflect reality. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/storage_file/storage_source.c | 13 +++++++++++++ src/storage_file/storage_source.h | 1 + 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ef79fc72aa..e7afeb3fa0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11651,7 +11651,7 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverConfig *cfg, return -1; } - ret = virStorageSourceGetSize(src, fd, &sb, &src->physical); + ret = virStorageSourceGetSize(src, fd, &sb, NULL, &src->physical); qemuDomainStorageCloseStat(src, &fd); diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index cc2d3a3198..fb4fc48947 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -589,6 +589,7 @@ virStorageSourceNewFromDataFile(virStorageSource *parent) * @src: disk source definition structure * @fd: file descriptor * @sb: stat buffer + * @allocation: current actual disk usage * @physical: filled with the physical size of @src * * Fetches size information about @src depending on the actual type of storage @@ -602,6 +603,7 @@ int virStorageSourceGetSize(virStorageSource *src, int fd, struct stat const *sb, + unsigned long long *allocation, unsigned long long *physical) { off_t end; @@ -611,9 +613,18 @@ virStorageSourceGetSize(virStorageSource *src, if (!physical) physical = &dummy; + if (!allocation) + allocation = &dummy; + switch (actual_type) { case VIR_STORAGE_TYPE_FILE: case VIR_STORAGE_TYPE_NETWORK: +#ifndef WIN32 + *allocation = (unsigned long long) sb->st_blocks * (unsigned long long) DEV_BSIZE; +#else + *allocation = sb->st_size; +#endif + *physical = sb->st_size; break; @@ -621,10 +632,12 @@ virStorageSourceGetSize(virStorageSource *src, if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) return -1; + *allocation = end; *physical = end; break; case VIR_STORAGE_TYPE_DIR: + *allocation = 0; *physical = 0; break; diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index fd58509eb3..774dcbffee 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -57,6 +57,7 @@ int virStorageSourceGetSize(virStorageSource *src, int fd, struct stat const *sb, + unsigned long long *allocation, unsigned long long *physical) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> The 'path' argument is used only for VIR_DEBUG/VIR_WARN message. Document the function and allow passing NULL path, in which case the VIR_WARN is skipped. This will be used to allow simpler calling of metadata probing in VM stats code which will not even probe format as VM XMLs now require that format is populated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index a1f4c25b3d..ff6512db77 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -854,6 +854,14 @@ virStorageFileMatchesVersion(int versionOffset, } +/** + * virStorageFileProbeFormatFromBuf: + * @path: path of image (used only for logs; may be NULL) + * @buf: buffer to probe format from + * @buflen: length of @buf + * + * Probes the format of the image header passed via @buf. + */ static int virStorageFileProbeFormatFromBuf(const char *path, char *buf, @@ -862,7 +870,7 @@ virStorageFileProbeFormatFromBuf(const char *path, int format = VIR_STORAGE_FILE_RAW; size_t i; int possibleFormat = VIR_STORAGE_FILE_RAW; - VIR_DEBUG("path=%s, buf=%p, buflen=%zu", path, buf, buflen); + VIR_DEBUG("path=%s, buf=%p, buflen=%zu", NULLSTR(path), buf, buflen); /* First check file magic */ for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) { @@ -882,7 +890,7 @@ virStorageFileProbeFormatFromBuf(const char *path, } } - if (possibleFormat != VIR_STORAGE_FILE_RAW) + if (path && possibleFormat != VIR_STORAGE_FILE_RAW) VIR_WARN("File %s matches %s magic, but version is wrong. Please report new version to devel@lists.libvirt.org", path, virStorageFileFormatTypeToString(possibleFormat)); -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Document that the caller doesn't have to populate the field as it's used only for VIR_DEBUG and optional VIR_WARN calls. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_file_probe.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/storage_file/storage_file_probe.c b/src/storage_file/storage_file_probe.c index ff6512db77..8fa84a796e 100644 --- a/src/storage_file/storage_file_probe.c +++ b/src/storage_file/storage_file_probe.c @@ -960,8 +960,10 @@ virStorageFileGetEncryptionPayloadOffset(const struct FileEncryptionInfo *info, /* Given a header in BUF with length LEN, as parsed from the storage file * assuming it has the given FORMAT, populate information into META * with information about the file and its backing store. Return format - * of the backing store as BACKING_FORMAT. PATH and FORMAT have to be - * pre-populated in META. + * of the backing store as BACKING_FORMAT. + * + * 'meta->format' and 'meta->path' are expected to be pre-populated although + * 'meta->path' may be NULL as the value is used only in debug messages. * * Note that this function may be called repeatedly on @meta, so it must * clean up any existing allocated memory which would be overwritten. @@ -974,7 +976,7 @@ virStorageFileProbeGetMetadata(virStorageSource *meta, size_t i; VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", - meta->path, buf, len, meta->format); + NULLSTR(meta->path), buf, len, meta->format); if (meta->format == VIR_STORAGE_FILE_AUTO) meta->format = virStorageFileProbeFormatFromBuf(meta->path, buf, len); -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Use automatic freeing for 'ret' and stop checking return value from 'virStorageSourceMetadataNew' which can't fail. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage_file/storage_source.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index fb4fc48947..651822eb65 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -119,17 +119,12 @@ virStorageSourceGetMetadataFromBuf(const char *path, size_t len, int format) { - virStorageSource *ret = NULL; + g_autoptr(virStorageSource) ret = virStorageSourceMetadataNew(path, format); - if (!(ret = virStorageSourceMetadataNew(path, format))) + if (virStorageFileProbeGetMetadata(ret, buf, len) < 0) return NULL; - if (virStorageFileProbeGetMetadata(ret, buf, len) < 0) { - virObjectUnref(ret); - return NULL; - } - - return ret; + return g_steal_pointer(&ret); } -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Extract the logic to probe the image format header to a separate caller which will not touch the virStorageSource. Note that this does no longer update 'encryption->payload_offset' of the source when called from 'virStorageSourceUpdateCapacity', but that value is not actually used elsewhere. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/storage_file/storage_source.c | 52 ++++++++++++++++++++++++------- src/storage_file/storage_source.h | 7 +++++ 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a7ce33b428..8d5b4fb875 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1933,6 +1933,7 @@ virStorageSourceInitAs; virStorageSourceNewFromBacking; virStorageSourceNewFromBackingAbsolute; virStorageSourceParseRBDColonString; +virStorageSourceProbeCapacityFromBuf; virStorageSourceRead; virStorageSourceReportBrokenChain; virStorageSourceStat; diff --git a/src/storage_file/storage_source.c b/src/storage_file/storage_source.c index 651822eb65..1108865b03 100644 --- a/src/storage_file/storage_source.c +++ b/src/storage_file/storage_source.c @@ -731,7 +731,6 @@ virStorageSourceUpdateCapacity(virStorageSource *src, ssize_t len) { int format = src->format; - g_autoptr(virStorageSource) meta = NULL; /* Raw files: capacity is physical size. For all other files: if * the metadata has a capacity, use that, otherwise fall back to @@ -743,19 +742,48 @@ virStorageSourceUpdateCapacity(virStorageSource *src, return -1; } - if (format == VIR_STORAGE_FILE_RAW && !src->encryption) { - src->capacity = src->physical; - } else if ((meta = virStorageSourceGetMetadataFromBuf(src->path, buf, - len, format))) { - src->capacity = meta->capacity ? meta->capacity : src->physical; - if (src->encryption && meta->encryption) - src->encryption->payload_offset = meta->encryption->payload_offset; - } else { + src->capacity = src->physical; + + if (src->format == VIR_STORAGE_FILE_RAW && !src->encryption) + return 0; + + if (virStorageSourceProbeCapacityFromBuf(format, buf, len, &src->capacity)) return -1; - } - if (src->encryption && src->encryption->payload_offset != -1) - src->capacity -= src->encryption->payload_offset * 512; + return 0; +} + + +/** + * virStorageSourceProbeCapacityFromBuf: + * @format: format of the image to probe + * @buf: buffer containing the header of the image + * @buflen: length of @buf + * @capacity: filled with the probed capacity; caller must pre-fill it with + * physical size of the image in case when the image header doesn't + * contain lenght + * + * Probes the image header in @buf for capacity recorded in the header and fills + * it into @capacity. Calers must pre-fill @capacity with the physical size + * of the backing storage as some formats don't contain capacity information + * in the header. It doesn't make sense to call this for 'raw' files. + */ +int +virStorageSourceProbeCapacityFromBuf(virStorageFileFormat format, + char *buf, + ssize_t buflen, + unsigned long long *capacity) +{ + g_autoptr(virStorageSource) meta = virStorageSourceMetadataNew(NULL, format); + + if (virStorageFileProbeGetMetadata(meta, buf, buflen) < 0) + return -1; + + if (meta->capacity > 0) + *capacity = meta->capacity; + + if (meta->encryption && meta->encryption->payload_offset != -1) + *capacity -= meta->encryption->payload_offset * 512; return 0; } diff --git a/src/storage_file/storage_source.h b/src/storage_file/storage_source.h index 774dcbffee..c8cd244ca1 100644 --- a/src/storage_file/storage_source.h +++ b/src/storage_file/storage_source.h @@ -71,6 +71,13 @@ virStorageSourceUpdateCapacity(virStorageSource *src, char *buf, ssize_t len); +int +virStorageSourceProbeCapacityFromBuf(virStorageFileFormat format, + char *buf, + ssize_t buflen, + unsigned long long *capacity) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4); + int virStorageSourceNewFromBacking(virStorageSource *parent, virStorageSource **backing); -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Introduce a new helper for querying disk sizes called 'qemuDomainStorageSourceProbeSize'. It combines what 'qemuDomainStorageUpdatePhysical' and 'qemuStorageLimitsRefresh' query, but doesn't update the virStorageSource struct. Instead the fetched size is returned via arguments as pointers. The idea of this helper is to centralize the two functions it replaces into a single place with more reasonable semantics w.r.t access of @src so that it can be later used with unlocked domain object to avoid cases where stuck storage would make the domain object stuck too. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 69 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 9 ++++++ 2 files changed, 78 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e7afeb3fa0..1792b0e9d0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -53,6 +53,7 @@ #include "virnetdevbandwidth.h" #include "virstoragefile.h" #include "storage_source.h" +#include "storage_file_probe.h" #include "virstring.h" #include "virprocess.h" #include "vircrypto.h" @@ -11659,6 +11660,74 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverConfig *cfg, } +/** + * qemuDomainStorageSourceProbeSize: + * @cfg: qemu driver configuration object + * @vm: domain object + * @src: storage source to update + * @allocation: filled by current allocation (used) size in bytes (may be NULL) + * @capacity: filled by (guest visible) capacity of @src in bytes (may be NULL) + * @physical_ret: filed by size of the underlying storage of @src in bytes (may be NULL) + * + * Fetches size information of @src and fills @allocation/@capacity/@physical + * (whichever is non-NULL). @src and the fields with same names in that structure + * are not used/touched. + * + * Returns 0 on success, -1 on error. No libvirt errors are reported. + */ +int +qemuDomainStorageSourceProbeSize(virQEMUDriverConfig *cfg, + virDomainObj *vm, + virStorageSource *src, + unsigned long long *allocation, + unsigned long long *capacity, + unsigned long long *physical_ret) +{ + int rc; + int fd = -1; + struct stat sb; + g_autofree char *buf = NULL; + ssize_t buflen = 0; + unsigned long long physical = 0; + + if (virStorageSourceIsEmpty(src)) + return 0; + + if (qemuDomainStorageOpenStat(cfg, vm, src, &fd, &sb, true) <= 0) + return -1; + + rc = virStorageSourceGetSize(src, fd, &sb, allocation, &physical); + + if (capacity && !qemuBlockStorageSourceIsRaw(src)) { + if (virStorageSourceIsLocalStorage(src)) { + buflen = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf); + } else { + if ((buflen = virStorageSourceRead(src, 0, VIR_STORAGE_MAX_HEADER, &buf)) < 0) + virResetLastError(); + } + } + + qemuDomainStorageCloseStat(src, &fd); + + if (rc < 0 || buflen < 0) + return -1; + + if (capacity) { + *capacity = physical; + + if (!qemuBlockStorageSourceIsRaw(src)) { + if (virStorageSourceProbeCapacityFromBuf(src->format, buf, buflen, capacity)) + return -1; + } + } + + if (physical_ret) + *physical_ret = physical; + + return 0; +} + + /** * qemuDomainCheckCPU: * @arch: CPU architecture diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b321a64e96..9bb8d0f97d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1176,6 +1176,15 @@ int qemuDomainStorageUpdatePhysical(virQEMUDriverConfig *cfg, virDomainObj *vm, virStorageSource *src); + +int +qemuDomainStorageSourceProbeSize(virQEMUDriverConfig *cfg, + virDomainObj *vm, + virStorageSource *src, + unsigned long long *allocation, + unsigned long long *capacity, + unsigned long long *physical_ret); + virCPUCompareResult qemuDomainCheckCPU(virArch arch, virDomainVirtType virtType, -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Use 'qemuDomainStorageSourceProbeSize' instead of 'qemuDomainStorageUpdatePhysical'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bcafacfb60..e37c852c98 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10803,12 +10803,14 @@ qemuDomainGetBlockInfo(virDomainPtr dom, * value for this API. NB: May still be 0 for block. */ if (entry->physical == 0 || info->allocation == 0 || info->allocation == entry->physical) { + unsigned long long physical; + if (info->allocation == 0) info->allocation = entry->physical; - if (qemuDomainStorageUpdatePhysical(cfg, vm, disk->src) == 0) { - VIR_DEBUG("updating physical disk size to '%llu'", disk->src->physical); - info->physical = disk->src->physical; + if (qemuDomainStorageSourceProbeSize(cfg, vm, disk->src, NULL, NULL, &physical) == 0) { + VIR_DEBUG("updating physical disk size to '%llu'", physical); + info->physical = physical; } else { info->physical = entry->physical; } -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Use 'qemuDomainStorageSourceProbeSize' instead of 'qemuDomainStorageUpdatePhysical'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e37c852c98..253727129c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17830,8 +17830,10 @@ qemuDomainGetStatsOneBlock(virQEMUDriverConfig *cfg, virTypedParamListAddULLong(params, entry->physical, VIR_DOMAIN_STATS_BLOCK_PREFIX "%zu" VIR_DOMAIN_STATS_BLOCK_SUFFIX_PHYSICAL, block_idx); } else { - if (qemuDomainStorageUpdatePhysical(cfg, dom, src) == 0) { - virTypedParamListAddULLong(params, src->physical, + unsigned long long physical = 0; + + if (qemuDomainStorageSourceProbeSize(cfg, dom, src, NULL, NULL, &physical) == 0) { + virTypedParamListAddULLong(params, physical, VIR_DOMAIN_STATS_BLOCK_PREFIX "%zu" VIR_DOMAIN_STATS_BLOCK_SUFFIX_PHYSICAL, block_idx); } } -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Use 'qemuDomainStorageSourceProbeSize' instead of 'qemuDomainStorageUpdatePhysical'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 4a43ab83b0..67955aa762 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -482,9 +482,10 @@ qemuMigrationDstPrepareStorage(virDomainObj *vm, qemuDomainObjPrivate *priv = vm->privateData; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + unsigned long long physical = 0; - if (qemuDomainStorageUpdatePhysical(cfg, vm, disk->src) == 0 && - disk->src->physical > nbd->disks[i].capacity) { + if (qemuDomainStorageSourceProbeSize(cfg, vm, disk->src, NULL, NULL, &physical) == 0 && + physical > nbd->disks[i].capacity) { disk->src->sliceStorage = g_new0(virStorageSourceSlice, 1); disk->src->sliceStorage->size = nbd->disks[i].capacity; diskPriv->migrationslice = true; -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Use 'qemuDomainStorageSourceProbeSize' instead of 'qemuDomainStorageUpdatePhysical'. Since 'size_src' was used just to fetch the 'physical' field, the patch localizes the scope of the pointer and uses the 'physical' value instead. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 253727129c..1b14c6d42e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9547,7 +9547,7 @@ qemuDomainBlockResize(virDomainPtr dom, const char *nodename = NULL; virDomainDiskDef *disk = NULL; virDomainDiskDef *persistDisk = NULL; - virStorageSource *size_src = NULL; + unsigned long long physical = 0; virCheckFlags(VIR_DOMAIN_BLOCK_RESIZE_BYTES | VIR_DOMAIN_BLOCK_RESIZE_CAPACITY | @@ -9606,6 +9606,7 @@ qemuDomainBlockResize(virDomainPtr dom, */ if ((flags & VIR_DOMAIN_BLOCK_RESIZE_CAPACITY) || disk->src->sliceStorage) { g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); + virStorageSource *size_src = NULL; if (disk->src->dataFileStore) size_src = disk->src->dataFileStore; @@ -9622,7 +9623,7 @@ qemuDomainBlockResize(virDomainPtr dom, } if (virStorageSourceIsBlockLocal(size_src)) { - if (qemuDomainStorageUpdatePhysical(cfg, vm, size_src) < 0) { + if (qemuDomainStorageSourceProbeSize(cfg, vm, size_src, NULL, NULL, &physical) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, _("failed to update capacity of '%1$s'"), size_src->path); goto endjob; @@ -9632,11 +9633,11 @@ qemuDomainBlockResize(virDomainPtr dom, if (flags & VIR_DOMAIN_BLOCK_RESIZE_CAPACITY) { if (size == 0) { - size = size_src->physical; - } else if (size != size_src->physical) { + size = physical; + } else if (size != physical) { virReportError(VIR_ERR_INVALID_ARG, _("Requested resize to '%1$llu' but device size is '%2$llu'"), - size, size_src->physical); + size, physical); goto endjob; } } @@ -9676,10 +9677,10 @@ qemuDomainBlockResize(virDomainPtr dom, /* trying to resize a block device to a size not equal to the actual * size of the block device will cause qemu to fail */ if (virStorageSourceIsBlockLocal(disk->src) && - disk->src->physical != size) { + physical != size) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("block device backed disk must be resized to its actual size '%1$llu'"), - disk->src->physical); + physical); goto endjob; } -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> All uses were replaced by 'qemuDomainStorageSourceProbeSize'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 38 -------------------------------------- src/qemu/qemu_domain.h | 4 ---- 2 files changed, 42 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1792b0e9d0..40b59a4f31 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11622,44 +11622,6 @@ qemuDomainStorageCloseStat(virStorageSource *src, } -/** - * qemuDomainStorageUpdatePhysical: - * @cfg: qemu driver configuration object - * @vm: domain object - * @src: storage source to update - * - * Update the physical size of the disk by reading the actual size of the image - * on disk. - * - * Returns 0 on successful update and -1 otherwise (some uncommon errors may be - * reported but are reset (thus only logged)). - */ -int -qemuDomainStorageUpdatePhysical(virQEMUDriverConfig *cfg, - virDomainObj *vm, - virStorageSource *src) -{ - int ret; - int fd = -1; - struct stat sb; - - if (virStorageSourceIsEmpty(src)) - return 0; - - if ((ret = qemuDomainStorageOpenStat(cfg, vm, src, &fd, &sb, true)) <= 0) { - if (ret < 0) - virResetLastError(); - return -1; - } - - ret = virStorageSourceGetSize(src, fd, &sb, NULL, &src->physical); - - qemuDomainStorageCloseStat(src, &fd); - - return ret; -} - - /** * qemuDomainStorageSourceProbeSize: * @cfg: qemu driver configuration object diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 9bb8d0f97d..ec1c2a4779 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -1172,10 +1172,6 @@ qemuDomainStorageOpenStat(virQEMUDriverConfig *cfg, void qemuDomainStorageCloseStat(virStorageSource *src, int *fd); -int -qemuDomainStorageUpdatePhysical(virQEMUDriverConfig *cfg, - virDomainObj *vm, - virStorageSource *src); int qemuDomainStorageSourceProbeSize(virQEMUDriverConfig *cfg, -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Replace use of 'qemuStorageLimitsRefresh' by 'qemuDomainStorageSourceProbeSize'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1b14c6d42e..c6e35c58a4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17773,25 +17773,28 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverConfig *cfg, virStorageSource *src, size_t block_idx) { + unsigned long long allocation = 0; + unsigned long long capacity = 0; + unsigned long long physical = 0; + if (virStorageSourceIsEmpty(src) || virStorageSourceIsFD(src)) return; - if (qemuStorageLimitsRefresh(cfg, dom, src, true) <= 0) { - virResetLastError(); + if (qemuDomainStorageSourceProbeSize(cfg, dom, src, &allocation, + &capacity, &physical) < 0) return; - } - if (src->allocation) - virTypedParamListAddULLong(params, src->allocation, + if (allocation) + virTypedParamListAddULLong(params, allocation, VIR_DOMAIN_STATS_BLOCK_PREFIX "%zu" VIR_DOMAIN_STATS_BLOCK_SUFFIX_ALLOCATION, block_idx); - if (src->capacity) - virTypedParamListAddULLong(params, src->capacity, + if (capacity) + virTypedParamListAddULLong(params, capacity, VIR_DOMAIN_STATS_BLOCK_PREFIX "%zu" VIR_DOMAIN_STATS_BLOCK_SUFFIX_CAPACITY, block_idx); - if (src->physical) - virTypedParamListAddULLong(params, src->physical, + if (physical) + virTypedParamListAddULLong(params, physical, VIR_DOMAIN_STATS_BLOCK_PREFIX "%zu" VIR_DOMAIN_STATS_BLOCK_SUFFIX_PHYSICAL, block_idx); } -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Replace use of 'qemuStorageLimitsRefresh' by 'qemuDomainStorageSourceProbeSize'. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 93 ++++-------------------------------------- 1 file changed, 7 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c6e35c58a4..13988d6568 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10633,87 +10633,6 @@ qemuDomainMemoryPeek(virDomainPtr dom, } -/** - * @cfg: driver configuration data - * @vm: domain object - * @src: storage source data - * @skipInaccessible: Suppress reporting of common errors when accessing @src - * - * Refresh the capacity and allocation limits of a given storage source. - * - * Assumes that the caller has already obtained a domain job and only - * called for an offline domain. Being offline is particularly important - * since reading a file while qemu is writing it risks the reader seeing - * bogus data or avoiding opening a file in order to get stat data. - * - * We always want to check current on-disk statistics (as users have been - * known to change offline images behind our backs). - * - * For read-only disks, nothing should be changing unless the user has - * requested a block-commit action. For read-write disks, we know some - * special cases: capacity should not change without a block-resize (where - * capacity is the only stat that requires reading a file, and even then, - * only for non-raw files); and physical size of a raw image or of a - * block device should likewise not be changing without block-resize. - * On the other hand, allocation of a raw file can change (if the file - * is sparse, but the amount of sparseness changes due to writes or - * punching holes), and physical size of a non-raw file can change. - * - * Returns 1 if @src was successfully updated, 0 if @src can't be opened and - * @skipInaccessible is true (no errors are reported) or -1 otherwise (errors - * are reported). - */ -static int -qemuStorageLimitsRefresh(virQEMUDriverConfig *cfg, - virDomainObj *vm, - virStorageSource *src, - bool skipInaccessible) -{ - int rc; - int ret = -1; - int fd = -1; - struct stat sb; - g_autofree char *buf = NULL; - ssize_t len; - - if ((rc = qemuDomainStorageOpenStat(cfg, vm, src, &fd, &sb, - skipInaccessible)) <= 0) - return rc; - - if (virStorageSourceIsLocalStorage(src)) { - if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { - virReportSystemError(errno, _("cannot read header '%1$s'"), - src->path); - goto cleanup; - } - } else { - if ((len = virStorageSourceRead(src, 0, VIR_STORAGE_MAX_HEADER, &buf)) < 0) - goto cleanup; - } - - if (virStorageSourceUpdateBackingSizes(src, fd, &sb) < 0) - goto cleanup; - - if (virStorageSourceUpdateCapacity(src, buf, len) < 0) - goto cleanup; - - /* If guest is not using raw disk format and is on a host block - * device, then leave the value unspecified, so caller knows to - * query the highest allocated extent from QEMU - */ - if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK && - !qemuBlockStorageSourceIsRaw(src) && - S_ISBLK(sb.st_mode)) - src->allocation = 0; - - ret = 1; - - cleanup: - qemuDomainStorageCloseStat(src, &fd); - return ret; -} - - static int qemuDomainGetBlockInfo(virDomainPtr dom, const char *path, @@ -10767,12 +10686,14 @@ qemuDomainGetBlockInfo(virDomainPtr dom, /* for inactive domains we have to peek into the files */ if (!virDomainObjIsActive(vm)) { - if ((qemuStorageLimitsRefresh(cfg, vm, disk->src, false)) < 0) + if (qemuDomainStorageSourceProbeSize(cfg, vm, disk->src, + &info->allocation, + &info->capacity, + &info->physical) < 0) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("failed to fetch size data for '%1$s'"), path); goto endjob; - - info->capacity = disk->src->capacity; - info->allocation = disk->src->allocation; - info->physical = disk->src->physical; + } ret = 0; goto endjob; -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Fix coding style around the monitor/remote helpers. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 40b59a4f31..9368ab916d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5157,6 +5157,7 @@ qemuDomainObjEnterMonitorInternal(virDomainObj *obj, return 0; } + /* obj must NOT be locked before calling * * Should be paired with an earlier qemuDomainObjEnterMonitor() call @@ -5187,7 +5188,9 @@ qemuDomainObjExitMonitor(virDomainObj *obj) virDomainObjEndJob(obj); } -void qemuDomainObjEnterMonitor(virDomainObj *obj) + +void +qemuDomainObjEnterMonitor(virDomainObj *obj) { ignore_value(qemuDomainObjEnterMonitorInternal(obj, VIR_ASYNC_JOB_NONE)); } @@ -5255,7 +5258,9 @@ qemuDomainObjExitAgent(virDomainObj *obj, qemuAgent *agent) agent, obj, obj->def->name); } -void qemuDomainObjEnterRemote(virDomainObj *obj) + +void +qemuDomainObjEnterRemote(virDomainObj *obj) { VIR_DEBUG("Entering remote (vm=%p name=%s)", obj, obj->def->name); -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Similarly to how we enter monitor context, so that the VM object is unlocked while waiting for qemu, there are non-monitor operations where we might want to yield the domain object (while holding a JOB on the domain object) so that other threads are not blocked e.g. on looking upi a list of active domains. This basically copies the helpers to enter monitor minus locking of the monitor object. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 65 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 8 ++++++ 2 files changed, 73 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9368ab916d..ea591e4c95 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5287,6 +5287,71 @@ qemuDomainObjExitRemote(virDomainObj *obj, } +/** + * qemuDomainObjEnterOperation: + * @obj: domain object + * + * @obj must be locked when this function is called. + * + * Enter a potentially long running operation on @vm. Unlocks @vm and allows + * other threads to e.g. look up the object. + * + * This requires that virDomainObjBeginJob() was already called on @vm and + * checked that the VM is still active. + * + * To be followed with qemuDomainObjExitOperation() once complete + */ +int +qemuDomainObjEnterOperation(virDomainObj *obj, + virDomainAsyncJob asyncJob) +{ + if (asyncJob != VIR_ASYNC_JOB_NONE) { + int ret; + if ((ret = virDomainObjBeginNestedJob(obj, asyncJob)) < 0) + return ret; + if (!virDomainObjIsActive(obj)) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("domain is no longer running")); + virDomainObjEndJob(obj); + return -1; + } + } else if (obj->job->asyncOwner == virThreadSelfID()) { + VIR_WARN("This thread seems to be the async job owner; entering operation without asking for a nested job is dangerous"); + } else if (obj->job->owner != virThreadSelfID()) { + VIR_WARN("Entering an operation without owning a job. Job %s owner %s (%llu)", + virDomainJobTypeToString(obj->job->active), + obj->job->ownerAPI, obj->job->owner); + } + + VIR_DEBUG("Entering operation (vm=%p name=%s)", + obj, obj->def->name); + virObjectUnlock(obj); + + return 0; +} + + +/** + * qemuDomainObjExitOperation: + * @obj: domain object + * + * @obj must be *UNLOCKED* when this is called. + * + * Enter normal context on return from the long running operation started by + * qemuDomainObjEnterOperation(). + */ +void +qemuDomainObjExitOperation(virDomainObj *obj) +{ + virObjectLock(obj); + VIR_DEBUG("Exited operation (vm=%p name=%s)", + obj, obj->def->name); + + if (obj->job->active == VIR_JOB_ASYNC_NESTED) + virDomainObjEndJob(obj); +} + + static virDomainDef * qemuDomainDefFromXML(virQEMUDriver *driver, virQEMUCaps *qemuCaps, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ec1c2a4779..ad44e0cc2a 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -624,6 +624,14 @@ int qemuDomainObjExitRemote(virDomainObj *obj, bool checkActive) ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; + +int qemuDomainObjEnterOperation(virDomainObj *obj, + virDomainAsyncJob asyncJob) + ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; + +void qemuDomainObjExitOperation(virDomainObj *obj) + ATTRIBUTE_NONNULL(1); + virDomainDef *qemuDomainDefCopy(virQEMUDriver *driver, virQEMUCaps *qemuCaps, virDomainDef *src, -- 2.54.0
From: Peter Krempa <pkrempa@redhat.com> Use 'qemuDomainObjEnterOperation' to unlock the domain object while accessing storage. In case when storage blocks this allows other jobs to fetch the domain object and get queued up. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ea591e4c95..d17f0dc0d6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -11725,6 +11725,9 @@ qemuDomainStorageSourceProbeSize(virQEMUDriverConfig *cfg, if (virStorageSourceIsEmpty(src)) return 0; + if (qemuDomainObjEnterOperation(vm, VIR_ASYNC_JOB_NONE) < 0) + return -1; + if (qemuDomainStorageOpenStat(cfg, vm, src, &fd, &sb, true) <= 0) return -1; @@ -11741,6 +11744,8 @@ qemuDomainStorageSourceProbeSize(virQEMUDriverConfig *cfg, qemuDomainStorageCloseStat(src, &fd); + qemuDomainObjExitOperation(vm); + if (rc < 0 || buflen < 0) return -1; -- 2.54.0
participants (1)
-
Peter Krempa