From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pekka Enberg Subject: Re: [PATCH 2/2] kvm tools: Add scatter-gather support for disk images Date: Sat, 16 Apr 2011 16:55:05 +0300 (EEST) Message-ID: References: <1302954344-11700-1-git-send-email-levinsasha928@gmail.com> <1302954344-11700-2-git-send-email-levinsasha928@gmail.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: mingo@elte.hu, asias.hejun@gmail.com, gorcunov@gmail.com, kvm@vger.kernel.org To: Sasha Levin Return-path: Received: from filtteri6.pp.htv.fi ([213.243.153.189]:50670 "EHLO filtteri6.pp.htv.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751110Ab1DPNzI (ORCPT ); Sat, 16 Apr 2011 09:55:08 -0400 In-Reply-To: <1302954344-11700-2-git-send-email-levinsasha928@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > >