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 | 42 +++++++++++--------- src/util/virfile.c | 98 ++++++++++++++++++++++++++--------------------- src/util/virfile.h | 18 +++++---- 3 files changed, 87 insertions(+), 71 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3f940e8..01dbcd7 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,8 @@ 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, true, path)) == NULL) goto endjob; /* Write header to file, followed by XML */ @@ -2644,14 +2645,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 +2704,7 @@ endjob: cleanup: VIR_FORCE_CLOSE(fd); - virFileDirectFdFree(directFd); + virFileWrapperFdFree(wrapperFd); VIR_FREE(xml); if (ret != 0 && needUnlink) unlink(path); @@ -2939,7 +2940,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 +2960,8 @@ doCoreDump(struct qemud_driver *driver, NULL, NULL)) < 0) goto cleanup; - if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL) + if (bypass_cache && + (wrapperFd = virFileWrapperFdNew(&fd, true, path)) == NULL) goto cleanup; if (qemuMigrationToFile(driver, vm, fd, 0, path, @@ -2973,14 +2975,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 +3928,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 +3951,8 @@ 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, true, path)) == NULL) goto error; if (saferead(fd, &header, sizeof(header)) != sizeof(header)) { @@ -4187,7 +4191,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 +4207,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 +4227,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 +4240,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); - virFileDirectFdFree(directFd); + virFileWrapperFdFree(wrapperFd); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -4364,10 +4368,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 +4398,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..218feab 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,46 @@ 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 + * @bypassCache: bypass system cache * @name: name of fd, for diagnostics * - * 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 + * Update @fd (possibly created with virFileDirectFdFlag() among the flags + * to open()) to ensure it properly supports non-blocking I/O, i.e., it will + * report EAGAIN. Iff @direct is true (in which case fd must have been + * created with virFileDirectFdFlag()), it will also be ensured 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. * * 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, bool bypassCache, const char *name) { - 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 + /* XXX support posix_fadvise rather than O_DIRECT, if the * kernel support for that is decent enough. */ - if (!O_DIRECT) { + if (bypassCache && !O_DIRECT) { virFileError(VIR_ERR_INTERNAL_ERROR, "%s", _("O_DIRECT unsupported on this platform")); return NULL; @@ -156,12 +160,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 +207,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, + bool bypassCache ATTRIBUTE_UNUSED, + const char *name 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..a412f25 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -50,20 +50,22 @@ 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) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +virFileWrapperFdPtr virFileWrapperFdNew(int *fd, + bool bypassCache, + const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) 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

On Mon, Feb 06, 2012 at 15:42:43 -0700, Eric Blake wrote: OK, good point. However, I added a single int flags instead of two bool parameters since I consider combining named bits more readable than several true/false arguments for which you always need to go to the header to get the meaning. There's no need to pass a proper non-blocking fd to qemu here since save is not live... While here core dump can be live so we need non-blocking. And this place was the only place I touched in 2/2 so I'm not quite sure why you say I missed it... Jirka

On 02/06/2012 07:51 AM, Jiri Denemark wrote: What you have is right (and amazingly short for the bug fix at hand!), but you missed the core dump fd creation path, which needs the same treatment. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 02/07/2012 05:09 AM, Jiri Denemark wrote: Ah, then I misread the @@ hunk lines. I knew you had only touched one of two places that created an fd, but then spoke about the wrong hunk of the two as being the one that you missed. My apologies for compounding my confusion onto the list. There are two situations where we save to file - one during core dump (which, as you point out, can be live where the guest continues to run after the dump, and therefore must be interruptible), and one during 'virsh save' or 'virsh managedsave' (which is currently non-live - we pause the guest, do the migration, then kill qemu; resuming requires starting a new qemu). In both situations, my understanding is that qemu does throttling based on whether it gets EAGAIN from the fd it is writing to, and that it doesn't regain responsiveness if it doesn't get EAGAIN. For the dump case, where we might have requested the live flag, we need the responsiveness. I'm just worried whether the save case also needs to present an EAGAIN interface to qemu (it would be nice if we didn't have to rely on that, so that we could change the throttling speeds - after all, we had to revert the patch that provided unlimited throttling when saving to disk because qemu didn't handle it well). -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org