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

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

[Stefan Hajnoczi]

v2:
 * Link in util/qemu-error.o instead of stubbing error_report() [Markus]

Deepak C Shetty <deepakcs@linux.vnet.ibm.com> tried to open an image file
O_DIRECT on tmpfs and received EINVAL.  This error is too generic, making it
hard to figure out that the problem is -drive ...,cache=none on tmpfs.

Add a warning when opening a file O_DIRECT on tmpfs fails.

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       | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 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] [PATCH v2 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.  This saves
users a lot of time trying to figure out the EINVAL error.

Daniel P. Berrange <berrange@redhat.com> suggested opening the file
without O_DIRECT as a portable way to check whether the file system
supports O_DIRECT.  That gets messy when flags contains O_CREAT since
we'd create a file but return an error - or a race condition if we try
to unlink the file.  It's simpler to check the file system type.

Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/osdep.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/util/osdep.c b/util/osdep.c
index 685c8ae..446a1dc 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -30,6 +30,11 @@
 #include <unistd.h>
 #include <fcntl.h>
 
+#ifdef __linux__
+#include <sys/vfs.h>
+#include <linux/magic.h>
+#endif
+
 /* Needed early for CONFIG_BSD etc. */
 #include "config-host.h"
 
@@ -207,6 +212,20 @@ int qemu_open(const char *name, int flags, ...)
     }
 #endif
 
+#ifdef __linux__
+    /* It is not possible to open files O_DIRECT on tmpfs.  Provide a hint that
+     * this may be the case (of course it could change in future kernel
+     * versions).
+     */
+    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
+        struct statfs st;
+        if (statfs(name, &st) == 0 && st.f_type == TMPFS_MAGIC) {
+            error_report("tmpfs file systems may not support O_DIRECT");
+        }
+        errno = EINVAL; /* in case it was clobbered */
+    }
+#endif /* __linux__ */
+
     return ret;
 }
 
-- 
1.8.3.1

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> Print a warning when opening a file O_DIRECT on tmpfs fails.  This saves

Only when it fails with EINVAL, actually.  Suggest "on tmpfs fails with
EINVAL."

> users a lot of time trying to figure out the EINVAL error.
>
> Daniel P. Berrange <berrange@redhat.com> suggested opening the file
> without O_DIRECT as a portable way to check whether the file system
> supports O_DIRECT.  That gets messy when flags contains O_CREAT since
> we'd create a file but return an error - or a race condition if we try
> to unlink the file.  It's simpler to check the file system type.
>
> Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/osdep.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index 685c8ae..446a1dc 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -30,6 +30,11 @@
>  #include <unistd.h>
>  #include <fcntl.h>
>  
> +#ifdef __linux__
> +#include <sys/vfs.h>
> +#include <linux/magic.h>
> +#endif
> +
>  /* Needed early for CONFIG_BSD etc. */
>  #include "config-host.h"
>  
> @@ -207,6 +212,20 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  #endif
>  
> +#ifdef __linux__
> +    /* It is not possible to open files O_DIRECT on tmpfs.  Provide a hint that
> +     * this may be the case (of course it could change in future kernel
> +     * versions).
> +     */
> +    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> +        struct statfs st;
> +        if (statfs(name, &st) == 0 && st.f_type == TMPFS_MAGIC) {
> +            error_report("tmpfs file systems may not support O_DIRECT");
> +        }
> +        errno = EINVAL; /* in case it was clobbered */
> +    }
> +#endif /* __linux__ */
> +
>      return ret;
>  }

In theory, the warning could be misleading, because we can get EINVAL
for reasons not related to flags & O_DIRECT.  In practice, the warning
probably does much more good than harm.

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

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> v2:
>  * Link in util/qemu-error.o instead of stubbing error_report() [Markus]
>
> Deepak C Shetty <deepakcs@linux.vnet.ibm.com> tried to open an image file
> O_DIRECT on tmpfs and received EINVAL.  This error is too generic, making it
> hard to figure out that the problem is -drive ...,cache=none on tmpfs.
>
> Add a warning when opening a file O_DIRECT on tmpfs fails.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 913 bytes --]

