Re: [Qemu-devel] [PATCH v2 8/8] virtio-blk: Fill in VirtIOBlockReq.out in dataplane code

[Fam Zheng <famz@redhat.com>]

On Thu, 06/05 04:55, Paolo Bonzini wrote:
> Il 05/06/2014 03:58, Fam Zheng ha scritto:
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> > hw/block/dataplane/virtio-blk.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> >diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> >index 96068cd..323793a 100644
> >--- a/hw/block/dataplane/virtio-blk.c
> >+++ b/hw/block/dataplane/virtio-blk.c
> >@@ -113,7 +113,8 @@ static void do_get_id_cmd(VirtIOBlockDataPlane *s,
> > static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
> >                         struct iovec *iov, unsigned iov_cnt,
> >                         int64_t sector_num, VirtQueueElement *elem,
> >-                        struct virtio_blk_inhdr *inhdr)
> >+                        struct virtio_blk_inhdr *inhdr,
> >+                        struct virtio_blk_outhdr *outhdr)
> > {
> >     VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
> >     VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
> >@@ -124,6 +125,7 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
> >     req->elem = elem;
> >     req->dev = dev;
> >     req->in = inhdr;
> >+    req->out = *outhdr;
> >     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
> >
> >     qiov = &req->qiov;
> >@@ -155,13 +157,15 @@ static void complete_flush(void *opaque, int ret)
> > }
> >
> > static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
> >-                         struct virtio_blk_inhdr *inhdr)
> >+                         struct virtio_blk_inhdr *inhdr,
> >+                         struct virtio_blk_outhdr *outhdr)
> > {
> >     VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
> >     VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
> >     req->dev = dev;
> >     req->elem = elem;
> >     req->in = inhdr;
> >+    req->out = *outhdr;
> >
> >     bdrv_aio_flush(s->blk->conf.bs, complete_flush, req);
> > }
> >@@ -202,13 +206,13 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
> >     case VIRTIO_BLK_T_IN:
> >         do_rdwr_cmd(s, true, in_iov, in_num,
> >                     outhdr.sector * 512 / BDRV_SECTOR_SIZE,
> >-                    elem, inhdr);
> >+                    elem, inhdr, &outhdr);
> >         return 0;
> >
> >     case VIRTIO_BLK_T_OUT:
> >         do_rdwr_cmd(s, false, iov, out_num,
> >                     outhdr.sector * 512 / BDRV_SECTOR_SIZE,
> >-                    elem, inhdr);
> >+                    elem, inhdr, &outhdr);
> >         return 0;
> >
> >     case VIRTIO_BLK_T_SCSI_CMD:
> >@@ -216,7 +220,7 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
> >         return 0;
> >
> >     case VIRTIO_BLK_T_FLUSH:
> >-        do_flush_cmd(s, elem, inhdr);
> >+        do_flush_cmd(s, elem, inhdr, &outhdr);
> >         return 0;
> >
> >     case VIRTIO_BLK_T_GET_ID:
> >
> 
> Can you try moving the req allocation and assignments inside process_request
> instead?  Then you can fill in req->out directly without the struct
> assignment.
> 

The owners of req are do_rdwr_cmd and do_flush_cmd, but do_scsi_cmd and
do_get_id_cmd don't need to allocate.

Moving the allocation means transfering the ownership from process_request to
respective cmd functions, and it's not used in two out of four. Not sure much
better than this way, but that's doable.

Fam