Re: [libvirt PATCH] iohelper: some refactoring

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
>          }
>      }
> 




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux