Re: [libvirt] [PATCH v2] iohelper: fix reading with O_DIRECT
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