this is of course broken (missing a write),
will send a v2 to fix it up.
Claudio
On 3/26/22 5:07 PM, Claudio Fontana wrote:
> while doing research with alternative implementations of runIO,
> it seemed necessary to do some refactoring, in order to separate
> parameter setting from the actual copy, so that alternative
> copy methods can be researched and hopefully eventually implemented.
>
> No functional changes are expected.
>
> Signed-off-by: Claudio Fontana <cfontana@xxxxxxx>
> ---
> src/util/iohelper.c | 193 +++++++++++++++++++++++++++-----------------
> 1 file changed, 118 insertions(+), 75 deletions(-)
>
> diff --git a/src/util/iohelper.c b/src/util/iohelper.c
> index 2c91bf4f93..c04c97af27 100644
> --- a/src/util/iohelper.c
> +++ b/src/util/iohelper.c
> @@ -45,21 +45,38 @@
> # define O_DIRECT 0
> #endif
>
> -static int
> -runIO(const char *path, int fd, int oflags)
> +struct runIOParams {
> + bool isBlockDev;
> + bool isDirect;
> + bool isWrite;
> + int fdin;
> + const char *fdinname;
> + int fdout;
> + const char *fdoutname;
> +};
> +
> +/* runIOCopy: execute the IO copy based on the passed parameters */
> +/**
> + * runIOCopy:
> + * @p: the IO parameters
> + *
> + * Execute the copy based on the passed parameters.
> + *
> + * Returns: size transfered, or < 0 on error.
> + * Errors: -1 = read/write error
> + * -2 = read error
> + * -3 = write error
> + * -4 = truncate error
> + */
> +
> +static ssize_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;
> - int fdin, fdout;
> - const char *fdinname, *fdoutname;
> - unsigned long long total = 0;
> - bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0);
> - off_t end = 0;
> - struct stat sb;
> - bool isBlockDev = false;
> + ssize_t total = 0;
>
> #if WITH_POSIX_MEMALIGN
> if (posix_memalign(&base, alignMask + 1, buflen))
> @@ -71,50 +88,6 @@ runIO(const char *path, int fd, int oflags)
> buf = (char *) (((intptr_t) base + alignMask) & ~alignMask);
> #endif
>
> - if (fstat(fd, &sb) < 0) {
> - virReportSystemError(errno,
> - _("Unable to access file descriptor %d path %s"),
> - fd, path);
> - goto cleanup;
> - }
> - isBlockDev = S_ISBLK(sb.st_mode);
> -
> - 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;
> - }
> - 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;
> - }
> - break;
> -
> - case O_RDWR:
> - default:
> - virReportSystemError(EINVAL,
> - _("Unable to process file with flags %d"),
> - (oflags & O_ACCMODE));
> - goto cleanup;
> - }
> -
> while (1) {
> ssize_t got;
>
> @@ -124,53 +97,123 @@ 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);
> - }
> -
> - if (got < 0) {
> - virReportSystemError(errno, _("Unable to read %s"), fdinname);
> - goto cleanup;
> + got = saferead(p.fdin, buf, buflen);
> }
> + if (got < 0)
> + return -2;
> if (got == 0)
> break;
>
> 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);
> - goto cleanup;
> + if (safewrite(p.fdout, buf, aligned_got) < 0) {
> + return -3;
> }
> -
> - if (!isBlockDev && ftruncate(fd, total) < 0) {
> - virReportSystemError(errno, _("Unable to truncate %s"), fdoutname);
> - goto cleanup;
> + if (!p.isBlockDev && ftruncate(p.fdout, total) < 0) {
> + return -4;
> }
> -
> break;
> }
> + }
> + return total;
> +}
>
> - if (safewrite(fdout, buf, got) < 0) {
> - virReportSystemError(errno, _("Unable to write %s"), fdoutname);
> +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"),
> + fd, path);
> + goto cleanup;
> + }
> + p.isBlockDev = S_ISBLK(sb.st_mode);
> + p.isDirect = O_DIRECT && (oflags & O_DIRECT);
> +
> + switch (oflags & O_ACCMODE) {
> + case O_RDONLY:
> + p.isWrite = false;
> + p.fdin = fd;
> + p.fdinname = path;
> + p.fdout = STDOUT_FILENO;
> + p.fdoutname = "stdout";
> + break;
> + case O_WRONLY:
> + p.isWrite = true;
> + p.fdin = STDIN_FILENO;
> + p.fdinname = "stdin";
> + p.fdout = fd;
> + p.fdoutname = path;
> + break;
> + case O_RDWR:
> + default:
> + virReportSystemError(EINVAL,
> + _("Unable to process file with flags %d"),
> + (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 end;
> + if (p.isWrite) {
> + if ((end = lseek(fd, 0, SEEK_END)) != 0) {
> + virReportSystemError(end < 0 ? errno : EINVAL, "%s",
> + _("O_DIRECT write needs empty seekable file"));
> + goto cleanup;
> + }
> + } else if ((end = lseek(fd, 0, SEEK_CUR)) != 0) {
> + virReportSystemError(end < 0 ? errno : EINVAL, "%s",
> + _("O_DIRECT read needs entire seekable file"));
> goto cleanup;
> }
> }
>
> + total = runIOCopy(p);
> +
> + if (total < 0) {
> + switch (total) {
> + case -1:
> + virReportSystemError(errno, _("Unable to move data from %s to %s"),
> + p.fdinname, p.fdoutname);
> + break;
> + case -2:
> + virReportSystemError(errno, _("Unable to read %s"), p.fdinname);
> + break;
> + case -3:
> + virReportSystemError(errno, _("Unable to write %s"), p.fdoutname);
> + break;
> + case -4:
> + virReportSystemError(errno, _("Unable to truncate %s"), p.fdoutname);
> + break;
> + default:
> + virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown runIOCopy error %ld"), total);
> + break;
> + }
> + 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;
> }
> }
>
