[Qemu-devel] [PATCH 0/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Qemu-devel] [PATCH 0/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Stefan Hajnoczi]

After trying out a few approaches, here is what I think is the simplest viable
way to print a user-friendly warning if opening a file O_DIRECT fails with
EINVAL.  This happens on Linux tmpfs.

We don't really know why we got EINVAL but if O_DIRECT was used it's a good
clue that the file system does not support O_DIRECT.

Stefan Hajnoczi (2):
  libcacard: link against qemu-error.o for error_report()
  osdep: warn if open(O_DIRECT) on fails with EINVAL

 libcacard/Makefile | 3 ++-
 util/osdep.c       | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 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 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Stefan Hajnoczi]

Print a warning when opening a file O_DIRECT fails with EINVAL.  This
saves users a lot of time trying to figure out the EINVAL error, which
is typical when attempting to open a file O_DIRECT on Linux tmpfs.

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

diff --git a/util/osdep.c b/util/osdep.c
index 685c8ae..62072b4 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -207,6 +207,13 @@ int qemu_open(const char *name, int flags, ...)
     }
 #endif
 
+#ifdef O_DIRECT
+    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
+        error_report("file system may not support O_DIRECT");
+        errno = EINVAL; /* in case it was clobbered */
+    }
+#endif /* O_DIRECT */
+
     return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Eric Blake]

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

On 08/22/2013 03:29 AM, Stefan Hajnoczi wrote:
> Print a warning when opening a file O_DIRECT fails with EINVAL.  This
> saves users a lot of time trying to figure out the EINVAL error, which
> is typical when attempting to open a file O_DIRECT on Linux tmpfs.
> 
> Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/osdep.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 685c8ae..62072b4 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -207,6 +207,13 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  #endif
>  
> +#ifdef O_DIRECT

On some other projects I've worked with (hello gnulib!), the
compatibility headers do:

#define O_DIRECT 0

so that the rest of the code can just blindly use open(...,|O_DIRECT)
(provided, of course, that not having O_DIRECT semantics is not
fatal...).  If that is done, then this #ifdef will always be true...

> +    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {

...but the '(flags & O_DIRECT)' subclause would be false, so that's safe.

If O_DIRECT were always defined, then '#if O_DIRECT', or better yet, 'if
(O_DIRECT)', is a better way to conditionalize code that depends on it
being a useful value (no false positives when defined to 0, and by
moving the check from preprocessor to C, you get benefits of compiler
verification of type safety to avoid bitrot in what is otherwise dead code).

Grepping through qemu, I see that block/raw-posix.c has some weird
#ifdef'ery:

/* OS X does not have O_DSYNC */
#ifndef O_DSYNC
#ifdef O_SYNC
#define O_DSYNC O_SYNC
#elif defined(O_FSYNC)
#define O_DSYNC O_FSYNC
#endif
#endif

/* Approximate O_DIRECT with O_DSYNC if O_DIRECT isn't available */
#ifndef O_DIRECT
#define O_DIRECT O_DSYNC
#endif

which strikes me as odd (O_DSYNC and O_DIRECT have orthogonal, and
somewhat opposing goals - the former says don't return from write()
until data is on disk, which slows things down for safety; the latter
says "I'm a special user space, get out of my way, by not caching
anything, and leaving the flushing to me", to speed things up if the
user knows what they are doing - then again, the Linux man page hints
that they are often used together when worrying about guarantees for
synchronous I/O).

But enough of that side diversion - that one #define of O_DIRECT is not
related to the file you are touching.  I like the elegance of your patch
(nicer than the race in my double-open attempt).

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 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Alex Bligh]

--On 22 August 2013 11:57:56 -0600 Eric Blake <eblake@redhat.com> wrote:

># define O_DIRECT 0
>
> so that the rest of the code can just blindly use open(...,|O_DIRECT)
> (provided, of course, that not having O_DIRECT semantics is not
> fatal...).  If that is done, then this #ifdef will always be true...

I think this is undesirable as the result of opening without O_DIRECT
when you really wanted O_DIRECT could be subtle data corruption due
to unexpected caching. Is an error not more appropriate here than
proceeding regardless?

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Eric Blake]

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

On 08/22/2013 01:31 PM, Alex Bligh wrote:
> 
> 
> --On 22 August 2013 11:57:56 -0600 Eric Blake <eblake@redhat.com> wrote:
> 
>> # define O_DIRECT 0
>>
>> so that the rest of the code can just blindly use open(...,|O_DIRECT)
>> (provided, of course, that not having O_DIRECT semantics is not
>> fatal...).  If that is done, then this #ifdef will always be true...
> 
> I think this is undesirable as the result of opening without O_DIRECT
> when you really wanted O_DIRECT could be subtle data corruption due
> to unexpected caching. Is an error not more appropriate here than
> proceeding regardless?

As I said in the surrounding text you snipped:

