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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list