[Qemu-devel] [RFC 0/2] osdep: warn if opening a file O_DIRECT on tmpfs fails

[Qemu-devel] [RFC 0/2] osdep: warn if opening a file O_DIRECT on tmpfs fails

[Stefan Hajnoczi]

This is an implementation of Dan and Eric's idea for probing a failed O_DIRECT
open(2) call to see if the file system does not support O_DIRECT.

I wanted to see what the implementation looks like but I don't like it:

1. We still need to guess if O_DIRECT is supported in the O_CREAT EINVAL case
   because we can't probe if O_CREAT | O_DIRECT | O_EXCL returned EINVAL.
2. There is a race condition between open(O_CREAT | O_EXCL | O_DIRECT) and
   opening again without O_CREAT.  If the file is deleted we'll get ENOENT
   which would have been impossible before.
3. It's way complicated.

Issue #1 gives me an idea: why play games when we can simply warn the user?

if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
    error_report("file system may not support O_DIRECT");
    errno = EINVAL; /* in case it was clobbered */
}

I think this simple, portable approach beats statfs tmpfs and open probing.
Will send a patch for that and plan to merge it.

Stefan Hajnoczi (2):
  libcacard: link against qemu-error.o for error_report()
  osdep: warn if opening a file O_DIRECT on tmpfs fails

 libcacard/Makefile |  3 ++-
 util/osdep.c       | 62 ++++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 57 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC 1/2] libcacard: link against qemu-error.o for error_report()

[Stefan Hajnoczi]

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 libcacard/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libcacard/Makefile b/libcacard/Makefile
index 47827a0..4d15da4 100644
--- a/libcacard/Makefile
+++ b/libcacard/Makefile
@@ -4,7 +4,8 @@ TOOLS += vscclient$(EXESUF)
 
 # objects linked into a shared library, built with libtool with -fPIC if required
 libcacard-obj-y = $(stub-obj-y) $(libcacard-y)
-libcacard-obj-y += util/osdep.o util/cutils.o util/qemu-timer-common.o util/error.o
+libcacard-obj-y += util/osdep.o util/cutils.o util/qemu-timer-common.o
+libcacard-obj-y += util/error.o util/qemu-error.o
 libcacard-obj-$(CONFIG_WIN32) += util/oslib-win32.o util/qemu-thread-win32.o
 libcacard-obj-$(CONFIG_POSIX) += util/oslib-posix.o util/qemu-thread-posix.o
 libcacard-obj-y += $(filter trace/%, $(util-obj-y))
-- 
1.8.3.1

[Qemu-devel] [RFC 2/2] osdep: warn if opening a file O_DIRECT on tmpfs fails

[Stefan Hajnoczi]

Print a warning when opening a file O_DIRECT on tmpfs fails with EINVAL.
This saves users a lot of time trying to figure out the EINVAL error.

Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
Suggested-by: Eric Blake <eblake@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/osdep.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index 685c8ae..cbee2b7 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -148,6 +148,23 @@ static int qemu_parse_fdset(const char *param)
 #endif
 
 /*
+ * Open a file with O_CLOEXEC semantics
+ */
+static int open_cloexec(const char *name, int flags, int mode)
+{
+    int ret;
+#ifdef O_CLOEXEC
+    ret = open(name, flags | O_CLOEXEC, mode);
+#else
+    ret = open(name, flags, mode);
+    if (ret >= 0) {
+        qemu_set_cloexec(ret);
+    }
+#endif
+    return ret;
+}
+
+/*
  * Opens a file with FD_CLOEXEC set
  */
 int qemu_open(const char *name, int flags, ...)
@@ -198,14 +215,45 @@ int qemu_open(const char *name, int flags, ...)
         va_end(ap);
     }
 
-#ifdef O_CLOEXEC
-    ret = open(name, flags | O_CLOEXEC, mode);
-#else
-    ret = open(name, flags, mode);
-    if (ret >= 0) {
-        qemu_set_cloexec(ret);
+    /* Some file systems do not support O_DIRECT and fail with EINVAL.  Confirm
+     * the problem by trying again without O_DIRECT and printing a warning.
+     *
+     * Sounds easy enough but we need to be careful with O_CREAT since this
+     * function should not create a file when an error is returned.
+     */
+    if (flags & (O_CREAT | O_DIRECT) == O_CREAT | O_DIRECT) {
+        ret = open_cloexec(name, flags | O_EXCL, mode);
+
+        if (ret >= 0) {
+            return ret;
+        }
+
+        if (errno == EINVAL) {
+            error_report("file system may not support O_DIRECT");
+            errno = EINVAL; /* in case it was clobbered */
+            return ret;
+        } else if (errno == EEXIST && (flags & O_EXCL) == 0) {
+            /* We know the file existed, drop O_CREAT so the following open
+             * attempts do not create a file if O_DIRECT produces EINVAL.  Note
+             * there is a race condition here if the file is deleted while we
+             * perform our open calls.
+             */
+            flags &= O_CREAT;
+        } else {
+            return ret;
+        }
+    }
+
+    ret = open_cloexec(name, flags, mode);
+
+    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
+        int fd = open_cloexec(name, flags & ~O_DIRECT, mode);
+        if (fd >= 0) {
+            close(fd);
+            error_report("file system does not support O_DIRECT");
+        }
+        errno = EINVAL; /* in case it was clobbered */
     }
-#endif
 
     return ret;
 }
-- 
1.8.3.1