The iohelper currently calls saferead() to get data from the
underlying file. This has a problem with O_DIRECT when hitting
end-of-file. saferead() is asked to read 1MB, but the first
read() it does may return only a few KB, so it'll try another
read() to fill the remaining buffer. Unfortunately the buffer
pointer passed into this 2nd read() is likely not aligned
to the extent that O_DIRECT requires, so rather than seeing
'0' for end-of-file, we'll get -1 + EINVAL due to misaligned
buffer.

The way the iohelper is currently written, it already handles
getting short reads, so there is actually no need to use
saferead() at all. We can simply call read() directly. The
benefit of this is that we can now write() the data immediately
so when we go into the subsequent reads() we'll always have a
correctly aligned buffer.

Technically the file position ought to be aligned for O_DIRECT
too, but this does not appear to matter when at end-of-file.

Tested-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 src/util/iohelper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/util/iohelper.c b/src/util/iohelper.c
index fe15a92e6..5416d4506 100644
--- a/src/util/iohelper.c
+++ b/src/util/iohelper.c
@@ -109,7 +109,9 @@ runIO(const char *path, int fd, int oflags)
     while (1) {
         ssize_t got;
 
-        if ((got = saferead(fdin, buf, buflen)) < 0) {
+        if ((got = read(fdin, buf, buflen)) < 0) {
+            if (errno == EINTR)
+                continue;
             virReportSystemError(errno, _("Unable to read %s"), fdinname);
             goto cleanup;
         }
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 09/20/2017 10:31 AM, Daniel P. Berrange wrote:
> The iohelper currently calls saferead() to get data from the
> underlying file. This has a problem with O_DIRECT when hitting
> end-of-file. saferead() is asked to read 1MB, but the first
> read() it does may return only a few KB, so it'll try another
> read() to fill the remaining buffer. Unfortunately the buffer
> pointer passed into this 2nd read() is likely not aligned
> to the extent that O_DIRECT requires, so rather than seeing
> '0' for end-of-file, we'll get -1 + EINVAL due to misaligned
> buffer.
> 
> The way the iohelper is currently written, it already handles
> getting short reads, so there is actually no need to use
> saferead() at all. We can simply call read() directly. The
> benefit of this is that we can now write() the data immediately
> so when we go into the subsequent reads() we'll always have a
> correctly aligned buffer.
> 
> Technically the file position ought to be aligned for O_DIRECT
> too, but this does not appear to matter when at end-of-file.
> 
> Tested-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  src/util/iohelper.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list