On 07/19/2011 10:10 AM, Daniel P. Berrange wrote: Absolutely a good idea! I'll fold that rename into v2, particularly since, if posix_fadvise ever gets smarter on Linux, then we can bypass the cache without using O_DIRECT. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 07/14/2011 08:24 AM, Eric Blake wrote: Squash this in if my on-the-wire enum verification goes in first: https://www.redhat.com/archives/libvir-list/2011-July/msg00862.html diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index e20b741..8e90287 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -1865,4 +1865,5 @@ enum remote_procedure { REMOTE_PROC_NODE_GET_MEMORY_STATS = 228, REMOTE_PROC_DOMAIN_GET_CONTROL_INFO = 229, REMOTE_PROC_DOMAIN_GET_VCPU_PIN_INFO = 230, + REMOTE_PROC_DOMAIN_SAVE_FLAGS = 231, }; -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

For all hypervisors that support save, the new API now performs the same functions as the old. * src/libxl/libxl_driver.c (libxlDomainSave): Move guts... (libxlDomainSaveFlags): ...to new function. * src/test/test_driver.c (testDomainSave, testDomainSaveFlags): Likewise. * src/vbox/vbox_tmpl.c (vboxDomainSave, vboxDomainSaveFlags): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainSave) (xenUnifiedDomainSaveFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainSave): Rename and move guts. (qemuDomainSave, qemuDomainSaveFlags): ...here. (qemudDomainSaveFlag): Rename... (qemuDomainSaveInternal): ...to this, and update callers. --- src/libxl/libxl_driver.c | 17 ++++++++++++++++- src/qemu/qemu_driver.c | 31 ++++++++++++++++++++++++------- src/test/test_driver.c | 20 ++++++++++++++++++-- src/vbox/vbox_tmpl.c | 19 ++++++++++++++++++- src/xen/xen_driver.c | 17 ++++++++++++++++- 5 files changed, 92 insertions(+), 12 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 2bc998d..a88d0dc 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1906,12 +1906,20 @@ cleanup: } static int -libxlDomainSave(virDomainPtr dom, const char *to) +libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, + unsigned int flags) { libxlDriverPrivatePtr driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; + virCheckFlags(0, -1); + if (dxml) { + libxlError(VIR_ERR_INVALID_ARG, "%s", + _("xml modification unsupported")); + return -1; + } + libxlDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -1938,6 +1946,12 @@ cleanup: } static int +libxlDomainSave(virDomainPtr dom, const char *to) +{ + return libxlDomainSaveFlags(dom, to, NULL, 0); +} + +static int libxlDomainRestore(virConnectPtr conn, const char *from) { libxlDriverPrivatePtr driver = conn->privateData; @@ -3822,6 +3836,7 @@ static virDriver libxlDriver = { .domainGetInfo = libxlDomainGetInfo, /* 0.9.0 */ .domainGetState = libxlDomainGetState, /* 0.9.2 */ .domainSave = libxlDomainSave, /* 0.9.2 */ + .domainSaveFlags = libxlDomainSaveFlags, /* 0.9.4 */ .domainRestore = libxlDomainRestore, /* 0.9.2 */ .domainCoreDump = libxlDomainCoreDump, /* 0.9.2 */ .domainSetVcpus = libxlDomainSetVcpus, /* 0.9.0 */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f33abab..ceeef1c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2107,9 +2107,10 @@ qemuCompressProgramName(int compress) * shutdown). So 'vm' must not be referenced by the caller after * this returns (whether returning success or failure). */ -static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, - virDomainObjPtr vm, const char *path, - int compressed) +static int +qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, + virDomainObjPtr vm, const char *path, + int compressed) { char *xml = NULL; struct qemud_save_header header; @@ -2351,13 +2352,22 @@ static bool qemudCompressProgramAvailable(enum qemud_save_formats compress) return true; } -static int qemudDomainSave(virDomainPtr dom, const char *path) +static int +qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int compressed; int ret = -1; virDomainObjPtr vm = NULL; + virCheckFlags(0, -1); + if (dxml) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("xml modification unsupported")); + return -1; + } + qemuDriverLock(driver); if (driver->saveImageFormat == NULL) @@ -2393,7 +2403,7 @@ static int qemudDomainSave(virDomainPtr dom, const char *path) goto cleanup; } - ret = qemudDomainSaveFlag(driver, dom, vm, path, compressed); + ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed); vm = NULL; cleanup: @@ -2404,6 +2414,12 @@ cleanup: return ret; } +static int +qemuDomainSave(virDomainPtr dom, const char *path) +{ + return qemuDomainSaveFlags(dom, path, NULL, 0); +} + static char * qemuDomainManagedSavePath(struct qemud_driver *driver, virDomainObjPtr vm) { char *ret; @@ -2450,7 +2466,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) VIR_INFO("Saving state to %s", name); compressed = QEMUD_SAVE_FORMAT_RAW; - ret = qemudDomainSaveFlag(driver, dom, vm, name, compressed); + ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed); vm = NULL; cleanup: @@ -8578,7 +8594,8 @@ static virDriver qemuDriver = { .domainGetInfo = qemudDomainGetInfo, /* 0.2.0 */ .domainGetState = qemuDomainGetState, /* 0.9.2 */ .domainGetControlInfo = qemuDomainGetControlInfo, /* 0.9.3 */ - .domainSave = qemudDomainSave, /* 0.2.0 */ + .domainSave = qemuDomainSave, /* 0.2.0 */ + .domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */ .domainRestore = qemuDomainRestore, /* 0.2.0 */ .domainCoreDump = qemudDomainCoreDump, /* 0.7.0 */ .domainScreenshot = qemuDomainScreenshot, /* 0.9.2 */ diff --git a/src/test/test_driver.c b/src/test/test_driver.c index eb800f5..665a344 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1726,8 +1726,9 @@ cleanup: #define TEST_SAVE_MAGIC "TestGuestMagic" -static int testDomainSave(virDomainPtr domain, - const char *path) +static int +testDomainSaveFlags(virDomainPtr domain, const char *path, + const char *dxml, unsigned int flags) { testConnPtr privconn = domain->conn->privateData; char *xml = NULL; @@ -1737,6 +1738,13 @@ static int testDomainSave(virDomainPtr domain, virDomainEventPtr event = NULL; int ret = -1; + virCheckFlags(0, -1); + if (dxml) { + testError(VIR_ERR_INVALID_ARG, "%s", + _("xml modification unsupported")); + return -1; + } + testDriverLock(privconn); privdom = virDomainFindByName(&privconn->domains, domain->name); @@ -1820,6 +1828,13 @@ cleanup: return ret; } +static int +testDomainSave(virDomainPtr domain, + const char *path) +{ + return testDomainSaveFlags(domain, path, NULL, 0); +} + static int testDomainRestore(virConnectPtr conn, const char *path) { @@ -5543,6 +5558,7 @@ static virDriver testDriver = { .domainGetInfo = testGetDomainInfo, /* 0.1.1 */ .domainGetState = testDomainGetState, /* 0.9.2 */ .domainSave = testDomainSave, /* 0.3.2 */ + .domainSaveFlags = testDomainSaveFlags, /* 0.9.4 */ .domainRestore = testDomainRestore, /* 0.3.2 */ .domainCoreDump = testDomainCoreDump, /* 0.3.2 */ .domainSetVcpus = testSetVcpus, /* 0.1.4 */ diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 79423ee..c81d1a9 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -1979,13 +1979,23 @@ cleanup: return ret; } -static int vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) { +static int +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED, + const char *dxml, unsigned int flags) +{ VBOX_OBJECT_CHECK(dom->conn, int, -1); IConsole *console = NULL; vboxIID iid = VBOX_IID_INITIALIZER; IMachine *machine = NULL; nsresult rc; + virCheckFlags(0, -1); + if (dxml) { + vboxError(VIR_ERR_INVALID_ARG, "%s", + _("xml modification unsupported")); + return -1; + } + /* VirtualBox currently doesn't support saving to a file * at a location other then the machine folder and thus * setting path to ATTRIBUTE_UNUSED for now, will change @@ -2040,6 +2050,12 @@ static int vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED) { } static int +vboxDomainSave(virDomainPtr dom, const char *path) +{ + return vboxDomainSaveFlags(dom, path, NULL, 0); +} + +static int vboxDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, unsigned int flags) { @@ -8781,6 +8797,7 @@ virDriver NAME(Driver) = { .domainGetInfo = vboxDomainGetInfo, /* 0.6.3 */ .domainGetState = vboxDomainGetState, /* 0.9.2 */ .domainSave = vboxDomainSave, /* 0.6.3 */ + .domainSaveFlags = vboxDomainSaveFlags, /* 0.9.4 */ .domainSetVcpus = vboxDomainSetVcpus, /* 0.7.1 */ .domainSetVcpusFlags = vboxDomainSetVcpusFlags, /* 0.8.5 */ .domainGetVcpusFlags = vboxDomainGetVcpusFlags, /* 0.8.5 */ diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index 83c28db..01c9c4a 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1051,11 +1051,19 @@ xenUnifiedDomainGetState(virDomainPtr dom, } static int -xenUnifiedDomainSave (virDomainPtr dom, const char *to) +xenUnifiedDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml, + unsigned int flags) { GET_PRIVATE(dom->conn); int i; + virCheckFlags(0, -1); + if (dxml) { + xenUnifiedError(VIR_ERR_INVALID_ARG, "%s", + _("xml modification unsupported")); + return -1; + } + for (i = 0; i < XEN_UNIFIED_NR_DRIVERS; ++i) if (priv->opened[i] && drivers[i]->domainSave && @@ -1066,6 +1074,12 @@ xenUnifiedDomainSave (virDomainPtr dom, const char *to) } static int +xenUnifiedDomainSave(virDomainPtr dom, const char *to) +{ + return xenUnifiedDomainSaveFlags(dom, to, NULL, 0); +} + +static int xenUnifiedDomainRestore (virConnectPtr conn, const char *from) { GET_PRIVATE(conn); @@ -2200,6 +2214,7 @@ static virDriver xenUnifiedDriver = { .domainGetInfo = xenUnifiedDomainGetInfo, /* 0.0.3 */ .domainGetState = xenUnifiedDomainGetState, /* 0.9.2 */ .domainSave = xenUnifiedDomainSave, /* 0.0.3 */ + .domainSaveFlags = xenUnifiedDomainSaveFlags, /* 0.9.4 */ .domainRestore = xenUnifiedDomainRestore, /* 0.0.3 */ .domainCoreDump = xenUnifiedDomainCoreDump, /* 0.1.9 */ .domainSetVcpus = xenUnifiedDomainSetVcpus, /* 0.1.4 */ -- 1.7.4.4

On Thu, Jul 14, 2011 at 08:24:31AM -0600, Eric Blake wrote: My question about flag naming also applies here Nit pick, for checking return values I prefr '< 0' or '>= 0' rather than '== 0' Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 07/14/2011 08:24 AM, Eric Blake wrote: Squash this in if my v2 command patch goes in first: https://www.redhat.com/archives/libvir-list/2011-July/msg00864.html diff --git i/src/fdstream.c w/src/fdstream.c index 754431b..b70c831 100644 --- i/src/fdstream.c +++ w/src/fdstream.c @@ -570,7 +570,7 @@ virFDStreamOpenFileInternal(virStreamPtr st, path, NULL); virCommandAddArgFormat(cmd, "%llu", length); - virCommandTransferFD(cmd, fd); + virCommandTransferFD(cmd, &fd); virCommandAddArgFormat(cmd, "%d", fd); if (oflags == O_RDONLY) { -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Required for a coming patch where iohelper will operate on O_DIRECT fds. There, the user-space memory must be aligned to file system boundaries (at least page-aligned works best, and some file systems prefer 64k). Made tougher by the fact that VIR_ALLOC won't work on void *, but posix_memalign won't work on char * and isn't available everywhere. This patch makes some simplifying assumptions - namely, output to an O_DIRECT fd will only be attempted on an empty seekable file (hence, no need to worry about preserving existing data on a partial block, and ftruncate will work to undo the effects of having to round up the size of the last block written). * configure.ac (AC_CHECK_FUNCS_ONCE): Check for posix_memalign. * src/util/iohelper.c (runIO): Use aligned memory, and handle quirks of O_DIRECT on last write. --- This one took me the longest time to get right, so a careful review is appreciated. configure.ac | 6 +++--- src/util/iohelper.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index e9d5be4..9e39f44 100644 --- a/configure.ac +++ b/configure.ac @@ -121,9 +121,9 @@ AC_CHECK_SIZEOF([long]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions -AC_CHECK_FUNCS_ONCE([cfmakeraw regexec sched_getaffinity getuid getgid \ - geteuid initgroups posix_fallocate mmap kill \ - getmntent_r getgrnam_r getpwuid_r]) +AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ + getpwuid_r getuid initgroups kill mmap posix_fallocate posix_memalign \ + regexec sched_getaffinity]) dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 1185bde..82f62e1 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -74,17 +74,32 @@ cleanup: static int runIO(const char *path, int fd, int oflags, unsigned long long length) { - char *buf = NULL; + void *base = NULL; /* Location to be freed */ + char *buf = NULL; /* Aligned location within base */ size_t buflen = 1024*1024; + intptr_t alignMask = 64*1024 - 1; int ret = -1; int fdin, fdout; const char *fdinname, *fdoutname; unsigned long long total = 0; + bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); + bool shortRead = false; /* true if we hit a short read */ + off_t end = 0; - if (VIR_ALLOC_N(buf, buflen) < 0) { +#if HAVE_POSIX_MEMALIGN + if (posix_memalign(&base, alignMask + 1, buflen)) { virReportOOMError(); goto cleanup; } + buf = base; +#else + if (VIR_ALLOC_N(buf, buflen + alignMask) < 0) { + virReportOOMError(); + goto cleanup; + } + base = buf; + buf = (char *) (((intptr_t) base + alignMask) & alignMask); +#endif switch (oflags & O_ACCMODE) { case O_RDONLY: @@ -98,6 +113,13 @@ runIO(const char *path, int fd, int oflags, unsigned long long length) fdinname = "stdin"; fdout = fd; fdoutname = path; + /* To make the implementation simpler, we give up on any + * attempt to use O_DIRECT in a non-trivial manner. */ + if (direct && (end = lseek(fd, 0, SEEK_END)) != 0) { + virReportSystemError(end < 0 ? errno : EINVAL, "%s", + _("O_DIRECT needs empty seekable file")); + goto cleanup; + } break; case O_RDWR: @@ -124,12 +146,29 @@ runIO(const char *path, int fd, int oflags, unsigned long long length) } if (got == 0) break; /* End of file before end of requested data */ + if (got < buflen || (buflen & alignMask)) { + /* O_DIRECT can handle at most one short read, at end of file */ + if (direct && shortRead) { + virReportSystemError(EINVAL, "%s", + _("Too many short reads for O_DIRECT")); + } + shortRead = true; + } total += got; + if (fdout == fd && direct && shortRead) { + end = total; + memset(buf + got, 0, buflen - got); + got = (got + alignMask) & ~alignMask; + } if (safewrite(fdout, buf, got) < 0) { virReportSystemError(errno, _("Unable to write %s"), fdoutname); goto cleanup; } + if (end && ftruncate(fd, end) < 0) { + virReportSystemError(errno, _("Unable to truncate %s"), fdoutname); + goto cleanup; + } } ret = 0; @@ -141,7 +180,7 @@ cleanup: ret = -1; } - VIR_FREE(buf); + VIR_FREE(base); return ret; } -- 1.7.4.4

O_DIRECT has stringent requirements - I/O must occur with buffers that have both alignment and size as multiples of the file system block size (used to be 512 bytes, but these days, 4k is safer, and 64k allows for better throughput). Rather than make lots of changes at each site that wants to use O_DIRECT, it is easier to offload the work through a helper process that mirrors the I/O between a pipe and the actual direct fd, so that the other end of the pipe no longer has to worry about constraints. * src/util/virdirect.h: New file. * src/util/virdirect.c: Likewise. * src/Makefile.am (UTIL_SOURCES): Build them. * src/libvirt_private.syms: Export new symbols. * cfg.mk (useless_free_options): Add to list. * po/POTFILES.in: Translate new file. --- cfg.mk | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 6 ++ src/util/virdirect.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virdirect.h | 37 +++++++++++ 6 files changed, 195 insertions(+), 0 deletions(-) create mode 100644 src/util/virdirect.c create mode 100644 src/util/virdirect.h diff --git a/cfg.mk b/cfg.mk index 2873177..69d2b6a 100644 --- a/cfg.mk +++ b/cfg.mk @@ -97,6 +97,7 @@ useless_free_options = \ --name=virCommandFree \ --name=virConfFreeList \ --name=virConfFreeValue \ + --name=virDirectFdFree \ --name=virDomainChrDefFree \ --name=virDomainChrSourceDefFree \ --name=virDomainControllerDefFree \ diff --git a/po/POTFILES.in b/po/POTFILES.in index 5782cbf..ad031f3 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -122,6 +122,7 @@ src/util/storage_file.c src/util/sysinfo.c src/util/util.c src/util/viraudit.c +src/util/virdirect.c src/util/virterror.c src/util/xml.c src/vbox/vbox_MSCOMGlue.c diff --git a/src/Makefile.am b/src/Makefile.am index d19d1ca..ce041b4 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -80,6 +80,7 @@ UTIL_SOURCES = \ util/uuid.c util/uuid.h \ util/util.c util/util.h \ util/viraudit.c util/viraudit.h \ + util/virdirect.c util/virdirect.h \ util/xml.c util/xml.h \ util/virterror.c util/virterror_internal.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f95d341..c78485d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1085,6 +1085,12 @@ virAuditOpen; virAuditSend; +# virdirect.h +virDirectFdClose; +virDirectFdFree; +virDirectFdNew; + + # virterror_internal.h virDispatchError; virErrorMsg; diff --git a/src/util/virdirect.c b/src/util/virdirect.c new file mode 100644 index 0000000..a3dca77 --- /dev/null +++ b/src/util/virdirect.c @@ -0,0 +1,149 @@ +/* + * direct.c: management of O_DIRECT fds + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include <config.h> +#include "internal.h" + +#include "virdirect.h" + +#include <fcntl.h> +#include <sys/stat.h> + +#include "command.h" +#include "configmake.h" +#include "files.h" +#include "memory.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_NONE +#define virDirectError(code, ...) \ + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +/* Opaque type for managing a wrapper around an O_DIRECT fd. For now, + * read-write is not supported, just a single direction. */ +struct _virDirectFd { + virCommandPtr cmd; /* Child iohelper process to do the I/O. */ +}; + +/* Replace *FD (open to an O_DIRECT file or device) with a new (pipe) + * fd that will pass all I/O through a child process, in order to obey + * O_DIRECT restrictions when doing I/O to the original fd. NAME is + * for diagnostics only. On success, the new wrapper object is + * returned, the original fd is no longer open in the parent process, + * and the caller must close the new *FD descriptor. On failure, *FD + * is unchanged, an error message is output, and NULL is returned. */ +virDirectFdPtr +virDirectFdNew(int *fd, const char *name) +{ + virDirectFdPtr ret = NULL; + bool output = false; + int pipefd[2] = { -1, -1 }; + int mode = -1; + + if (VIR_ALLOC(ret) < 0) { + virReportOOMError(); + goto error; + } + if (!O_DIRECT) + return ret; + +#ifdef F_GETFL + /* Mingw lacks F_GETFL, but it also lacks O_DIRECT, in which case + * we don't need a child process in the first place. */ + mode = fcntl(*fd, F_GETFL); +#endif + + if (mode < 0) { + virDirectError(VIR_ERR_INTERNAL_ERROR, _("invalid fd %d for %s"), + *fd, name); + goto error; + } else if ((mode & O_ACCMODE) == O_WRONLY) { + output = true; + } else if ((mode & O_ACCMODE) != O_RDONLY) { + virDirectError(VIR_ERR_INTERNAL_ERROR, _("unexpected mode %x for %s"), + mode & O_ACCMODE, name); + goto error; + } + + if (pipe2(pipefd, O_CLOEXEC) < 0) { + virDirectError(VIR_ERR_INTERNAL_ERROR, + _("unable to create pipe for %s"), name); + goto error; + } + + ret->cmd = virCommandNewArgList(LIBEXECDIR "/libvirt_iohelper", + name, "0", NULL); + if (output) { + virCommandSetInputFD(ret->cmd, pipefd[0]); + virCommandSetOutputFD(ret->cmd, fd); + virCommandAddArg(ret->cmd, "1"); + } else { + virCommandSetInputFD(ret->cmd, *fd); + virCommandSetOutputFD(ret->cmd, &pipefd[1]); + virCommandAddArg(ret->cmd, "0"); + } + + if (virCommandRunAsync(ret->cmd, NULL) < 0) + goto error; + + if (VIR_CLOSE(pipefd[!output]) < 0) { + virDirectError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to close pipe")); + goto error; + } + + VIR_FORCE_CLOSE(*fd); + *fd = pipefd[output]; + return ret; + +error: + VIR_FORCE_CLOSE(pipefd[0]); + VIR_FORCE_CLOSE(pipefd[1]); + virDirectFdFree(ret); + return NULL; +} + +/* If DFD is valid, then reap the child process. Return 0 if the + * child process was successfully reaped, or -1 on failure with an + * error emitted. This function intentionally returns 0 when DFD is + * not valid, so that callers can conditionally create a virDirectFd + * wrapper but unconditionally call the cleanup code. */ +int +virDirectFdClose(virDirectFdPtr dfd) +{ + if (!dfd || !O_DIRECT) + return 0; + + return virCommandWait(dfd->cmd, NULL); +} + +/* Free all resources associated with DFD. If virDirectFdClose was + * not previously called, then this forcefully terminates the child + * process. */ +void +virDirectFdFree(virDirectFdPtr dfd) +{ + if (!dfd) + return; + + virCommandFree(dfd->cmd); + VIR_FREE(dfd); +} diff --git a/src/util/virdirect.h b/src/util/virdirect.h new file mode 100644 index 0000000..35f5f6b --- /dev/null +++ b/src/util/virdirect.h @@ -0,0 +1,37 @@ +/* + * direct.h: management of O_DIRECT fds + * + * Copyright (C) 2011 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#ifndef __VIR_DIRECT_H__ +# define __VIR_DIRECT_H__ + +/* Opaque type for managing a wrapper around an O_DIRECT fd. */ +struct _virDirectFd; + +typedef struct _virDirectFd virDirectFd; +typedef virDirectFd *virDirectFdPtr; + +virDirectFdPtr virDirectFdNew(int *fd, const char *name) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int virDirectFdClose(virDirectFdPtr dfd); + +void virDirectFdFree(virDirectFdPtr dfd); + +#endif /* __VIR_DIRECT_H__ */ -- 1.7.4.4

On 07/19/2011 11:45 AM, Daniel P. Berrange wrote: Sure, will do that on my v2 respin. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

libvirt-guests is a perfect use case for O_DIRECT - lots of filesystem traffic done at system shutdown, where caching is pointless, and startup, where caching gets in the way. Make this a configurable option in the init script, but defaulting to existing behavior. * tools/libvirt-guests.sysconf (SAVE_DIRECT): New variable. * tools/libvirt-guests.init.sh (suspend_guest): Use it. --- tools/libvirt-guests.init.sh | 5 ++++- tools/libvirt-guests.sysconf | 4 ++++ 2 files changed, 8 insertions(+), 1 deletions(-) diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh index 30f957a..4e383c4 100644 --- a/tools/libvirt-guests.init.sh +++ b/tools/libvirt-guests.init.sh @@ -43,6 +43,7 @@ ON_BOOT=start ON_SHUTDOWN=suspend SHUTDOWN_TIMEOUT=0 START_DELAY=0 +SAVE_DIRECT=0 test -f "$sysconfdir"/sysconfig/libvirt-guests && . "$sysconfdir"/sysconfig/libvirt-guests @@ -190,8 +191,10 @@ suspend_guest() name=$(guest_name "$uri" "$guest") label=$(eval_gettext "Suspending \$name: ") + direct= + test "x$SAVE_DIRECT" = x0 || direct=--direct printf %s "$label" - run_virsh "$uri" managedsave "$guest" >/dev/null & + run_virsh "$uri" managedsave "$direct" "$guest" >/dev/null & virsh_pid=$! while true; do sleep 1 diff --git a/tools/libvirt-guests.sysconf b/tools/libvirt-guests.sysconf index 37b258e..da465ad 100644 --- a/tools/libvirt-guests.sysconf +++ b/tools/libvirt-guests.sysconf @@ -25,3 +25,7 @@ # number of seconds we're willing to wait for a guest to shut down #SHUTDOWN_TIMEOUT=0 + +# If non-zero, try to use O_DIRECT when saving and restoring guests, +# for less filesystem cache pollution, but possibly slower operation. +#SAVE_DIRECT=0 -- 1.7.4.4

On Thu, Jul 14, 2011 at 07:02:23PM -0600, Eric Blake wrote: Same note about flag naming as first patch ACK to new API Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, Jul 15, 2011 at 08:42:44PM -0600, Eric Blake wrote: ACK when flag name question is resolved. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, Jul 15, 2011 at 08:48:18PM -0600, Eric Blake wrote: ACK, if this really works for saving guests. We need to skip over the initial header + XML in save files, so I would have thought we needed to support a non-empty seekable file. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Wire up the restore direction to use iohelper for O_DIRECT. * src/qemu/qemu_driver.c (qemuddDomainObjStart) (qemuDomainSaveImageOpen, qemuDomainObjRestore) (qemuDomainObjStart): Add parameter. (qemudDomainStartWithFlags, qemuAutostartDomain) (qemuDomainRestoreFlags): Update callers. --- Could be squashed into 8/8. src/qemu/qemu_driver.c | 80 ++++++++++++++++++++++++++++++----------------- 1 files changed, 51 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a42ff8e..27b971f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -117,11 +117,12 @@ static void processWatchdogEvent(void *data, void *opaque); static int qemudShutdown(void); -static int qemudDomainObjStart(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - bool start_paused, - bool autodestroy); +static int qemuDomainObjStart(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_paused, + bool autodestroy, + bool direct); static int qemudDomainGetMaxVcpus(virDomainPtr dom); @@ -149,9 +150,11 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq vm->def->name, err ? err->message : _("unknown error")); } else { + /* XXX need to wire direct autostart into qemu.conf */ if (vm->autostart && !virDomainObjIsActive(vm) && - qemudDomainObjStart(data->conn, data->driver, vm, false, false) < 0) { + qemuDomainObjStart(data->conn, data->driver, vm, + false, false, false) < 0) { err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, @@ -3604,18 +3607,20 @@ cleanup: return ret; } -static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) +static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, - struct qemud_save_header *ret_header) + struct qemud_save_header *ret_header, + bool direct, virDirectFdPtr *directFd) { int fd; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; - if ((fd = virFileOpenAs(path, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) { + if ((fd = virFileOpenAs(path, O_RDONLY | (direct ? O_DIRECT : 0), 0, + getuid(), getgid(), 0)) < 0) { if ((fd != -EACCES && fd != -EPERM) || driver->user == getuid()) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3625,7 +3630,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, /* Opening as root failed, but qemu runs as a different user * that might have better luck. */ - if ((fd = virFileOpenAs(path, O_RDONLY, 0, + if ((fd = virFileOpenAs(path, O_RDONLY | (direct ? O_DIRECT : 0), 0, driver->user, driver->group, VIR_FILE_OPEN_AS_UID)) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3633,6 +3638,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, goto error; } } + if (direct && (*directFd = virDirectFdNew(&fd, path)) == NULL) + goto error; if (saferead(fd, &header, sizeof(header)) != sizeof(header)) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3811,8 +3818,9 @@ qemuDomainRestoreFlags(virConnectPtr conn, int fd = -1; int ret = -1; struct qemud_save_header header; + virDirectFdPtr directFd = NULL; - virCheckFlags(0, -1); + virCheckFlags(VIR_DOMAIN_SAVE_DIRECT, -1); if (dxml) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("xml modification unsupported")); @@ -3821,7 +3829,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, qemuDriverLock(driver); - fd = qemuDomainSaveImageOpen(driver, path, &def, &header); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, + flags & VIR_DOMAIN_SAVE_DIRECT, &directFd); if (fd < 0) goto cleanup; @@ -3840,6 +3849,8 @@ qemuDomainRestoreFlags(virConnectPtr conn, goto cleanup; ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + if (virDirectFdClose(directFd) < 0) + VIR_WARN("Failed to close %s", path); if (qemuDomainObjEndJob(driver, vm) == 0) vm = NULL; @@ -3851,6 +3862,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); + virDirectFdFree(directFd); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); @@ -3868,14 +3880,17 @@ static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, virDomainObjPtr vm, - const char *path) + const char *path, + bool direct) { virDomainDefPtr def = NULL; int fd = -1; int ret = -1; struct qemud_save_header header; + virDirectFdPtr directFd = NULL; - fd = qemuDomainSaveImageOpen(driver, path, &def, &header); + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, + direct, &directFd); if (fd < 0) goto cleanup; @@ -3897,10 +3912,13 @@ qemuDomainObjRestore(virConnectPtr conn, def = NULL; ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path); + if (virDirectFdClose(directFd) < 0) + VIR_WARN("Failed to close %s", path); cleanup: virDomainDefFree(def); VIR_FORCE_CLOSE(fd); + virDirectFdFree(directFd); return ret; } @@ -4114,11 +4132,13 @@ static int qemudNumDefinedDomains(virConnectPtr conn) { } -static int qemudDomainObjStart(virConnectPtr conn, - struct qemud_driver *driver, - virDomainObjPtr vm, - bool start_paused, - bool autodestroy) +static int +qemuDomainObjStart(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm, + bool start_paused, + bool autodestroy, + bool direct) { int ret = -1; char *managed_save; @@ -4133,7 +4153,7 @@ static int qemudDomainObjStart(virConnectPtr conn, goto cleanup; if (virFileExists(managed_save)) { - ret = qemuDomainObjRestore(conn, driver, vm, managed_save); + ret = qemuDomainObjRestore(conn, driver, vm, managed_save, direct); if ((ret == 0) && (unlink(managed_save) < 0)) VIR_WARN("Failed to remove the managed state %s", managed_save); @@ -4159,14 +4179,15 @@ cleanup: } static int -qemudDomainStartWithFlags(virDomainPtr dom, unsigned int flags) +qemuDomainStartWithFlags(virDomainPtr dom, unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; int ret = -1; virCheckFlags(VIR_DOMAIN_START_PAUSED | - VIR_DOMAIN_START_AUTODESTROY, -1); + VIR_DOMAIN_START_AUTODESTROY | + VIR_DOMAIN_START_DIRECT, -1); qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -4188,9 +4209,10 @@ qemudDomainStartWithFlags(virDomainPtr dom, unsigned int flags) goto endjob; } - if (qemudDomainObjStart(dom->conn, driver, vm, - (flags & VIR_DOMAIN_START_PAUSED) != 0, - (flags & VIR_DOMAIN_START_AUTODESTROY) != 0) < 0) + if (qemuDomainObjStart(dom->conn, driver, vm, + (flags & VIR_DOMAIN_START_PAUSED) != 0, + (flags & VIR_DOMAIN_START_AUTODESTROY) != 0, + (flags & VIR_DOMAIN_START_DIRECT) != 0) < 0) goto endjob; ret = 0; @@ -4207,9 +4229,9 @@ cleanup: } static int -qemudDomainStart(virDomainPtr dom) +qemuDomainStart(virDomainPtr dom) { - return qemudDomainStartWithFlags(dom, 0); + return qemuDomainStartWithFlags(dom, 0); } static int @@ -8615,8 +8637,8 @@ static virDriver qemuDriver = { .domainXMLToNative = qemuDomainXMLToNative, /* 0.6.4 */ .listDefinedDomains = qemudListDefinedDomains, /* 0.2.0 */ .numOfDefinedDomains = qemudNumDefinedDomains, /* 0.2.0 */ - .domainCreate = qemudDomainStart, /* 0.2.0 */ - .domainCreateWithFlags = qemudDomainStartWithFlags, /* 0.8.2 */ + .domainCreate = qemuDomainStart, /* 0.2.0 */ + .domainCreateWithFlags = qemuDomainStartWithFlags, /* 0.8.2 */ .domainDefineXML = qemudDomainDefine, /* 0.2.0 */ .domainUndefine = qemudDomainUndefine, /* 0.2.0 */ .domainAttachDevice = qemuDomainAttachDevice, /* 0.4.1 */ -- 1.7.4.4

When auto-starting a domain on libvirtd startup, let the user configure whether to have the VIR_DOMAIN_START_DIRECT flag effect. * src/qemu/qemu.conf (auto_start_direct): Document new variable. * src/qemu/libvirtd_qemu.aug (vnc_entry): Let augeas parse it. * src/qemu/qemu_conf.h (qemud_driver): Store new preference. * src/qemu/qemu_conf.c (qemudLoadDriverConfig): Parse it. * src/qemu/qemu_driver.c (qemuAutostartDomain): Honor it. --- Might be worth combining with 9/8. src/qemu/libvirtd_qemu.aug | 1 + src/qemu/qemu.conf | 8 ++++++++ src/qemu/qemu_conf.c | 4 ++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_driver.c | 4 ++-- 5 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index dea6770..a78cd10 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -42,6 +42,7 @@ module Libvirtd_qemu = | str_entry "dump_image_format" | str_entry "auto_dump_path" | bool_entry "auto_dump_direct" + | bool_entry "auto_start_direct" | str_entry "hugetlbfs_mount" | bool_entry "relaxed_acs_check" | bool_entry "vnc_allow_host_audio" diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 2a0664d..48ae781 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -216,6 +216,14 @@ # # auto_dump_direct = 0 +# When a domain is configured to be auto-started, enabling this flag +# has the same effect as using the VIR_DOMAIN_START_DIRECT flag with the +# virDomainCreateWithFlags API. That is, the system uses O_DIRECT if +# possible, which puts less pressure on the file system cache but may +# cause slower operation. +# +# auto_start_direct = 0 + # If provided by the host and a hugetlbfs mount point is configured, # a guest may request huge page backing. When this mount point is # unspecified here, determination of a host mount point in /proc/mounts diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index cf6cb6b..144dbda 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -382,6 +382,10 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, CHECK_TYPE ("auto_dump_direct", VIR_CONF_LONG); if (p) driver->autoDumpDirect = true; + p = virConfGetValue (conf, "auto_start_direct"); + CHECK_TYPE ("auto_start_direct", VIR_CONF_LONG); + if (p) driver->autoStartDirect = true; + p = virConfGetValue (conf, "hugetlbfs_mount"); CHECK_TYPE ("hugetlbfs_mount", VIR_CONF_STRING); if (p && p->str) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index bc025af..afc3ef4 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -121,6 +121,8 @@ struct qemud_driver { char *autoDumpPath; bool autoDumpDirect; + bool autoStartDirect; + pciDeviceList *activePciHostdevs; virBitmapPtr reservedVNCPorts; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 27b971f..64fe3b9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -150,11 +150,11 @@ qemuAutostartDomain(void *payload, const void *name ATTRIBUTE_UNUSED, void *opaq vm->def->name, err ? err->message : _("unknown error")); } else { - /* XXX need to wire direct autostart into qemu.conf */ if (vm->autostart && !virDomainObjIsActive(vm) && qemuDomainObjStart(data->conn, data->driver, vm, - false, false, false) < 0) { + false, false, + data->driver->autoStartDirect) < 0) { err = virGetLastError(); VIR_ERROR(_("Failed to autostart VM '%s': %s"), vm->def->name, -- 1.7.4.4

* tools/libvirt-guests.init.sh (start): Use SAVE_DIRECT. --- Patch 10/8 has a bug fixed here - the code must use $direct rather than "$direct" so as to elide the argument rather than inject an empty string argument when the option is not in use. The more I look at this series, the more I like the idea of adding both save and restore direct support at the same time, so I'll post a v2 that rebases things accordingly. tools/libvirt-guests.init.sh | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/libvirt-guests.init.sh b/tools/libvirt-guests.init.sh index 4e383c4..7e1dd51 100644 --- a/tools/libvirt-guests.init.sh +++ b/tools/libvirt-guests.init.sh @@ -144,6 +144,8 @@ start() { fi isfirst=true + direct= + test "x$SAVE_DIRECT" = x0 || direct=--direct while read uri list; do configured=false set -f @@ -173,7 +175,7 @@ start() { else sleep $START_DELAY fi - retval run_virsh "$uri" start "$name" >/dev/null && \ + retval run_virsh "$uri" start $direct "$name" >/dev/null && \ gettext "done"; echo fi fi @@ -194,7 +196,7 @@ suspend_guest() direct= test "x$SAVE_DIRECT" = x0 || direct=--direct printf %s "$label" - run_virsh "$uri" managedsave "$direct" "$guest" >/dev/null & + run_virsh "$uri" managedsave $direct "$guest" >/dev/null & virsh_pid=$! while true; do sleep 1 -- 1.7.4.4