Re: [PATCH 1/2 V2] kvm tools: Add scatter-gather variants of IO functions

[Konstantin Khlebnikov <khlebnikov@openvz.org>]

one note below

Sasha Levin wrote:
> Added scatter-gather variants of [x,xp][read,write]() and [p]read_in_full().
>
> Signed-off-by: Sasha Levin<levinsasha928@gmail.com>
> ---
>   tools/kvm/include/kvm/read-write.h |   13 +++
>   tools/kvm/read-write.c             |  172 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 185 insertions(+), 0 deletions(-)
>
> diff --git a/tools/kvm/include/kvm/read-write.h b/tools/kvm/include/kvm/read-write.h
> index 069326b..3351103 100644
> --- a/tools/kvm/include/kvm/read-write.h
> +++ b/tools/kvm/include/kvm/read-write.h
> @@ -2,6 +2,7 @@
>   #define KVM_READ_WRITE_H
>
>   #include<sys/types.h>
> +#include<sys/uio.h>
>   #include<unistd.h>
>
>   ssize_t xread(int fd, void *buf, size_t count);
> @@ -16,4 +17,16 @@ ssize_t xpwrite(int fd, const void *buf, size_t count, off_t offset);
>   ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset);
>   ssize_t pwrite_in_full(int fd, const void *buf, size_t count, off_t offset);
>
> +ssize_t xreadv(int fd, const struct iovec *iov, int iovcnt);
> +ssize_t xwritev(int fd, const struct iovec *iov, int iovcnt);
> +
> +ssize_t readv_in_full(int fd, const struct iovec *iov, int iovcnt);
> +ssize_t writev_in_full(int fd, const struct iovec *iov, int iovcnt);
> +
> +ssize_t xpreadv(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> +ssize_t xpwritev(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> +
> +ssize_t preadv_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> +ssize_t pwritev_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset);
> +
>   #endif /* KVM_READ_WRITE_H */
> diff --git a/tools/kvm/read-write.c b/tools/kvm/read-write.c
> index 398918b..449e8a2 100644
> --- a/tools/kvm/read-write.c
> +++ b/tools/kvm/read-write.c
> @@ -152,3 +152,175 @@ ssize_t pwrite_in_full(int fd, const void *buf, size_t count, off_t offset)
>
>   	return total;
>   }
> +
> +/* Same as readv(2) except that this function never returns EAGAIN or EINTR. */
> +ssize_t xreadv(int fd, const struct iovec *iov, int iovcnt)
> +{
> +	ssize_t nr;
> +
> +restart:
> +	nr = readv(fd, iov, iovcnt);
> +	if ((nr<  0)&&  ((errno == EAGAIN) || (errno == EINTR)))
> +		goto restart;
> +
> +	return nr;
> +}
> +
> +/* Same as writev(2) except that this function never returns EAGAIN or EINTR. */
> +ssize_t xwritev(int fd, const struct iovec *iov, int iovcnt)
> +{
> +	ssize_t nr;
> +
> +restart:
> +	nr = write(fd, iov, iovcnt);
> +	if ((nr<  0)&&  ((errno == EAGAIN) || (errno == EINTR)))
> +		goto restart;
> +
> +	return nr;
> +}
> +
> +static inline ssize_t get_iov_size(const struct iovec *iov, int iovcnt)
> +{
> +	size_t size = 0;
> +	while (iovcnt--)
> +		size += (iov++)->iov_len;
> +
> +	return size;
> +}
> +
> +ssize_t readv_in_full(int fd, const struct iovec *iov, int iovcnt)
> +{
> +	ssize_t total = 0;
> +	ssize_t count = get_iov_size(iov, iovcnt);
> +
> +	while (count>  0) {
> +		ssize_t nr;
> +
> +		nr = xreadv(fd, iov, iovcnt);
> +		if (nr<= 0) {
> +			if (total>  0)
> +				return total;
> +
> +			return -1;
> +		}
> +
> +		while ((size_t)nr>= iov->iov_len) {
> +			nr -= iov->iov_len;
> +			total += iov->iov_len;
> +			count -= iov->iov_len;
> +			iovcnt--;
> +			iov++;
> +		}
> +	}
> +
> +	return total;
> +}
> +
> +ssize_t writev_in_full(int fd, const struct iovec *iov, int iovcnt)
> +{
> +	ssize_t total = 0;
> +	size_t count = get_iov_size(iov, iovcnt);
> +
> +	while (count>  0) {
> +		ssize_t nr;
> +
> +		nr = xwritev(fd, iov, iovcnt);
> +		if (nr<  0)
> +			return -1;
> +		if (nr == 0) {
> +			errno = ENOSPC;
> +			return -1;
> +		}
> +
> +		while ((size_t)nr>= iov->iov_len) {
> +			nr -= iov->iov_len;
> +			total += iov->iov_len;
> +			count -= iov->iov_len;
> +			iovcnt--;
> +			iov++;
> +		}
> +	}
> +
> +	return total;
> +}
> +
> +/* Same as preadv(2) except that this function never returns EAGAIN or EINTR. */
> +ssize_t xpreadv(int fd, const struct iovec *iov, int iovcnt, off_t offset)
> +{
> +	ssize_t nr;
> +
> +restart:
> +	nr = preadv(fd, iov, iovcnt, offset);
> +	if ((nr<  0)&&  ((errno == EAGAIN) || (errno == EINTR)))
> +		goto restart;
> +
> +	return nr;
> +}
> +
> +/* Same as pwritev(2) except that this function never returns EAGAIN or EINTR. */
> +ssize_t xpwritev(int fd, const struct iovec *iov, int iovcnt, off_t offset)
> +{
> +	ssize_t nr;
> +
> +restart:
> +	nr = pwritev(fd, iov, iovcnt, offset);
> +	if ((nr<  0)&&  ((errno == EAGAIN) || (errno == EINTR)))
> +		goto restart;
> +
> +	return nr;
> +}
> +
> +ssize_t preadv_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset)
> +{
> +	ssize_t total = 0;
> +	size_t count = get_iov_size(iov, iovcnt);
> +
> +	while (count>  0) {
> +		ssize_t nr;
> +
> +		nr = xpreadv(fd, iov, iovcnt, offset);
> +		if (nr<= 0) {
> +			if (total>  0)
> +				return total;
> +
> +			return -1;
> +		}
> +
> +		while ((size_t)nr>= iov->iov_len) {
> +			nr -= iov->iov_len;
> +			total += iov->iov_len;
> +			count -= iov->iov_len;
> +			iovcnt--;
> +			iov++;
> +		}
> +	}
> +
> +	return total;
> +}
> +
> +ssize_t pwritev_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset)
> +{
> +	ssize_t total = 0;
> +	size_t count = get_iov_size(iov, iovcnt);
> +
> +	while (count>  0) {
> +		ssize_t nr;
> +
> +		nr = xpwritev(fd, iov, iovcnt, offset);
> +		if (nr<  0)
> +			return -1;
> +		if (nr == 0) {
> +			errno = ENOSPC;
> +			return -1;
> +		}
> +		while ((size_t)nr>= iov->iov_len) {
> +			nr -= iov->iov_len;
> +			total += iov->iov_len;
> +			count -= iov->iov_len;
> +			iovcnt--;
> +			iov++;

You forget here to change offset.

> +		}
> +	}
> +
> +	return total;
> +}


It might be better to add an helper function, something like this:

static inline void shift_iovec(const struct iovec **iov, int *iovcnt,
			       size_t nr, ssize_t *total, off_t *offset)
{
	while (nr >= (*iov)->iov_len) {
		nr -= (*iov)->iov_len;
		*total += (*iov)->iov_len;
		*offset += (*iov)->iov_len;
		*iovcnt--;
		*iov++;
	}
}

ssize_t pwritev_in_full(int fd, const struct iovec *iov, int iovcnt, off_t offset)
{
	ssize_t nr, total = 0;

	while (iovcnt) {
		nr = xpwritev(fd, iov, iovcnt, offset);
		if (nr < 0)
			return -1;
		if (nr == 0) {
			errno = ENOSPC;
			return -1;
		}
		shift_iovec(&iov, &iovcnt, nr, &total, &offset);
	}

	return total;
}