Re: [PATCH 2/2] kvm tools: Add scatter-gather support for disk images

[Pekka Enberg <penberg@kernel.org>]

On Sat, 16 Apr 2011, Sasha Levin wrote:
> Add optional support for scatter-gather to disk_image.
> Formats that can't take advantage of scatter-gather fallback to simple IO.

Cool!

> +static size_t raw_image__read_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
> +{
> +	uint64_t offset = sector << SECTOR_SHIFT;
> +	size_t ret = -1;
> +
> +	ret = preadv(self->fd, iov, iovcount, offset);
> +
> +	return ret;

Please just do

   return preadv(...);

Also, I guess you need preadv_in_full() similar to read_in_full() that 
handles EINTR and EAGAIN.

> +}
> +
> +static size_t raw_image__write_sector_sg(struct disk_image *self, uint64_t sector, const struct iovec *iov, int iovcount)
> +{
> +	uint64_t offset = sector << SECTOR_SHIFT;
> +	size_t ret = -1;
> +
> +	ret = pwritev(self->fd, iov, iovcount, offset);
> +
> +	return ret;

Same comments apply here as well.

> @@ -115,50 +115,23 @@ static bool virtio_blk_do_io_request(struct kvm *self, struct virt_queue *queue)
> {
> 	struct iovec iov[VIRTIO_BLK_QUEUE_SIZE];
> 	struct virtio_blk_outhdr *req;
> -	uint32_t block_len, block_cnt;
> +	size_t block_cnt = -1;
> 	uint16_t out, in, head;
> 	uint8_t *status;
> -	bool io_error;
> -	void *block;
> -	int err, i;
> -
> -	io_error	= false;
>
> 	head		= virt_queue__get_iov(queue, iov, &out, &in, self);
>
> 	/* head */
> 	req		= iov[0].iov_base;
>
> -	/* block */
> -	block_cnt	= 0;
> -
> -	for (i = 1; i < out + in - 1; i++) {
> -		block		= iov[i].iov_base;
> -		block_len	= iov[i].iov_len;
> -
> -		switch (req->type) {
> -		case VIRTIO_BLK_T_IN:
> -			err	= disk_image__read_sector(self->disk_image, req->sector, block, block_len);
> -			if (err)
> -				io_error = true;
> -			break;
> -		case VIRTIO_BLK_T_OUT:
> -			err	= disk_image__write_sector(self->disk_image, req->sector, block, block_len);
> -			if (err)
> -				io_error = true;
> -			break;
> -		default:
> -			warning("request type %d", req->type);
> -			io_error	= true;
> -		}
> -
> -		req->sector	+= block_len >> SECTOR_SHIFT;
> -		block_cnt	+= block_len;
> -	}
> +	if (req->type == VIRTIO_BLK_T_IN)
> +		block_cnt = disk_image__read_sector_sg(self->disk_image, req->sector, iov + 1, in + out - 2);
> +	else if (req->type == VIRTIO_BLK_T_OUT)
> +		block_cnt = disk_image__write_sector_sg(self->disk_image, req->sector, iov + 1, in + out - 2);

Please us switch statement like in the above loop instead of if-else and 
retain the warning.

>
> 	/* status */
> 	status			= iov[out + in - 1].iov_base;
> -	*status			= io_error ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
> +	*status			= (block_cnt == (size_t)-1) ? VIRTIO_BLK_S_IOERR : VIRTIO_BLK_S_OK;
>
> 	virt_queue__set_used_elem(queue, head, block_cnt);
>
> -- 
> 1.7.5.rc1
>
>