[libvirt] [PATCH 0/7] Adjustments for storage/qemu volume/file backing size settings
A bit of a mix between fixing some bugs and merging some code between the block info/stats fetching of backing store data and storage backend data for volume allocation, capacity, and physical sizes. Patch 1 just adjusts some comments Patches 2-3 are code motion Patch 4 alters how domain/qemu will open the backing store from an open to a qemuFileOpen and it allows for fetching of more physical sizes (the code only gets the block backing stores now) Patch 5 shares the setting of the various size values between storage driver and domain/qemu Patch 6 shares the recalculation of the capacity value (it may not be necessary for patch 5, but I left it there since there's multiple other paths to the VolTargetInfoFD API). Patch 7 fixes an issue with displaying the allocation value of a qcow2 backed file for the GetBlockInfo API (the results will match volume code now). John Ferlan (7): qemu: Clean up description for qemuStorageLimitsRefresh qemu: Add helpers to handle stat data for qemuStorageLimitsRefresh qemu: Introduce helper qemuDomainStorageUpdatePhysical util: Introduce virStorageSourceUpdatePhysicalSize util: Introduce virStorageSourceUpdateBackingSizes util: Introduce virStorageSourceUpdateCapacity qemu: Fix GetBlockInfo setting allocation from wr_highest_offset src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 237 ++++++++++++++++++++++++------------------ src/storage/storage_backend.c | 41 +------- src/util/virstoragefile.c | 173 ++++++++++++++++++++++++++---- src/util/virstoragefile.h | 15 ++- 5 files changed, 304 insertions(+), 167 deletions(-) -- 2.7.4
Originally added by commit id '89646e69' prior to commit id '15fa84ac' and '71d2c172' which ensured that qemuStorageLimitsRefresh was only called for inactive domains. Adjust the comment describing the need for FIXME and move all the text to the function description. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 51 +++++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3517aa2..653fe5d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11532,9 +11532,34 @@ qemuDomainMemoryPeek(virDomainPtr dom, } -/* Refresh the capacity and allocation limits of a given storage - * source. Assumes that the caller has already obtained a domain - * job. */ +/** + * @driver: qemu driver data + * @cfg: driver configuration data + * @vm: domain object + * @src: storage source data + * + * 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 0 on success, -1 on failure + */ static int qemuStorageLimitsRefresh(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, @@ -11550,26 +11575,6 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, char *buf = NULL; ssize_t len; - /* FIXME: For an offline domain, we always want to check current - * on-disk statistics (as users have been known to change offline - * images behind our backs). For a running domain, however, it - * would be nice to avoid opening a file (particularly since - * reading a file while qemu is writing it risks the reader seeing - * bogus data), or even avoid a stat, if the information - * remembered from the previous run is still viable. - * - * 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. - */ if (virStorageSourceIsLocalStorage(src)) { if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY, NULL, NULL)) == -1) -- 2.7.4
Split out the opening of the file and fetch of the stat buffer into a helper qemuDomainStorageOpenStat. This will handle either opening the local or remote storage. Additionally split out the cleanup of that into a separate helper qemuDomainStorageCloseStat which will either close the file or call the virStorageFileDeinit function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 93 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 653fe5d..6a312d2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11537,6 +11537,74 @@ qemuDomainMemoryPeek(virDomainPtr dom, * @cfg: driver configuration data * @vm: domain object * @src: storage source data + * @ret_fd: pointer to return open'd file descriptor + * @ret_sb: pointer to return stat buffer (local or remote) + * + * For local storage, open the file using qemuOpenFile and then use + * fstat() to grab the stat struct data for the caller. + * + * For remote storage, attempt to access the file and grab the stat + * struct data if the remote connection supports it. + * + * Returns 0 on success with @ret_fd and @ret_sb populated, -1 on failure + */ +static int +qemuDomainStorageOpenStat(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src, + int *ret_fd, + struct stat *ret_sb) +{ + if (virStorageSourceIsLocalStorage(src)) { + if ((*ret_fd = qemuOpenFile(driver, vm, src->path, O_RDONLY, + NULL, NULL)) == -1) + return -1; + + if (fstat(*ret_fd, ret_sb) < 0) { + virReportSystemError(errno, _("cannot stat file '%s'"), src->path); + VIR_FORCE_CLOSE(*ret_fd); + return -1; + } + } else { + if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0) + return -1; + + if (virStorageFileStat(src, ret_sb) < 0) { + virStorageFileDeinit(src); + virReportSystemError(errno, _("failed to stat remote file '%s'"), + NULLSTR(src->path)); + return -1; + } + } + + return 0; +} + + +/** + * @src: storage source data + * @fd: file descriptor to close for local + * + * If local, then just close the file descriptor. + * else remote, then tear down the storage driver backend connection. + */ +static void +qemuDomainStorageCloseStat(virStorageSourcePtr src, + int *fd) +{ + if (virStorageSourceIsLocalStorage(src)) + VIR_FORCE_CLOSE(*fd); + else + virStorageFileDeinit(src); +} + + +/** + * @driver: qemu driver data + * @cfg: driver configuration data + * @vm: domain object + * @src: storage source data * * Refresh the capacity and allocation limits of a given storage source. * @@ -11575,35 +11643,19 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, char *buf = NULL; ssize_t len; - if (virStorageSourceIsLocalStorage(src)) { - if ((fd = qemuOpenFile(driver, vm, src->path, O_RDONLY, - NULL, NULL)) == -1) - goto cleanup; - - if (fstat(fd, &sb) < 0) { - virReportSystemError(errno, - _("cannot stat file '%s'"), src->path); - goto cleanup; - } + if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb) < 0) + goto cleanup; + if (virStorageSourceIsLocalStorage(src)) { if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { virReportSystemError(errno, _("cannot read header '%s'"), src->path); goto cleanup; } } else { - if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0) - goto cleanup; - if ((len = virStorageFileReadHeader(src, VIR_STORAGE_MAX_HEADER, &buf)) < 0) goto cleanup; - - if (virStorageFileStat(src, &sb) < 0) { - virReportSystemError(errno, _("failed to stat remote file '%s'"), - NULLSTR(src->path)); - goto cleanup; - } } /* Get info for normal formats */ @@ -11671,8 +11723,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, cleanup: VIR_FREE(buf); virStorageSourceFree(meta); - VIR_FORCE_CLOSE(fd); - virStorageFileDeinit(src); + qemuDomainStorageCloseStat(src, &fd); return ret; } -- 2.7.4
Currently just a shim to call virStorageSourceUpdateBlockPhysicalSize Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6a312d2..a744fda 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11600,6 +11600,14 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src, } +static int +qemuDomainStorageUpdatePhysical(virStorageSourcePtr src, + bool report) +{ + return virStorageSourceUpdateBlockPhysicalSize(src, report); +} + + /** * @driver: qemu driver data * @cfg: driver configuration data @@ -11823,7 +11831,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (entry->physical) { info->physical = entry->physical; } else { - if (virStorageSourceUpdateBlockPhysicalSize(disk->src, true) < 0) + if (qemuDomainStorageUpdatePhysical(disk->src, true) < 0) goto endjob; info->physical = disk->src->physical; @@ -19378,7 +19386,7 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver, QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", entry->physical); } else { - if (virStorageSourceUpdateBlockPhysicalSize(src, false) == 0) { + if (qemuDomainStorageUpdatePhysical(src, false) == 0) { QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", src->physical); } -- 2.7.4
Commit id '8dc27259' introduced virStorageSourceUpdateBlockPhysicalSize in order to retrieve the physical size for a block backed source device since commit id '15fa84ac' changed to use the qemuMonitorGetAllBlockStatsInfo and qemuMonitorBlockStatsUpdateCapacity API's to (essentially) retrieve the "actual-size" from a 'query-block' operation for the source device. However, the code only was made functional for a BLOCK backing type and it neglected to use qemuOpenFile, instead using just open. After the open the block lseek would find the end of the block and set the physical value, close the fd and return. Since the code would return 0 immediately if the source device wasn't a BLOCK backed device, the physical would be displayed incorrectly, such as follows in domblkinfo for a file backed source device: Capacity: 1073741824 Allocation: 0 Physical: 0 This patch will modify the algorithm to get the physical size for other backing types and it will make use of the qemuDomainStorageOpenStat helper in order to open/stat the source file depending on its type. The qemuDomainGetStatsOneBlock will no longer inhibit printing errors, but it will still ignore them leaving the physical value set to 0. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 25 +++++++++++++++---- src/util/virstoragefile.c | 61 ++++++++++++++++++++++++++++------------------- src/util/virstoragefile.h | 6 +++-- 4 files changed, 62 insertions(+), 32 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 43220e0..eaaa088 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2442,7 +2442,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; -virStorageSourceUpdateBlockPhysicalSize; +virStorageSourceUpdatePhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a744fda..6563a1e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11601,10 +11601,23 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src, static int -qemuDomainStorageUpdatePhysical(virStorageSourcePtr src, - bool report) +qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, + virQEMUDriverConfigPtr cfg, + virDomainObjPtr vm, + virStorageSourcePtr src) { - return virStorageSourceUpdateBlockPhysicalSize(src, report); + int ret; + int fd = -1; + struct stat sb; + + if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb) < 0) + return -1; + + ret = virStorageSourceUpdatePhysicalSize(src, fd, &sb); + + qemuDomainStorageCloseStat(src, &fd); + + return ret; } @@ -11831,7 +11844,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (entry->physical) { info->physical = entry->physical; } else { - if (qemuDomainStorageUpdatePhysical(disk->src, true) < 0) + if (qemuDomainStorageUpdatePhysical(driver, cfg, vm, disk->src) < 0) goto endjob; info->physical = disk->src->physical; @@ -19386,9 +19399,11 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver, QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", entry->physical); } else { - if (qemuDomainStorageUpdatePhysical(src, false) == 0) { + if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0) { QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", src->physical); + } else { + virResetLastError(); } } diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1011bd0..809d825 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -24,7 +24,6 @@ #include <config.h> #include "virstoragefile.h" -#include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <stdlib.h> @@ -3175,41 +3174,55 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) /** - * @src: disk source definiton structure - * @report: report libvirt errors if set to true + * @src: disk source definition structure + * @fd: file descriptor + * @sb: stat buffer * - * Updates src->physical for block devices since qemu doesn't report the current - * size correctly for them. Returns 0 on success, -1 on error. + * Updates src->physical depending on the actual type of storage being used. + * + * Returns 0 on success, -1 on error. */ int -virStorageSourceUpdateBlockPhysicalSize(virStorageSourcePtr src, - bool report) +virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, + int fd, + struct stat const *sb) { - int fd = -1; off_t end; - int ret = -1; + virStorageType actual_type = virStorageSourceGetActualType(src); - if (virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_BLOCK) - return 0; + switch (actual_type) { + case VIR_STORAGE_TYPE_FILE: + case VIR_STORAGE_TYPE_NETWORK: + src->physical = sb->st_size; + break; - if ((fd = open(src->path, O_RDONLY)) < 0) { - if (report) - virReportSystemError(errno, _("failed to open block device '%s'"), + case VIR_STORAGE_TYPE_BLOCK: + if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) { + virReportSystemError(errno, _("failed to seek to end of '%s'"), src->path); - return -1; - } + return -1; + } - if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) { - if (report) - virReportSystemError(errno, - _("failed to seek to end of '%s'"), src->path); - } else { src->physical = end; - ret = 0; + break; + + case VIR_STORAGE_TYPE_DIR: + src->physical = 0; + break; + + /* We shouldn't get VOLUME, but the switch requires all cases */ + case VIR_STORAGE_TYPE_VOLUME: + case VIR_STORAGE_TYPE_NONE: + case VIR_STORAGE_TYPE_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot retrieve physical for path '%s' type '%s'"), + NULLSTR(src->path), + virStorageTypeToString(actual_type)); + return -1; + break; } - VIR_FORCE_CLOSE(fd); - return ret; + return 0; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3d09468..e38d01f 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -24,6 +24,8 @@ #ifndef __VIR_STORAGE_FILE_H__ # define __VIR_STORAGE_FILE_H__ +# include <sys/stat.h> + # include "virbitmap.h" # include "virseclabel.h" # include "virstorageencryption.h" @@ -355,8 +357,8 @@ bool virStorageSourceIsEmpty(virStorageSourcePtr src); bool virStorageSourceIsBlockLocal(const virStorageSource *src); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); -int virStorageSourceUpdateBlockPhysicalSize(virStorageSourcePtr src, - bool report); +int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, + int fd, struct stat const *sb); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) -- 2.7.4
Instead of having duplicated code in qemuStorageLimitsRefresh and virStorageBackendUpdateVolTargetInfoFD to fill in the storage backing source or volume allocation, capacity, and physical values - create a common API that will handle the details for both. The common API will fill in "default" capacity values as well - although those more than likely will be overridden by subsequent code. Having just one place to make the determination of what the values should be will make things be more consistent. For the QEMU code - the data filled in will be for inactive domains for the GetBlockInfo and DomainGetStatsOneBlock API's. For the storage backend code - the data will be filled in during the volume updates. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 31 ++------------------- src/storage/storage_backend.c | 33 ++--------------------- src/util/virstoragefile.c | 63 +++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 +++ 5 files changed, 71 insertions(+), 60 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eaaa088..7a78905 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2442,6 +2442,7 @@ virStorageSourceParseRBDColonString; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceUpdateBackingSizes; virStorageSourceUpdatePhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6563a1e..3d20e30 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11657,7 +11657,6 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, { int ret = -1; int fd = -1; - off_t end; virStorageSourcePtr meta = NULL; struct stat sb; int format; @@ -11679,34 +11678,8 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, goto cleanup; } - /* Get info for normal formats */ - if (S_ISREG(sb.st_mode) || fd == -1) { -#ifndef WIN32 - src->allocation = (unsigned long long)sb.st_blocks * - (unsigned long long)DEV_BSIZE; -#else - src->allocation = sb.st_size; -#endif - /* Allocation tracks when the file is sparse, physical is the - * last offset of the file. */ - src->physical = sb.st_size; - } else { - /* NB. Because we configure with AC_SYS_LARGEFILE, off_t - * should be 64 bits on all platforms. For block devices, we - * have to seek (safe even if someone else is writing) to - * determine physical size, and assume that allocation is the - * same as physical (but can refine that assumption later if - * qemu is still running). - */ - end = lseek(fd, 0, SEEK_END); - if (end == (off_t)-1) { - virReportSystemError(errno, - _("failed to seek to end of %s"), src->path); - goto cleanup; - } - src->physical = end; - src->allocation = end; - } + if (virStorageSourceUpdateBackingSizes(src, fd, &sb) < 0) + goto cleanup; /* Raw files: capacity is physical size. For all other files: if * the metadata has a capacity, use that, otherwise fall back to diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index d4334dc..0810ced 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -2004,37 +2004,8 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target, security_context_t filecon = NULL; #endif - if (S_ISREG(sb->st_mode)) { -#ifndef WIN32 - target->allocation = (unsigned long long)sb->st_blocks * - (unsigned long long)DEV_BSIZE; -#else - target->allocation = sb->st_size; -#endif - /* Regular files may be sparse, so logical size (capacity) is not same - * as actual allocation above - */ - target->capacity = sb->st_size; - } else if (S_ISDIR(sb->st_mode)) { - target->allocation = 0; - target->capacity = 0; - } else if (fd >= 0) { - off_t end; - /* XXX this is POSIX compliant, but doesn't work for CHAR files, - * only BLOCK. There is a Linux specific ioctl() for getting - * size of both CHAR / BLOCK devices we should check for in - * configure - */ - end = lseek(fd, 0, SEEK_END); - if (end == (off_t)-1) { - virReportSystemError(errno, - _("cannot seek to end of file '%s'"), - target->path); - return -1; - } - target->allocation = end; - target->capacity = end; - } + if (virStorageSourceUpdateBackingSizes(target, fd, sb) < 0) + return -1; if (!target->perms && VIR_ALLOC(target->perms) < 0) return -1; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 809d825..ab0f9bd 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3226,6 +3226,69 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, } +/** + * @src: disk source definition structure + * @fd: file descriptor + * @sb: stat buffer + * + * Update the capacity, allocation, physical values for the storage @src + * + * Returns 0 on success, -1 on error. + */ +int +virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, + int fd, + struct stat const *sb) +{ + /* Get info for normal formats */ + if (S_ISREG(sb->st_mode) || fd == -1) { +#ifndef WIN32 + src->allocation = (unsigned long long)sb->st_blocks * + (unsigned long long)DEV_BSIZE; +#else + src->allocation = sb->st_size; +#endif + /* Regular files may be sparse, so logical size (capacity) is not same + * as actual allocation above + */ + src->capacity = sb->st_size; + + /* Allocation tracks when the file is sparse, physical is the + * last offset of the file. */ + src->physical = sb->st_size; + } else if (S_ISDIR(sb->st_mode)) { + src->allocation = 0; + src->capacity = 0; + src->physical = 0; + } else if (fd >= 0) { + off_t end; + + /* XXX this is POSIX compliant, but doesn't work for CHAR files, + * only BLOCK. There is a Linux specific ioctl() for getting + * size of both CHAR / BLOCK devices we should check for in + * configure + * + * NB. Because we configure with AC_SYS_LARGEFILE, off_t + * should be 64 bits on all platforms. For block devices, we + * have to seek (safe even if someone else is writing) to + * determine physical size, and assume that allocation is the + * same as physical (but can refine that assumption later if + * qemu is still running). + */ + if ((end = lseek(fd, 0, SEEK_END)) == (off_t)-1) { + virReportSystemError(errno, + _("failed to seek to end of %s"), src->path); + return -1; + } + src->physical = end; + src->allocation = end; + src->capacity = end; + } + + return 0; +} + + static char * virStorageFileCanonicalizeFormatPath(char **components, size_t ncomponents, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e38d01f..b91045b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -359,6 +359,9 @@ void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, int fd, struct stat const *sb); +int virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, + int fd, struct stat const *sb); + virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, bool backingChain) -- 2.7.4
Instead of having duplicated code in qemuStorageLimitsRefresh and virStorageBackendUpdateVolTargetInfo to get capacity specific data about the storage backing source or volume -- create a common API to handle the details for both. As a side effect, virStorageFileProbeFormatFromBuf returns to being a local/static helper to virstoragefile.c For the QEMU code - if the probe is done, then the format is saved so as to avoid future such probes. For the storage backend code, there is no need to deal with the probe since we cannot call the new API if target->format == NONE. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 29 +++------------------- src/storage/storage_backend.c | 8 +----- src/util/virstoragefile.c | 57 ++++++++++++++++++++++++++++++++++++++++++- src/util/virstoragefile.h | 6 ++--- 5 files changed, 65 insertions(+), 37 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7a78905..4b4a197 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2417,7 +2417,6 @@ virStorageFileGetSCSIKey; virStorageFileIsClusterFS; virStorageFileParseChainIndex; virStorageFileProbeFormat; -virStorageFileProbeFormatFromBuf; virStorageFileResize; virStorageIsFile; virStorageNetHostDefClear; @@ -2443,6 +2442,7 @@ virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; virStorageSourceUpdateBackingSizes; +virStorageSourceUpdateCapacity; virStorageSourceUpdatePhysicalSize; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3d20e30..f508872 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11657,9 +11657,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, { int ret = -1; int fd = -1; - virStorageSourcePtr meta = NULL; struct stat sb; - int format; char *buf = NULL; ssize_t len; @@ -11681,27 +11679,8 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, if (virStorageSourceUpdateBackingSizes(src, fd, &sb) < 0) goto cleanup; - /* Raw files: capacity is physical size. For all other files: if - * the metadata has a capacity, use that, otherwise fall back to - * physical size. */ - if (!(format = src->format)) { - if (!cfg->allowDiskFormatProbing) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("no disk format for %s and probing is disabled"), - src->path); - goto cleanup; - } - - if ((format = virStorageFileProbeFormatFromBuf(src->path, - buf, len)) < 0) - goto cleanup; - } - if (format == VIR_STORAGE_FILE_RAW) - src->capacity = src->physical; - else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf, - len, format, NULL))) - src->capacity = meta->capacity ? meta->capacity : src->physical; - else + if (virStorageSourceUpdateCapacity(src, buf, len, + cfg->allowDiskFormatProbing) < 0) goto cleanup; /* If guest is not using raw disk format and is on a host block @@ -11709,14 +11688,14 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, * query the highest allocated extent from QEMU */ if (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_BLOCK && - format != VIR_STORAGE_FILE_RAW && + src->format != VIR_STORAGE_FILE_RAW && S_ISBLK(sb.st_mode)) src->allocation = 0; ret = 0; + cleanup: VIR_FREE(buf); - virStorageSourceFree(meta); qemuDomainStorageCloseStat(src, &fd); return ret; } diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 0810ced..a433190 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1884,7 +1884,6 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, { int ret, fd = -1; struct stat sb; - virStorageSourcePtr meta = NULL; char *buf = NULL; ssize_t len = VIR_STORAGE_MAX_HEADER; @@ -1929,14 +1928,10 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, goto cleanup; } - if (!(meta = virStorageFileGetMetadataFromBuf(target->path, buf, len, target->format, - NULL))) { + if (virStorageSourceUpdateCapacity(target, buf, len, false) < 0) { ret = -1; goto cleanup; } - - if (meta->capacity) - target->capacity = meta->capacity; } if (withBlockVolFormat) { @@ -1946,7 +1941,6 @@ virStorageBackendUpdateVolTargetInfo(virStorageSourcePtr target, } cleanup: - virStorageSourceFree(meta); VIR_FORCE_CLOSE(fd); VIR_FREE(buf); return ret; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ab0f9bd..cf62ca9 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -797,7 +797,7 @@ virStorageIsRelative(const char *backing) } -int +static int virStorageFileProbeFormatFromBuf(const char *path, char *buf, size_t buflen) @@ -3289,6 +3289,61 @@ virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, } +/** + * @src: disk source definition structure + * @buf: buffer to the storage file header + * @len: length of the storage file header + * @probe: allow probe + * + * Update the storage @src capacity. This may involve probing the storage + * @src in order to "see" if we can recognize what exists. + * + * Returns 0 on success, -1 on error. + */ +int +virStorageSourceUpdateCapacity(virStorageSourcePtr src, + char *buf, + ssize_t len, + bool probe) +{ + int ret = -1; + virStorageSourcePtr meta = NULL; + int format = src->format; + + /* Raw files: capacity is physical size. For all other files: if + * the metadata has a capacity, use that, otherwise fall back to + * physical size. */ + if (format == VIR_STORAGE_FILE_NONE) { + if (!probe) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("no disk format for %s and probing is disabled"), + src->path); + goto cleanup; + } + + if ((format = virStorageFileProbeFormatFromBuf(src->path, + buf, len)) < 0) + goto cleanup; + + src->format = format; + } + + if (format == VIR_STORAGE_FILE_RAW) + src->capacity = src->physical; + else if ((meta = virStorageFileGetMetadataFromBuf(src->path, buf, + len, format, NULL))) + src->capacity = meta->capacity ? meta->capacity : src->physical; + else + goto cleanup; + + ret = 0; + + cleanup: + virStorageSourceFree(meta); + return ret; +} + + static char * virStorageFileCanonicalizeFormatPath(char **components, size_t ncomponents, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b91045b..6d1aac7 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -284,9 +284,6 @@ struct _virStorageSource { # endif int virStorageFileProbeFormat(const char *path, uid_t uid, gid_t gid); -int virStorageFileProbeFormatFromBuf(const char *path, - char *buf, - size_t buflen); int virStorageFileGetMetadataInternal(virStorageSourcePtr meta, char *buf, @@ -361,6 +358,9 @@ int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, int fd, struct stat const *sb); int virStorageSourceUpdateBackingSizes(virStorageSourcePtr src, int fd, struct stat const *sb); +int virStorageSourceUpdateCapacity(virStorageSourcePtr src, + char *buf, ssize_t len, + bool probe); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, -- 2.7.4
The libvirt-domain.h documentation indicates that for a qcow2 file in a filesystem being used for a backing store should report the disk space occupied by a file; however, commit id '15fa84ac' altered the code to trust that the wr_highest_offset should be used whenever wr_highest_offset_valid was set. As it turns out this will lead to indeterminite results. For an active domain when qemu hasn't yet had the need to find the wr_highest_offset value, qemu will report 0 even though qemu-img will report the proper disk size. This causes reporting of the following XML: <disk type='file' device='disk'> <driver name='qemu' type='qcow2'/> <source file='/path/to/test-1g.qcow2'/> to be as follows: Capacity: 1073741824 Allocation: 0 Physical: 1074139136 with qemu-img indicating: image: /path/to/test-1g.qcow2 file format: qcow2 virtual size: 1.0G (1073741824 bytes) disk size: 1.0G Once the backing source file is opened on the guest, then wr_highest_offset is updated, but only to the high water mark and not the size of the file. This patch will adjust the logic to check for the file backed qcow2 image and enforce setting the allocation to the returned 'physical' value, which is the 'actual-size' value from a 'query-block' operation. NB: The other consumer of the wr_highest_offset output (GetAllDomainStats) has a contract that indicates 'allocation' is the offset of the highest written sector, so it doesn't need adjustment. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f508872..61171cb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11790,7 +11790,11 @@ qemuDomainGetBlockInfo(virDomainPtr dom, info->allocation = entry->physical; } else { - info->allocation = entry->wr_highest_offset; + if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_FILE && + disk->src->format == VIR_STORAGE_FILE_QCOW2) + info->allocation = entry->physical; + else + info->allocation = entry->wr_highest_offset; } if (entry->physical) { -- 2.7.4
On 02.12.2016 01:39, John Ferlan wrote:
A bit of a mix between fixing some bugs and merging some code between the block info/stats fetching of backing store data and storage backend data for volume allocation, capacity, and physical sizes.
Patch 1 just adjusts some comments
Patches 2-3 are code motion
Patch 4 alters how domain/qemu will open the backing store from an open to a qemuFileOpen and it allows for fetching of more physical sizes (the code only gets the block backing stores now)
Patch 5 shares the setting of the various size values between storage driver and domain/qemu
Patch 6 shares the recalculation of the capacity value (it may not be necessary for patch 5, but I left it there since there's multiple other paths to the VolTargetInfoFD API).
Patch 7 fixes an issue with displaying the allocation value of a qcow2 backed file for the GetBlockInfo API (the results will match volume code now).
John Ferlan (7): qemu: Clean up description for qemuStorageLimitsRefresh qemu: Add helpers to handle stat data for qemuStorageLimitsRefresh qemu: Introduce helper qemuDomainStorageUpdatePhysical util: Introduce virStorageSourceUpdatePhysicalSize util: Introduce virStorageSourceUpdateBackingSizes util: Introduce virStorageSourceUpdateCapacity qemu: Fix GetBlockInfo setting allocation from wr_highest_offset
src/libvirt_private.syms | 5 +- src/qemu/qemu_driver.c | 237 ++++++++++++++++++++++++------------------ src/storage/storage_backend.c | 41 +------- src/util/virstoragefile.c | 173 ++++++++++++++++++++++++++---- src/util/virstoragefile.h | 15 ++- 5 files changed, 304 insertions(+), 167 deletions(-)
ACK but I suggest postponing the push after the release. Michal
participants (2)
-
John Ferlan -
Michal Privoznik