[libvirt] [PATCH v2] iohelper: fix reading with O_DIRECT

[Nikolay Shirokovskiy]

On 20.09.2017 18:05, Daniel P. Berrange wrote:
> On Wed, Sep 20, 2017 at 06:00:59PM +0300, Nikolay Shirokovskiy wrote:
>>
>>
>> On 20.09.2017 17:48, Daniel P. Berrange wrote:
>>>
>>> IOW can simplify your patch to just this I believe:> 
>>> 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;
>>>          }
>>
>> But we still read half the buffer at the file end and then try misaligned read
>> after that.
> 
> No, we shouldn't be mis-aligned. The 'buf' variable we use is aligned
> correctly.  If we read half a buffer, we'll then write that data back
> to libvirtd. So when we go back into read(), we'll be reading into the
> start of 'buf' again, so we're still correctly aligned. This is the
> key difference against saferead() - that tries to fill up the buffer
> before we can write any data, whereas with this patch we'll always
> write whatever we read immediately
> 

I've tested this patch and this works. From read(2)

       EINVAL fd is attached to an object which is unsuitable for reading; or the file was opened with the O_DIRECT
              flag, and either the address specified in buf, the value specified in count, or the current file off‐
              set is not suitably aligned.

I thought everything should be aligned. But after giving the above sentence a thought - it is not stated )
So in case of EOF at least position may not be aligned.

But in open(2):

       The O_DIRECT flag may impose alignment restrictions on the length and address of user-space buffers and  the
       file  offset  of  I/Os.  In Linux alignment restrictions vary by file system and kernel version and might be
       absent entirely.  However there is currently no file system-independent interface for an application to dis‐
       cover  these  restrictions  for a given file or file system.  Some file systems provide their own interfaces
       for doing so, for example the XFS_IOC_DIOINFO operation in xfsctl(3).

So finally it is not clear should such EOF read be possible or not. But I vote for the simple approach while it works.

Nikolay