On 08/21/2013 07:16 AM, Stefan Hajnoczi wrote:
> Print a warning when opening a file O_DIRECT on tmpfs fails.  This saves
> users a lot of time trying to figure out the EINVAL error.
> 
> Daniel P. Berrange <berrange@redhat.com> suggested opening the file
> without O_DIRECT as a portable way to check whether the file system
> supports O_DIRECT.  That gets messy when flags contains O_CREAT since
> we'd create a file but return an error - or a race condition if we try
> to unlink the file.  It's simpler to check the file system type.
> 
> Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/osdep.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On 08/21/2013 07:16 AM, Stefan Hajnoczi wrote:
> Print a warning when opening a file O_DIRECT on tmpfs fails.  This saves
> users a lot of time trying to figure out the EINVAL error.
> 
> Daniel P. Berrange <berrange@redhat.com> suggested opening the file
> without O_DIRECT as a portable way to check whether the file system
> supports O_DIRECT.  That gets messy when flags contains O_CREAT since
> we'd create a file but return an error - or a race condition if we try
> to unlink the file.  It's simpler to check the file system type.

An alternative to this is as follows:

if the user passes in O_CREAT|O_DIRECT (but not O_EXCL), we _first_ try
O_CREAT|O_DIRECT|O_EXCL.  If that succeeds, O_DIRECT works and we
created the file, nothing further to do.  If it fails with EINVAL,
O_DIRECT is unsupported.  If it fails with EEXIST, the file already
exists, and we retry open(O_DIRECT) (no O_CREAT, because the file
already exists), and get our answer that way.  (It would be possible to
audit whether the kernel favors EINVAL vs. EEXIST in the case where both
errors are possible [ie. the file exists and O_DIRECT is unsupported],
to slightly optimize this code; but I'm not sure it's worth it).  If the
user passes in O_DIRECT but not O_CREAT, then you jump straight to the
middle case (since they didn't want creation in the first place).

That way, you can detect O_DIRECT failure on more than just tmpfs, and
without needing an #if __linux__, and with no nasty races in unlinking a
just-created file.  I'm starting to think Dan's suggestion has merit
after all, if done correctly.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[Doug Goldstein]

[-- Attachment #1: Type: text/plain, Size: 2740 bytes --]

On Wed, Aug 21, 2013 at 9:08 AM, Markus Armbruster <armbru@redhat.com>wrote:

> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > Print a warning when opening a file O_DIRECT on tmpfs fails.  This saves
>
> Only when it fails with EINVAL, actually.  Suggest "on tmpfs fails with
> EINVAL."
>
> > users a lot of time trying to figure out the EINVAL error.
> >
> > Daniel P. Berrange <berrange@redhat.com> suggested opening the file
> > without O_DIRECT as a portable way to check whether the file system
> > supports O_DIRECT.  That gets messy when flags contains O_CREAT since
> > we'd create a file but return an error - or a race condition if we try
> > to unlink the file.  It's simpler to check the file system type.
> >
> > Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  util/osdep.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 685c8ae..446a1dc 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -30,6 +30,11 @@
> >  #include <unistd.h>
> >  #include <fcntl.h>
> >
> > +#ifdef __linux__
> > +#include <sys/vfs.h>
> > +#include <linux/magic.h>
> > +#endif
> > +
> >  /* Needed early for CONFIG_BSD etc. */
> >  #include "config-host.h"
> >
> > @@ -207,6 +212,20 @@ int qemu_open(const char *name, int flags, ...)
> >      }
> >  #endif
> >
> > +#ifdef __linux__
> > +    /* It is not possible to open files O_DIRECT on tmpfs.  Provide a
> hint that
> > +     * this may be the case (of course it could change in future kernel
> > +     * versions).
> > +     */
> > +    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> > +        struct statfs st;
> > +        if (statfs(name, &st) == 0 && st.f_type == TMPFS_MAGIC) {
> > +            error_report("tmpfs file systems may not support O_DIRECT");
> > +        }
> > +        errno = EINVAL; /* in case it was clobbered */
> > +    }
> > +#endif /* __linux__ */
> > +
> >      return ret;
> >  }
>
> In theory, the warning could be misleading, because we can get EINVAL
> for reasons not related to flags & O_DIRECT.  In practice, the warning
> probably does much more good than harm.
>
>
Actually EINVAL is only for O_DIRECT with open(). There are other
filesystems that will return this other than tmpfs as well, but we were
discussing /var/run so the assumption was tmpfs [1]. The code that performs
the check to see if EINVAL should be returned checks to see if the
direct_IO address space op is defined [2] (or get_xip_mem but that's
another story).

[1] https://www.redhat.com/archives/libvir-list/2013-August/msg00768.html
[2] http://lxr.linux.no/#linux+v3.10.9/fs/open.c#L651

-- 
Doug Goldstein

[-- Attachment #2: Type: text/html, Size: 4123 bytes --]