> 
> On some other projects I've worked with (hello gnulib!), the
> compatibility headers do:
...
> 
> But enough of that side diversion - that one #define of O_DIRECT is not
> related to the file you are touching. 

Qemu doesn't define O_DIRECT to 0 for the mere sake of compilation, and
I'm not arguing that it should.  I was more making sure that this patch
was correct, by reassuring myself the policies that qemu uses for
O_DIRECT (and since I demonstrated that other projects have other
policies).  As to whether other projects may have a bug when using
O_DIRECT being 0, it is a problem for those projects to deal with.
Besides, O_DIRECT is a non-POSIX extension, so anyone using it already
has non-portability issues to think about, regardless of whether the
fallback for platforms that lack it is done by always defining to 0 or
always using #ifdef guards.

Thus, my diversion about other projects using O_DIRECT as 0 is a non-issue.

-- 
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 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Jeff Cody]

On Thu, Aug 22, 2013 at 11:29:03AM +0200, Stefan Hajnoczi wrote:
> Print a warning when opening a file O_DIRECT fails with EINVAL.  This
> saves users a lot of time trying to figure out the EINVAL error, which
> is typical when attempting to open a file O_DIRECT on Linux tmpfs.
> 
> Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/osdep.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/util/osdep.c b/util/osdep.c
> index 685c8ae..62072b4 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -207,6 +207,13 @@ int qemu_open(const char *name, int flags, ...)
>      }
>  #endif
>  
> +#ifdef O_DIRECT
> +    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> +        error_report("file system may not support O_DIRECT");
> +        errno = EINVAL; /* in case it was clobbered */
> +    }
> +#endif /* O_DIRECT */
> +
>      return ret;
>  }
>  
> -- 
> 1.8.3.1
> 
> 

What about putting something similar in error_setg_file_open(), in
util/error.c?  There are other occasions when O_DIRECT causes a file
open to fail (e.g. live snapshots with 'cache=none' to a tmpfs
filesystem).  That would give additional info then for QMP commands.
Of course, it would then be necessary to also pass the open flags (or
other equivalent info) to error_setg_file_open().

Re: [Qemu-devel] [PATCH 2/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Stefan Hajnoczi]

On Fri, Sep 06, 2013 at 04:32:43PM -0400, Jeff Cody wrote:
> On Thu, Aug 22, 2013 at 11:29:03AM +0200, Stefan Hajnoczi wrote:
> > Print a warning when opening a file O_DIRECT fails with EINVAL.  This
> > saves users a lot of time trying to figure out the EINVAL error, which
> > is typical when attempting to open a file O_DIRECT on Linux tmpfs.
> > 
> > Reported-by: Deepak C Shetty <deepakcs@linux.vnet.ibm.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  util/osdep.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 685c8ae..62072b4 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -207,6 +207,13 @@ int qemu_open(const char *name, int flags, ...)
> >      }
> >  #endif
> >  
> > +#ifdef O_DIRECT
> > +    if (ret == -1 && errno == EINVAL && (flags & O_DIRECT)) {
> > +        error_report("file system may not support O_DIRECT");
> > +        errno = EINVAL; /* in case it was clobbered */
> > +    }
> > +#endif /* O_DIRECT */
> > +
> >      return ret;
> >  }
> >  
> > -- 
> > 1.8.3.1
> > 
> > 
> 
> What about putting something similar in error_setg_file_open(), in
> util/error.c?  There are other occasions when O_DIRECT causes a file
> open to fail (e.g. live snapshots with 'cache=none' to a tmpfs
> filesystem).  That would give additional info then for QMP commands.
> Of course, it would then be necessary to also pass the open flags (or
> other equivalent info) to error_setg_file_open().

It would be nice to cover all file open cases.  But I think
error_setg_file_open() is a bit beyond the scope here.

error_setg_file_open() is not used in cases where O_DIRECT is relevant.
I checked all callers and they don't seem to allow O_DIRECT.  Some of
them even use qemu_fopen().

Stefan

Re: [Qemu-devel] [PATCH 0/2] osdep: warn if open(O_DIRECT) on fails with EINVAL

[Stefan Hajnoczi]

On Thu, Aug 22, 2013 at 11:29:01AM +0200, Stefan Hajnoczi wrote:
> After trying out a few approaches, here is what I think is the simplest viable
> way to print a user-friendly warning if opening a file O_DIRECT fails with
> EINVAL.  This happens on Linux tmpfs.
> 
> We don't really know why we got EINVAL but if O_DIRECT was used it's a good
> clue that the file system does not support O_DIRECT.
> 
> Stefan Hajnoczi (2):
>   libcacard: link against qemu-error.o for error_report()
>   osdep: warn if open(O_DIRECT) on fails with EINVAL
> 
>  libcacard/Makefile | 3 ++-
>  util/osdep.c       | 7 +++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)

Applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan