[libvirt PATCH v3 0/2] iohelper refactoring of io copy function
changes v1 -> v2: * fix missing write changes v2 -> v3: * move error reporting back into the copy function * split into two patches, one introducing the IO parameters struct, and another introducing the new function. Claudio Fontana (2): iohelper: introduce new struct to carry copy operation parameters iohelper: refactor copy operation as a separate function src/util/iohelper.c | 170 ++++++++++++++++++++++++++------------------ 1 file changed, 100 insertions(+), 70 deletions(-) -- 2.34.1
this is in preparation for a minor refactoring of the copy function itself out of runIO(). Signed-off-by: Claudio Fontana <cfontana@suse.de> --- src/util/iohelper.c | 95 +++++++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 42 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index 2c91bf4f93..c13746a547 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -45,6 +45,16 @@ # define O_DIRECT 0 #endif +struct runIOParams { + bool isBlockDev; + bool isDirect; + bool isWrite; + int fdin; + const char *fdinname; + int fdout; + const char *fdoutname; +}; + static int runIO(const char *path, int fd, int oflags) { @@ -53,13 +63,9 @@ runIO(const char *path, int fd, int oflags) 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); - off_t end = 0; + off_t total = 0; struct stat sb; - bool isBlockDev = false; + struct runIOParams p; #if WITH_POSIX_MEMALIGN if (posix_memalign(&base, alignMask + 1, buflen)) @@ -77,34 +83,23 @@ runIO(const char *path, int fd, int oflags) fd, path); goto cleanup; } - isBlockDev = S_ISBLK(sb.st_mode); + p.isBlockDev = S_ISBLK(sb.st_mode); + p.isDirect = O_DIRECT && (oflags & O_DIRECT); switch (oflags & O_ACCMODE) { case O_RDONLY: - fdin = fd; - fdinname = path; - fdout = STDOUT_FILENO; - fdoutname = "stdout"; - /* To make the implementation simpler, we give up on any - * attempt to use O_DIRECT in a non-trivial manner. */ - if (!isBlockDev && direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { - virReportSystemError(end < 0 ? errno : EINVAL, "%s", - _("O_DIRECT read needs entire seekable file")); - goto cleanup; - } + p.isWrite = false; + p.fdin = fd; + p.fdinname = path; + p.fdout = STDOUT_FILENO; + p.fdoutname = "stdout"; break; case O_WRONLY: - fdin = STDIN_FILENO; - 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 (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) { - virReportSystemError(end < 0 ? errno : EINVAL, "%s", - _("O_DIRECT write needs empty seekable file")); - goto cleanup; - } + p.isWrite = true; + p.fdin = STDIN_FILENO; + p.fdinname = "stdin"; + p.fdout = fd; + p.fdoutname = path; break; case O_RDWR: @@ -114,6 +109,22 @@ runIO(const char *path, int fd, int oflags) (oflags & O_ACCMODE)); goto cleanup; } + /* To make the implementation simpler, we give up on any + * attempt to use O_DIRECT in a non-trivial manner. */ + if (!p.isBlockDev && p.isDirect) { + off_t off; + if (p.isWrite) { + if ((off = lseek(fd, 0, SEEK_END)) != 0) { + virReportSystemError(off < 0 ? errno : EINVAL, "%s", + _("O_DIRECT write needs empty seekable file")); + goto cleanup; + } + } else if ((off = lseek(fd, 0, SEEK_CUR)) != 0) { + virReportSystemError(off < 0 ? errno : EINVAL, "%s", + _("O_DIRECT read needs entire seekable file")); + goto cleanup; + } + } while (1) { ssize_t got; @@ -124,16 +135,16 @@ runIO(const char *path, int fd, int oflags) * writes will be aligned. * In other cases using saferead reduces number of syscalls. */ - if (fdin == fd && direct) { - if ((got = read(fdin, buf, buflen)) < 0 && + if (!p.isWrite && p.isDirect) { + if ((got = read(p.fdin, buf, buflen)) < 0 && errno == EINTR) continue; } else { - got = saferead(fdin, buf, buflen); + got = saferead(p.fdin, buf, buflen); } if (got < 0) { - virReportSystemError(errno, _("Unable to read %s"), fdinname); + virReportSystemError(errno, _("Unable to read %s"), p.fdinname); goto cleanup; } if (got == 0) @@ -142,35 +153,35 @@ runIO(const char *path, int fd, int oflags) total += got; /* handle last write size align in direct case */ - if (got < buflen && direct && fdout == fd) { + if (got < buflen && p.isDirect && p.isWrite) { ssize_t aligned_got = (got + alignMask) & ~alignMask; memset(buf + got, 0, aligned_got - got); - if (safewrite(fdout, buf, aligned_got) < 0) { - virReportSystemError(errno, _("Unable to write %s"), fdoutname); + if (safewrite(p.fdout, buf, aligned_got) < 0) { + virReportSystemError(errno, _("Unable to write %s"), p.fdoutname); goto cleanup; } - if (!isBlockDev && ftruncate(fd, total) < 0) { - virReportSystemError(errno, _("Unable to truncate %s"), fdoutname); + if (!p.isBlockDev && ftruncate(p.fdout, total) < 0) { + virReportSystemError(errno, _("Unable to truncate %s"), p.fdoutname); goto cleanup; } break; } - if (safewrite(fdout, buf, got) < 0) { - virReportSystemError(errno, _("Unable to write %s"), fdoutname); + if (safewrite(p.fdout, buf, got) < 0) { + virReportSystemError(errno, _("Unable to write %s"), p.fdoutname); goto cleanup; } } /* Ensure all data is written */ - if (virFileDataSync(fdout) < 0) { + if (virFileDataSync(p.fdout) < 0) { if (errno != EINVAL && errno != EROFS) { /* fdatasync() may fail on some special FDs, e.g. pipes */ - virReportSystemError(errno, _("unable to fsync %s"), fdoutname); + virReportSystemError(errno, _("unable to fsync %s"), p.fdoutname); goto cleanup; } } -- 2.34.1
Signed-off-by: Claudio Fontana <cfontana@suse.de> --- src/util/iohelper.c | 131 +++++++++++++++++++++++++------------------- 1 file changed, 75 insertions(+), 56 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index c13746a547..1584321839 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -55,17 +55,23 @@ struct runIOParams { const char *fdoutname; }; -static int -runIO(const char *path, int fd, int oflags) +/** + * runIOCopy: execute the IO copy based on the passed parameters + * @p: the IO parameters + * + * Execute the copy based on the passed parameters. + * + * Returns: size transfered, or < 0 on error. + */ + +static off_t +runIOCopy(const struct runIOParams p) { g_autofree 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; off_t total = 0; - struct stat sb; - struct runIOParams p; #if WITH_POSIX_MEMALIGN if (posix_memalign(&base, alignMask + 1, buflen)) @@ -77,6 +83,67 @@ runIO(const char *path, int fd, int oflags) buf = (char *) (((intptr_t) base + alignMask) & ~alignMask); #endif + while (1) { + ssize_t got; + + /* If we read with O_DIRECT from file we can't use saferead as + * it can lead to unaligned read after reading last bytes. + * If we write with O_DIRECT use should use saferead so that + * writes will be aligned. + * In other cases using saferead reduces number of syscalls. + */ + if (!p.isWrite && p.isDirect) { + if ((got = read(p.fdin, buf, buflen)) < 0 && + errno == EINTR) + continue; + } else { + got = saferead(p.fdin, buf, buflen); + } + + if (got < 0) { + virReportSystemError(errno, _("Unable to read %s"), p.fdinname); + return -2; + } + if (got == 0) + break; + + total += got; + + /* handle last write size align in direct case */ + if (got < buflen && p.isDirect && p.isWrite) { + ssize_t aligned_got = (got + alignMask) & ~alignMask; + + memset(buf + got, 0, aligned_got - got); + + if (safewrite(p.fdout, buf, aligned_got) < 0) { + virReportSystemError(errno, _("Unable to write %s"), p.fdoutname); + return -3; + } + + if (!p.isBlockDev && ftruncate(p.fdout, total) < 0) { + virReportSystemError(errno, _("Unable to truncate %s"), p.fdoutname); + return -4; + } + + break; + } + + if (safewrite(p.fdout, buf, got) < 0) { + virReportSystemError(errno, _("Unable to write %s"), p.fdoutname); + return -3; + } + } + return total; +} + +static int +runIO(const char *path, int fd, int oflags) +{ + int ret = -1; + off_t total = 0; + struct stat sb; + struct runIOParams p; + if (fstat(fd, &sb) < 0) { virReportSystemError(errno, _("Unable to access file descriptor %d path %s"), @@ -125,57 +192,9 @@ runIO(const char *path, int fd, int oflags) goto cleanup; } } - - while (1) { - ssize_t got; - - /* If we read with O_DIRECT from file we can't use saferead as - * it can lead to unaligned read after reading last bytes. - * If we write with O_DIRECT use should use saferead so that - * writes will be aligned. - * In other cases using saferead reduces number of syscalls. - */ - if (!p.isWrite && p.isDirect) { - if ((got = read(p.fdin, buf, buflen)) < 0 && - errno == EINTR) - continue; - } else { - got = saferead(p.fdin, buf, buflen); - } - - if (got < 0) { - virReportSystemError(errno, _("Unable to read %s"), p.fdinname); - goto cleanup; - } - if (got == 0) - break; - - total += got; - - /* handle last write size align in direct case */ - if (got < buflen && p.isDirect && p.isWrite) { - ssize_t aligned_got = (got + alignMask) & ~alignMask; - - memset(buf + got, 0, aligned_got - got); - - if (safewrite(p.fdout, buf, aligned_got) < 0) { - virReportSystemError(errno, _("Unable to write %s"), p.fdoutname); - goto cleanup; - } - - if (!p.isBlockDev && ftruncate(p.fdout, total) < 0) { - virReportSystemError(errno, _("Unable to truncate %s"), p.fdoutname); - goto cleanup; - } - - break; - } - - if (safewrite(p.fdout, buf, got) < 0) { - virReportSystemError(errno, _("Unable to write %s"), p.fdoutname); - goto cleanup; - } - } + total = runIOCopy(p); + if (total < 0) + goto cleanup; /* Ensure all data is written */ if (virFileDataSync(p.fdout) < 0) { -- 2.34.1
participants (1)
-
Claudio Fontana