virFileDirectFd was used for accessing files opened with O_DIRECT using libvirt_iohelper. We will want to use the helper for accessing files regardless on O_DIRECT and thus virFileDirectFd was generalized and renamed to virFileWrapperFd. --- src/qemu/qemu_driver.c | 45 +++++++++++-------- src/util/virfile.c | 118 +++++++++++++++++++++++++++++------------------- src/util/virfile.h | 21 ++++++--- 3 files changed, 112 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f940e8..99da3f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2521,7 +2521,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, unsigned long long pad; int fd = -1; int directFlag = 0; - virFileDirectFdPtr directFd = NULL; + virFileWrapperFdPtr wrapperFd = NULL; bool bypass_cache = flags & VIR_DOMAIN_SAVE_BYPASS_CACHE; if (qemuProcessAutoDestroyActive(driver, vm)) { @@ -2625,7 +2625,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, &needUnlink, &bypassSecurityDriver); if (fd < 0) goto endjob; - if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) + if (bypass_cache && + !(wrapperFd = virFileWrapperFdNew(&fd, path, + VIR_FILE_WRAPPER_BYPASS_CACHE))) goto endjob; /* Write header to file, followed by XML */ @@ -2644,14 +2646,14 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, /* Touch up file header to mark image complete. */ if (bypass_cache) { /* Reopen the file to touch up the header, since we aren't set - * up to seek backwards on directFd. The reopened fd will + * up to seek backwards on wrapperFd. The reopened fd will * trigger a single page of file system cache pollution, but * that's acceptable. */ if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("unable to close %s"), path); goto endjob; } - if (virFileDirectFdClose(directFd) < 0) + if (virFileWrapperFdClose(wrapperFd) < 0) goto endjob; fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL); if (fd < 0) @@ -2703,7 +2705,7 @@ endjob: cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdFree(directFd); + virFileWrapperFdFree(wrapperFd); VIR_FREE(xml); if (ret != 0 && needUnlink) unlink(path); @@ -2939,7 +2941,7 @@ doCoreDump(struct qemud_driver *driver, { int fd = -1; int ret = -1; - virFileDirectFdPtr directFd = NULL; + virFileWrapperFdPtr wrapperFd = NULL; int directFlag = 0; /* Create an empty file with appropriate ownership. */ @@ -2959,7 +2961,9 @@ doCoreDump(struct qemud_driver *driver, NULL, NULL)) < 0) goto cleanup; - if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) + if (bypass_cache && + !(wrapperFd = virFileWrapperFdNew(&fd, path, + VIR_FILE_WRAPPER_BYPASS_CACHE))) goto cleanup; if (qemuMigrationToFile(driver, vm, fd, 0, path, @@ -2973,14 +2977,14 @@ doCoreDump(struct qemud_driver *driver, path); goto cleanup; } - if (virFileDirectFdClose(directFd) < 0) + if (virFileWrapperFdClose(wrapperFd) < 0) goto cleanup; ret = 0; cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdFree(directFd); + virFileWrapperFdFree(wrapperFd); if (ret != 0) unlink(path); return ret; @@ -3926,7 +3930,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, - bool bypass_cache, virFileDirectFdPtr *directFd, + bool bypass_cache, + virFileWrapperFdPtr *wrapperFd, const char *xmlin, int state, bool edit, bool unlink_corrupt) { @@ -3948,7 +3953,9 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, if ((fd = qemuOpenFile(driver, path, oflags, NULL, NULL)) < 0) goto error; - if (bypass_cache && (*directFd = virFileDirectFdNew(&fd, path)) == NULL) + if (bypass_cache && + !(*wrapperFd = virFileWrapperFdNew(&fd, path, + VIR_FILE_WRAPPER_BYPASS_CACHE))) goto error; if (saferead(fd, &header, sizeof(header)) != sizeof(header)) { @@ -4187,7 +4194,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, int fd = -1; int ret = -1; struct qemud_save_header header; - virFileDirectFdPtr directFd = NULL; + virFileWrapperFdPtr wrapperFd = NULL; int state = -1; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -4203,7 +4210,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd, dxml, state, false, false); + &wrapperFd, dxml, state, false, false); if (fd < 0) goto cleanup; @@ -4223,7 +4230,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path, false); - if (virFileDirectFdClose(directFd) < 0) + if (virFileWrapperFdClose(wrapperFd) < 0) VIR_WARN("Failed to close %s", path); if (qemuDomainObjEndJob(driver, vm) == 0) @@ -4236,7 +4243,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); - virFileDirectFdFree(directFd); + virFileWrapperFdFree(wrapperFd); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -4364,10 +4371,10 @@ qemuDomainObjRestore(virConnectPtr conn, int fd = -1; int ret = -1; struct qemud_save_header header; - virFileDirectFdPtr directFd = NULL; + virFileWrapperFdPtr wrapperFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL, -1, false, + bypass_cache, &wrapperFd, NULL, -1, false, true); if (fd < 0) { if (fd == -3) @@ -4394,13 +4401,13 @@ qemuDomainObjRestore(virConnectPtr conn, ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path, start_paused); - if (virFileDirectFdClose(directFd) < 0) + if (virFileWrapperFdClose(wrapperFd) < 0) VIR_WARN("Failed to close %s", path); cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); - virFileDirectFdFree(directFd); + virFileWrapperFdFree(wrapperFd); return ret; } diff --git a/src/util/virfile.c b/src/util/virfile.c index e6b469c..66160dc 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -94,12 +94,6 @@ FILE *virFileFdopen(int *fdptr, const char *mode) } -/* Opaque type for managing a wrapper around an O_DIRECT fd. For now, - * read-write is not supported, just a single direction. */ -struct _virFileDirectFd { - virCommandPtr cmd; /* Child iohelper process to do the I/O. */ -}; - /** * virFileDirectFdFlag: * @@ -116,36 +110,62 @@ virFileDirectFdFlag(void) return O_DIRECT ? O_DIRECT : -1; } +/* Opaque type for managing a wrapper around a fd. For now, + * read-write is not supported, just a single direction. */ +struct _virFileWrapperFd { + virCommandPtr cmd; /* Child iohelper process to do the I/O. */ +}; + +#ifndef WIN32 /** - * virFileDirectFdNew: + * virFileWrapperFdNew: * @fd: pointer to fd to wrap * @name: name of fd, for diagnostics + * @flags: bitwise-OR of virFileWrapperFdFlags + * + * Update @fd so that it meets parameters requested by @flags. + * + * If VIR_FILE_WRAPPER_BYPASS_CACHE bit is set in @flags, @fd will be updated + * in a way that all I/O to that file will bypass the system cache. The + * original fd must have been created with virFileDirectFdFlag() among the + * flags to open(). * - * Update *FD (created with virFileDirectFdFlag() among the flags to - * open()) to ensure that all I/O to that file will bypass the system - * cache. This must be called after open() and optional fchown() or - * fchmod(), but before any seek or I/O, and only on seekable fd. The - * file must be O_RDONLY (to read the entire existing file) or - * O_WRONLY (to write to an empty file). In some cases, *FD is - * changed to a non-seekable pipe; in this case, the caller must not - * do anything further with the original fd. + * If VIR_FILE_WRAPPER_NON_BLOCKING bit is set in @flags, @fd will be updated + * to ensure it properly supports non-blocking I/O, i.e., it will report + * EAGAIN. + * + * This must be called after open() and optional fchown() or fchmod(), but + * before any seek or I/O, and only on seekable fd. The file must be O_RDONLY + * (to read the entire existing file) or O_WRONLY (to write to an empty file). + * In some cases, @fd is changed to a non-seekable pipe; in this case, the + * caller must not do anything further with the original fd. * * On success, the new wrapper object is returned, which must be later - * freed with virFileDirectFdFree(). On failure, *FD is unchanged, an + * freed with virFileWrapperFdFree(). On failure, @fd is unchanged, an * error message is output, and NULL is returned. */ -virFileDirectFdPtr -virFileDirectFdNew(int *fd, const char *name) +virFileWrapperFdPtr +virFileWrapperFdNew(int *fd, const char *name, unsigned int flags) { - virFileDirectFdPtr ret = NULL; + virFileWrapperFdPtr ret = NULL; bool output = false; int pipefd[2] = { -1, -1 }; int mode = -1; - /* XXX support posix_fadvise rather than spawning a child, if the - * kernel support for that is decent enough. */ + if (!flags) { + virFileError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use with no flags")); + return NULL; + } + + /* XXX support posix_fadvise rather than O_DIRECT, if the kernel support + * for that is decent enough. In that case, we will also need to + * explicitly support VIR_FILE_WRAPPER_NON_BLOCKING since + * VIR_FILE_WRAPPER_BYPASS_CACHE alone will no longer require spawning + * iohelper. + */ - if (!O_DIRECT) { + if ((flags & VIR_FILE_WRAPPER_BYPASS_CACHE) && !O_DIRECT) { virFileError(VIR_ERR_INTERNAL_ERROR, "%s", _("O_DIRECT unsupported on this platform")); return NULL; @@ -156,12 +176,7 @@ virFileDirectFdNew(int *fd, const char *name) return NULL; } -#ifdef F_GETFL - /* Mingw lacks F_GETFL, but it also lacks O_DIRECT so didn't get - * here in the first place. All other platforms reach this - * line. */ mode = fcntl(*fd, F_GETFL); -#endif if (mode < 0) { virFileError(VIR_ERR_INTERNAL_ERROR, _("invalid fd %d for %s"), @@ -208,48 +223,59 @@ virFileDirectFdNew(int *fd, const char *name) error: VIR_FORCE_CLOSE(pipefd[0]); VIR_FORCE_CLOSE(pipefd[1]); - virFileDirectFdFree(ret); + virFileWrapperFdFree(ret); return NULL; } +#else +virFileWrapperFdPtr +virFileWrapperFdNew(int *fd ATTRIBUTE_UNUSED, + const char *name ATTRIBUTE_UNUSED, + unsigned int fdflags ATTRIBUTE_UNUSED) +{ + virFileError(VIR_ERR_INTERNAL_ERROR, "%s", + _("virFileWrapperFd unsupported on this platform")); + return NULL; +} +#endif /** - * virFileDirectFdClose: - * @dfd: direct fd wrapper, or NULL + * virFileWrapperFdClose: + * @wfd: fd wrapper, or NULL * - * If DFD is valid, then ensure that I/O has completed, which may + * If @wfd is valid, then ensure that I/O has completed, which may * include reaping a child process. Return 0 if all data for the * wrapped fd is complete, or -1 on failure with an error emitted. - * This function intentionally returns 0 when DFD is NULL, so that - * callers can conditionally create a virFileDirectFd wrapper but + * This function intentionally returns 0 when @wfd is NULL, so that + * callers can conditionally create a virFileWrapperFd wrapper but * unconditionally call the cleanup code. To avoid deadlock, only - * call this after closing the fd resulting from virFileDirectFdNew(). + * call this after closing the fd resulting from virFileWrapperFdNew(). */ int -virFileDirectFdClose(virFileDirectFdPtr dfd) +virFileWrapperFdClose(virFileWrapperFdPtr wfd) { - if (!dfd) + if (!wfd) return 0; - return virCommandWait(dfd->cmd, NULL); + return virCommandWait(wfd->cmd, NULL); } /** - * virFileDirectFdFree: - * @dfd: direct fd wrapper, or NULL + * virFileWrapperFdFree: + * @wfd: fd wrapper, or NULL * - * Free all remaining resources associated with DFD. If - * virFileDirectFdClose() was not previously called, then this may + * Free all remaining resources associated with @wfd. If + * virFileWrapperFdClose() was not previously called, then this may * discard some previous I/O. To avoid deadlock, only call this after - * closing the fd resulting from virFileDirectFdNew(). + * closing the fd resulting from virFileWrapperFdNew(). */ void -virFileDirectFdFree(virFileDirectFdPtr dfd) +virFileWrapperFdFree(virFileWrapperFdPtr wfd) { - if (!dfd) + if (!wfd) return; - virCommandFree(dfd->cmd); - VIR_FREE(dfd); + virCommandFree(wfd->cmd); + VIR_FREE(wfd); } diff --git a/src/util/virfile.h b/src/util/virfile.h index 7be15b5..ec1e90b 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -50,20 +50,27 @@ FILE *virFileFdopen(int *fdptr, const char *mode) ATTRIBUTE_RETURN_CHECK; # define VIR_FORCE_CLOSE(FD) ignore_value(virFileClose(&(FD), true)) # define VIR_FORCE_FCLOSE(FILE) ignore_value(virFileFclose(&(FILE), true)) -/* Opaque type for managing a wrapper around an O_DIRECT fd. */ -struct _virFileDirectFd; +/* Opaque type for managing a wrapper around a fd. */ +struct _virFileWrapperFd; -typedef struct _virFileDirectFd virFileDirectFd; -typedef virFileDirectFd *virFileDirectFdPtr; +typedef struct _virFileWrapperFd virFileWrapperFd; +typedef virFileWrapperFd *virFileWrapperFdPtr; int virFileDirectFdFlag(void); -virFileDirectFdPtr virFileDirectFdNew(int *fd, const char *name) +enum { + VIR_FILE_WRAPPER_BYPASS_CACHE = (1 << 0), + VIR_FILE_WRAPPER_NON_BLOCKING = (1 << 1), +} virFileWrapperFdFlags; + +virFileWrapperFdPtr virFileWrapperFdNew(int *fd, + const char *name, + unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -int virFileDirectFdClose(virFileDirectFdPtr dfd); +int virFileWrapperFdClose(virFileWrapperFdPtr dfd); -void virFileDirectFdFree(virFileDirectFdPtr dfd); +void virFileWrapperFdFree(virFileWrapperFdPtr dfd); int virFileLock(int fd, bool shared, off_t start, off_t len); int virFileUnlock(int fd, off_t start, off_t len); -- 1.7.8.4

