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

[Pekka Enberg]

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


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html