[libvirt] [PATCH 0/6] qemu: Allow suppressing some errors in qemu bulk stats code
There is an bugreport [1] that some Openstack deployments get periodic errors in the log. This is most probably from the bulk stats code which in some cases calls into the storage backends to update some aspects of storage volumes. Since none of the errors are fatal add code to allow skipping them. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1724808 Peter Krempa (6): util: Export virStorageFileSupportsBackingChainTraversal qemu: Allow skipping some errors in qemuDomainStorageOpenStat util: storagefile: Don't report errors from virStorageSourceUpdatePhysicalSize qemu: driver: Improve error suppression in qemuDomainStorageUpdatePhysical qemu: Allow suppressing errors from qemuStorageLimitsRefresh qemu: Don't report some ignored errors in qemuDomainGetStatsOneBlockFallback src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 60 ++++++++++++++++++++++++++++----------- src/util/virstoragefile.c | 16 +++-------- src/util/virstoragefile.h | 1 + 4 files changed, 50 insertions(+), 28 deletions(-) -- 2.21.0
The function will be reused in the qemu snapshot code. The argument is turned into const similarly to the other virStorageFileSupports* functions. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 4 ++-- src/util/virstoragefile.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 83f40cefe6..9db4ac7933 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2994,6 +2994,7 @@ virStorageFileReportBrokenChain; virStorageFileResize; virStorageFileStat; virStorageFileSupportsAccess; +virStorageFileSupportsBackingChainTraversal; virStorageFileSupportsCreate; virStorageFileSupportsSecurityDriver; virStorageFileUnlink; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ba56f452e9..4011187a7e 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -4448,8 +4448,8 @@ virStorageFileGetBackendForSupportCheck(const virStorageSource *src, } -static int -virStorageFileSupportsBackingChainTraversal(virStorageSourcePtr src) +int +virStorageFileSupportsBackingChainTraversal(const virStorageSource *src) { virStorageFileBackendPtr backend; int rv; diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 2882bacf3e..b65cd4c382 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -533,6 +533,7 @@ int virStorageFileChown(const virStorageSource *src, uid_t uid, gid_t gid); int virStorageFileSupportsSecurityDriver(const virStorageSource *src); int virStorageFileSupportsAccess(const virStorageSource *src); int virStorageFileSupportsCreate(const virStorageSource *src); +int virStorageFileSupportsBackingChainTraversal(const virStorageSource *src); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, -- 2.21.0
On Wed, Aug 14, 2019 at 06:59:16PM +0200, Peter Krempa wrote:
The function will be reused in the qemu snapshot code. The argument is turned into const similarly to the other virStorageFileSupports* functions.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 4 ++-- src/util/virstoragefile.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Some callers of this function actually don't care about errors and reset it. The message is still logged which might irritate users in this case. Add a boolean flag which will do few checks whether it actually makes sense to even try opening the storage file. For local files we check whether it exists and for remote files we at first see whether we even have a storage driver backend for it in the first place before trying to open it. Other problems will still report errors but these are the most common scenarios which can happen here. This patch changes the return value of the function so that the caller is able to differentiate the possibilities. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 48c7b5628b..cc296c1fe3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12247,6 +12247,7 @@ qemuDomainMemoryPeek(virDomainPtr dom, * @src: storage source data * @ret_fd: pointer to return open'd file descriptor * @ret_sb: pointer to return stat buffer (local or remote) + * @skipInaccessible: Don't report error if files are not accessible * * For local storage, open the file using qemuOpenFile and then use * fstat() to grab the stat struct data for the caller. @@ -12254,7 +12255,9 @@ qemuDomainMemoryPeek(virDomainPtr dom, * 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 + * Returns 1 if @src was successfully opened (@ret_fd and @ret_sb is populated), + * 0 if @src can't be opened and @skipInaccessible is true (no errors are + * reported) or -1 otherwise (errors are reported). */ static int qemuDomainStorageOpenStat(virQEMUDriverPtr driver, @@ -12262,9 +12265,13 @@ qemuDomainStorageOpenStat(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src, int *ret_fd, - struct stat *ret_sb) + struct stat *ret_sb, + bool skipInaccessible) { if (virStorageSourceIsLocalStorage(src)) { + if (skipInaccessible && !virFileExists(src->path)) + return 0; + if ((*ret_fd = qemuOpenFile(driver, vm, src->path, O_RDONLY, NULL)) < 0) return -1; @@ -12275,6 +12282,9 @@ qemuDomainStorageOpenStat(virQEMUDriverPtr driver, return -1; } } else { + if (skipInaccessible && virStorageFileSupportsBackingChainTraversal(src) <= 0) + return 0; + if (virStorageFileInitAs(src, cfg->user, cfg->group) < 0) return -1; @@ -12286,7 +12296,7 @@ qemuDomainStorageOpenStat(virQEMUDriverPtr driver, } } - return 0; + return 1; } @@ -12321,7 +12331,7 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(src)) return 0; - if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb) < 0) + if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, false) < 0) return -1; ret = virStorageSourceUpdatePhysicalSize(src, fd, &sb); @@ -12372,7 +12382,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, char *buf = NULL; ssize_t len; - if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb) < 0) + if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, false) < 0) goto cleanup; if (virStorageSourceIsLocalStorage(src)) { -- 2.21.0
On Wed, Aug 14, 2019 at 06:59:17PM +0200, Peter Krempa wrote:
Some callers of this function actually don't care about errors and reset it. The message is still logged which might irritate users in this case.
Add a boolean flag which will do few checks whether it actually makes sense to even try opening the storage file. For local files we check whether it exists and for remote files we at first see whether we even have a storage driver backend for it in the first place before trying to open it.
Other problems will still report errors but these are the most common scenarios which can happen here.
This patch changes the return value of the function so that the caller is able to differentiate the possibilities.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
virStorageSourceUpdatePhysicalSize is called only from qemuDomainStorageUpdatePhysical and all callers of it reset the libvirt error if -1 is returned. Don't bother setting the error in the first place. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 4011187a7e..21bd1f00c9 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -3746,7 +3746,7 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) * 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. + * Returns 0 on success, -1 on error. No libvirt errors are reported. */ int virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, @@ -3763,11 +3763,8 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, break; 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); + if ((end = lseek(fd, 0, SEEK_END)) == (off_t) -1) return -1; - } src->physical = end; break; @@ -3780,12 +3777,7 @@ virStorageSourceUpdatePhysicalSize(virStorageSourcePtr src, 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; } return 0; -- 2.21.0
On Wed, Aug 14, 2019 at 06:59:18PM +0200, Peter Krempa wrote:
virStorageSourceUpdatePhysicalSize is called only from qemuDomainStorageUpdatePhysical and all callers of it reset the libvirt error if -1 is returned.
Don't bother setting the error in the first place.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virstoragefile.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
None of the callers of qemuDomainStorageUpdatePhysical care about errors. Use the new flag for qemuDomainStorageOpenStat which suppresses some errors and move the reset of the rest of the uncommon errors into this function. Document what is happening in a comment for the function. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cc296c1fe3..2dffef9642 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12318,6 +12318,19 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src, } +/** + * qemuDomainStorageUpdatePhysical: + * @driver: qemu driver + * @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)). + */ static int qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, @@ -12331,8 +12344,11 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(src)) return 0; - if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, false) < 0) + if ((ret = qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, true)) <= 0) { + if (ret < 0) + virResetLastError(); return -1; + } ret = virStorageSourceUpdatePhysicalSize(src, fd, &sb); @@ -12504,7 +12520,6 @@ qemuDomainGetBlockInfo(virDomainPtr dom, if (qemuDomainStorageUpdatePhysical(driver, cfg, vm, disk->src) == 0) { info->physical = disk->src->physical; } else { - virResetLastError(); info->physical = entry->physical; } } else { @@ -21376,12 +21391,9 @@ qemuDomainGetStatsOneBlock(virQEMUDriverPtr driver, QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", entry->physical); } else { - if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0) { + if (qemuDomainStorageUpdatePhysical(driver, cfg, dom, src) == 0) QEMU_ADD_BLOCK_PARAM_ULL(record, maxparams, block_idx, "physical", src->physical); - } else { - virResetLastError(); - } } ret = 0; -- 2.21.0
On Wed, Aug 14, 2019 at 06:59:19PM +0200, Peter Krempa wrote:
None of the callers of qemuDomainStorageUpdatePhysical care about errors.
Use the new flag for qemuDomainStorageOpenStat which suppresses some errors and move the reset of the rest of the uncommon errors into this function. Document what is happening in a comment for the function.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cc296c1fe3..2dffef9642 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12318,6 +12318,19 @@ qemuDomainStorageCloseStat(virStorageSourcePtr src, }
+/** + * qemuDomainStorageUpdatePhysical: + * @driver: qemu driver + * @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)).
By uncommon you mean missing network storage? :)
+ */ static int qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, @@ -12331,8 +12344,11 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(src)) return 0;
- if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, false) < 0) + if ((ret = qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, true)) <= 0) { + if (ret < 0) + virResetLastError();
Still ugly to use virResetLastError, but at least it's an improvement. Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
qemuStorageLimitsRefresh uses qemuDomainStorageOpenStat internally and there are callers which don't care about the error. Propagate the skipInaccessible flag so that we can log less errors. Callers currently don't care about the return value change. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2dffef9642..243a8ac4cf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12363,6 +12363,7 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, * @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. * @@ -12384,22 +12385,27 @@ qemuDomainStorageUpdatePhysical(virQEMUDriverPtr driver, * 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 + * 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(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, - virStorageSourcePtr src) + virStorageSourcePtr src, + bool skipInaccessible) { + int rc; int ret = -1; int fd = -1; struct stat sb; char *buf = NULL; ssize_t len; - if (qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, false) < 0) - goto cleanup; + if ((rc = qemuDomainStorageOpenStat(driver, cfg, vm, src, &fd, &sb, + skipInaccessible)) <= 0) + return rc; if (virStorageSourceIsLocalStorage(src)) { if ((len = virFileReadHeaderFD(fd, VIR_STORAGE_MAX_HEADER, &buf)) < 0) { @@ -12427,7 +12433,7 @@ qemuStorageLimitsRefresh(virQEMUDriverPtr driver, S_ISBLK(sb.st_mode)) src->allocation = 0; - ret = 0; + ret = 1; cleanup: VIR_FREE(buf); @@ -12477,7 +12483,7 @@ qemuDomainGetBlockInfo(virDomainPtr dom, /* for inactive domains we have to peek into the files */ if (!virDomainObjIsActive(vm)) { - if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src)) < 0) + if ((qemuStorageLimitsRefresh(driver, cfg, vm, disk->src, false)) < 0) goto endjob; info->capacity = disk->src->capacity; @@ -21295,7 +21301,7 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(src)) return 0; - if (qemuStorageLimitsRefresh(driver, cfg, dom, src) < 0) { + if (qemuStorageLimitsRefresh(driver, cfg, dom, src, false) < 0) { virResetLastError(); return 0; } -- 2.21.0
On Wed, Aug 14, 2019 at 06:59:20PM +0200, Peter Krempa wrote:
qemuStorageLimitsRefresh uses qemuDomainStorageOpenStat internally and there are callers which don't care about the error. Propagate the skipInaccessible flag so that we can log less errors.
Callers currently don't care about the return value change.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
The function ignores all errors from qemuStorageLimitsRefresh by calling virResetLastError. This still logs them. Since qemuStorageLimitsRefresh allows suppressing some, do so. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 243a8ac4cf..f44d134b92 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -21301,7 +21301,7 @@ qemuDomainGetStatsOneBlockFallback(virQEMUDriverPtr driver, if (virStorageSourceIsEmpty(src)) return 0; - if (qemuStorageLimitsRefresh(driver, cfg, dom, src, false) < 0) { + if (qemuStorageLimitsRefresh(driver, cfg, dom, src, true) <= 0) { virResetLastError(); return 0; } -- 2.21.0
On Wed, Aug 14, 2019 at 06:59:21PM +0200, Peter Krempa wrote:
The function ignores all errors from qemuStorageLimitsRefresh by calling virResetLastError. This still logs them. Since qemuStorageLimitsRefresh allows suppressing some, do so.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Peter Krempa