Re: [libvirt] [PATCH 4/4] iohelper: fix reading with O_DIRECT
On 19.09.2017 12:36, Michal Privoznik wrote:
On 09/07/2017 09:44 AM, Nikolay Shirokovskiy wrote:
> saferead is not suitable for direct reads. If file size is not multiple
> of align size then we get EINVAL on the read(2) that is supposed to
> return 0 because read buffer will not be aligned at this point.
>
> Let's not read again after partial read and check that we read
> everything by comparing the number of total bytes read against file size.
> ---
> src/util/iohelper.c | 40 +++++++++++++++++++++++++++++++++++-----
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/iohelper.c b/src/util/iohelper.c
> index fe15a92..32c5a89 100644
> --- a/src/util/iohelper.c
> +++ b/src/util/iohelper.c
> @@ -78,10 +78,22 @@ runIO(const char *path, int fd, int oflags)
> fdoutname = "stdout";
> /* To make the implementation simpler, we give up on any
> * attempt to use O_DIRECT in a non-trivial manner. */
> - if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) {
> - virReportSystemError(end < 0 ? errno : EINVAL, "%s",
> - _("O_DIRECT read needs entire seekable
file"));
> - goto cleanup;
> + if (direct) {
> + if ((end = lseek(fd, 0, SEEK_CUR)) != 0) {
> + virReportSystemError(end < 0 ? errno : EINVAL, "%s",
> + _("O_DIRECT read needs entire seekable
file"));
> + goto cleanup;
> + }
> +
> + if ((end = lseek(fd, 0, SEEK_END)) < 0) {
> + virReportSystemError(errno, "%s", _("can not seek
file end"));
> + goto cleanup;
> + }
> +
> + if (lseek(fd, 0, SEEK_SET) < 0) {
> + virReportSystemError(errno, "%s", _("can not seek
file begin"));
> + goto cleanup;
> + }
This might change the position in the file. I mean, currently there is
no way that the @fd is not set at its beginning. However, I'd like to
have iohelper generic enough so that if a lseek()-ed fd was passed
iohelper reads from there and not reposition itself.
Looks like this will not be enough anyway. The first check makes sure we
are at the beginning. If we some day drop this check then we'll have
to write extra code too besides storing original position. Like taking
into account alignment issues.
Also, @end is rewritten here twice. It's confusing.
But as we are going to introduce cur variable why not?)
What we can do here is:
cur = lseek(fd, 0, SEEK_CUR);
end = lseek(fd, 0, SEEK_END);
lseek(fd, cur, SEEK_SET);
(with proper error handling obviously)
> }
> break;
> case O_WRONLY:
> @@ -109,7 +121,25 @@ runIO(const char *path, int fd, int oflags)
> while (1) {
> ssize_t got;
>
> - if ((got = saferead(fdin, buf, buflen)) < 0) {
> + /* in case of O_DIRECT we cannot read again to check for EOF
> + * after partial buffer read as it is done in saferead */
> + if (direct && fdin == fd && end - total < buflen) {
> + if (total == end)
> + break;
> +
> + while ((got = read(fd, buf, buflen)) < 0 && errno == EINTR);
In this case, the semicolon should be on a separate line:
while ()
;
> +
> + if (got < end - total) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to read last chunk from %s"),
> + fdinname);
> + goto cleanup;
> + }
> + } else {
> + got = saferead(fdin, buf, buflen);
> + }
> +
> + if (got < 0) {
> virReportSystemError(errno, _("Unable to read %s"),
fdinname);
> goto cleanup;
> }
>
Otherwise looking good. Also, ACK to patches 1-3. I'll merge them
shortly so that you can send v2 just for this one.
Ok.
Nikolay