This is probably not strictly needed as save operation is not live but we may have other reasons to avoid blocking qemu's main loop. --- src/qemu/qemu_driver.c | 44 +++++++++++++++++++------------------------- 1 files changed, 19 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 52350f2..0cac6a5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2522,7 +2522,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, int fd = -1; int directFlag = 0; virFileWrapperFdPtr wrapperFd = NULL; - bool bypass_cache = flags & VIR_DOMAIN_SAVE_BYPASS_CACHE; + unsigned int wrapperFlags = VIR_FILE_WRAPPER_NON_BLOCKING; if (qemuProcessAutoDestroyActive(driver, vm)) { qemuReportError(VIR_ERR_OPERATION_INVALID, @@ -2613,7 +2613,8 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, header.xml_len = len; /* Obtain the file handle. */ - if (bypass_cache) { + if ((flags & VIR_DOMAIN_SAVE_BYPASS_CACHE)) { + wrapperFlags |= VIR_FILE_WRAPPER_BYPASS_CACHE; directFlag = virFileDirectFdFlag(); if (directFlag < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", @@ -2625,9 +2626,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, &needUnlink, &bypassSecurityDriver); if (fd < 0) goto endjob; - if (bypass_cache && - !(wrapperFd = virFileWrapperFdNew(&fd, path, - VIR_FILE_WRAPPER_BYPASS_CACHE))) + if (!(wrapperFd = virFileWrapperFdNew(&fd, path, wrapperFlags))) goto endjob; /* Write header to file, followed by XML */ @@ -2643,27 +2642,22 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, QEMU_ASYNC_JOB_SAVE) < 0) goto endjob; - /* Touch up file header to mark image complete. */ - if (bypass_cache) { - /* Reopen the file to touch up the header, since we aren't set - * up to seek backwards on wrapperFd. The reopened fd will - * trigger a single page of file system cache pollution, but - * that's acceptable. */ - if (VIR_CLOSE(fd) < 0) { - virReportSystemError(errno, _("unable to close %s"), path); - goto endjob; - } - if (virFileWrapperFdClose(wrapperFd) < 0) - goto endjob; - fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL); - if (fd < 0) - goto endjob; - } else { - if (lseek(fd, 0, SEEK_SET) != 0) { - virReportSystemError(errno, _("unable to seek %s"), path); - goto endjob; - } + /* Touch up file header to mark image complete. */ + + /* Reopen the file to touch up the header, since we aren't set + * up to seek backwards on wrapperFd. The reopened fd will + * trigger a single page of file system cache pollution, but + * that's acceptable. */ + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("unable to close %s"), path); + goto endjob; } + if (virFileWrapperFdClose(wrapperFd) < 0) + goto endjob; + fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL); + if (fd < 0) + goto endjob; + memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic)); if (safewrite(fd, &header, sizeof(header)) != sizeof(header)) { virReportSystemError(errno, _("unable to write %s"), path); -- 1.7.8.4