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

Re: [libvirt] [PATCH 1/2] util: Generalize virFileDirectFd



On 02/06/2012 07:51 AM, Jiri Denemark wrote:
> 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(-)
> 

Everything you've got is a mechanical rename, so I have no objection to
th is as-is.  But if we ever get better kernel support for
posix_fadvise, we have a problem - use of posix_fadvise won't magically
make an fd report EAGAIN.  I think it might be worth a v2 with some
slight tweaks by adding a second bool parameter to virFileWrapperFdNew.

> @@ -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)

Here, call virFileWrapperFdNew(&fd, true, true, path)

and in patch 2/2, it would change to:

wrapperFd = virFileWrapperFdNew(&fd, bypass_cache, true, path)

> @@ -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)

Likewise (oops, you missed this one in 2/2).

>  /**
> - * virFileDirectFdNew:
> + * virFileWrapperFdNew:
>   * @fd: pointer to fd to wrap
> + * @bypassCache: bypass system cache

Here, I'd add:

* @nonBlock: ensure non-blocking I/O

>   * @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.

and here, I'd clarify what "In some cases" means:

If @nonBlock is true, or in some cases when @bypassCache is true, @fd is
changed to a non-seekable pipe; ...

> +virFileWrapperFdPtr
> +virFileWrapperFdNew(int *fd, bool bypassCache, const char *name)

add the bool nonBlock parameter here,

>  {
> -    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.  */

This is why I'm suggesting the fourth parameter.  If we ever get a fixed
kernel, then it is conceivable to want bypassCache but not nonBlock
(maybe not for qemu, but for other uses).  Right now, use of iohelper
happens to have a side-effect of conversion to a pipe, and thus
conversion to non-blocking, but if we find a way to avoid iohelper while
still implementing cache bypass, then we'd need a second parameter
stating whether we also need the iohelper effect of non-blocking I/O.

>  
> -    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"));

Don't forget to tweak this signature, too.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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