[Qemu-devel] [PATCH v2 0/8] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

[Qemu-devel] [PATCH v2 0/8] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

[Fam Zheng]

This unifies the request structure used by dataplane and non-dataplane code,
while dropping unnessary fields for bounce buffer and read flag.

Applies on top of Stefan's block tree.

v2: Address Paolo's comments. Thanks for reviewing!

[03/08] virtio-blk: Drop bounce buffer from dataplane code
        Moved to front.
[04/08] virtio-blk: Drop VirtIOBlockRequest.read
        Moved to front.
[05/08] virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq
        Also drop ->s with ->dev->dataplane.
        Fill in ->dev in dataplane code.
[06/08] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
        New.
[07/08] virtio-blk: Convert VirtIOBlockReq.out to structrue
        New.
[08/08] virtio-blk: Fill in VirtIOBlockReq.out in dataplane code
        New.



Fam Zheng (8):
  virtio-blk: Move VirtIOBlockReq to header
  virtio-blk: Convert VirtIOBlockReq.elem to pointer
  virtio-blk: Drop bounce buffer from dataplane code
  virtio-blk: Drop VirtIOBlockRequest.read
  virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq
  virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr
  virtio-blk: Convert VirtIOBlockReq.out to structrue
  virtio-blk: Fill in VirtIOBlockReq.out in dataplane code

 hw/block/dataplane/virtio-blk.c | 118 ++++++++++++----------------------------
 hw/block/virtio-blk.c           | 108 ++++++++++++++++++------------------
 include/hw/virtio/virtio-blk.h  |  11 ++++
 3 files changed, 103 insertions(+), 134 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v2 1/8] virtio-blk: Move VirtIOBlockReq to header

[Fam Zheng]

For later reusing by dataplane code.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 11 -----------
 include/hw/virtio/virtio-blk.h | 11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 0b1446e..49507ac 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -27,17 +27,6 @@
 #endif
 #include "hw/virtio/virtio-bus.h"
 
-typedef struct VirtIOBlockReq
-{
-    VirtIOBlock *dev;
-    VirtQueueElement elem;
-    struct virtio_blk_inhdr *in;
-    struct virtio_blk_outhdr *out;
-    QEMUIOVector qiov;
-    struct VirtIOBlockReq *next;
-    BlockAcctCookie acct;
-} VirtIOBlockReq;
-
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
     VirtIOBlock *s = req->dev;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 4bc9b54..6fc43f1 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -17,6 +17,7 @@
 #include "hw/virtio/virtio.h"
 #include "hw/block/block.h"
 #include "sysemu/iothread.h"
+#include "include/block/block.h"
 
 #define TYPE_VIRTIO_BLK "virtio-blk-device"
 #define VIRTIO_BLK(obj) \
@@ -133,6 +134,16 @@ typedef struct VirtIOBlock {
 #endif
 } VirtIOBlock;
 
+typedef struct VirtIOBlockReq {
+    VirtIOBlock *dev;
+    VirtQueueElement elem;
+    struct virtio_blk_inhdr *in;
+    struct virtio_blk_outhdr *out;
+    QEMUIOVector qiov;
+    struct VirtIOBlockReq *next;
+    BlockAcctCookie acct;
+} VirtIOBlockReq;
+
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
         DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
 
-- 
2.0.0

[Qemu-devel] [PATCH v2 2/8] virtio-blk: Convert VirtIOBlockReq.elem to pointer

[Fam Zheng]

This will make converging with dataplane code easier.

Add virtio_blk_free_request to handle the freeing of request internal
fields.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 85 +++++++++++++++++++++++-------------------
 include/hw/virtio/virtio-blk.h |  2 +-
 2 files changed, 47 insertions(+), 40 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 49507ac..388741e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -27,6 +27,22 @@
 #endif
 #include "hw/virtio/virtio-bus.h"
 
+static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
+{
+    VirtIOBlockReq *req = g_malloc0(sizeof(*req));
+    req->dev = s;
+    req->elem = g_slice_new0(VirtQueueElement);
+    return req;
+}
+
+static void virtio_blk_free_request(VirtIOBlockReq *req)
+{
+    if (req) {
+        g_slice_free(VirtQueueElement, req->elem);
+        g_free(req);
+    }
+}
+
 static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
 {
     VirtIOBlock *s = req->dev;
@@ -35,7 +51,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
     trace_virtio_blk_req_complete(req, status);
 
     stb_p(&req->in->status, status);
-    virtqueue_push(s->vq, &req->elem, req->qiov.size + sizeof(*req->in));
+    virtqueue_push(s->vq, req->elem, req->qiov.size + sizeof(*req->in));
     virtio_notify(vdev, s->vq);
 }
 
@@ -51,7 +67,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     } else if (action == BDRV_ACTION_REPORT) {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_acct_done(s->bs, &req->acct);
-        g_free(req);
+        virtio_blk_free_request(req);
     }
 
     bdrv_error_action(s->bs, action, is_read, error);
@@ -72,7 +88,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     bdrv_acct_done(req->dev->bs, &req->acct);
-    g_free(req);
+    virtio_blk_free_request(req);
 }
 
 static void virtio_blk_flush_complete(void *opaque, int ret)
@@ -87,27 +103,16 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
 
     virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
     bdrv_acct_done(req->dev->bs, &req->acct);
-    g_free(req);
-}
-
-static VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
-{
-    VirtIOBlockReq *req = g_malloc(sizeof(*req));
-    req->dev = s;
-    req->qiov.size = 0;
-    req->next = NULL;
-    return req;
+    virtio_blk_free_request(req);
 }
 
 static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock *s)
 {
     VirtIOBlockReq *req = virtio_blk_alloc_request(s);
 
-    if (req != NULL) {
-        if (!virtqueue_pop(s->vq, &req->elem)) {
-            g_free(req);
-            return NULL;
-        }
+    if (!virtqueue_pop(s->vq, req->elem)) {
+        virtio_blk_free_request(req);
+        return NULL;
     }
 
     return req;
@@ -236,9 +241,9 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
 {
     int status;
 
-    status = virtio_blk_handle_scsi_req(req->dev, &req->elem);
+    status = virtio_blk_handle_scsi_req(req->dev, req->elem);
     virtio_blk_req_complete(req, status);
-    g_free(req);
+    virtio_blk_free_request(req);
 }
 
 typedef struct MultiReqBuffer {
@@ -340,19 +345,19 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
 {
     uint32_t type;
 
-    if (req->elem.out_num < 1 || req->elem.in_num < 1) {
+    if (req->elem->out_num < 1 || req->elem->in_num < 1) {
         error_report("virtio-blk missing headers");
         exit(1);
     }
 
-    if (req->elem.out_sg[0].iov_len < sizeof(*req->out) ||
-        req->elem.in_sg[req->elem.in_num - 1].iov_len < sizeof(*req->in)) {
+    if (req->elem->out_sg[0].iov_len < sizeof(*req->out) ||
+        req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
         error_report("virtio-blk header not in correct element");
         exit(1);
     }
 
-    req->out = (void *)req->elem.out_sg[0].iov_base;
-    req->in = (void *)req->elem.in_sg[req->elem.in_num - 1].iov_base;
+    req->out = (void *)req->elem->out_sg[0].iov_base;
+    req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
 
     type = ldl_p(&req->out->type);
 
@@ -367,23 +372,23 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
          * NB: per existing s/n string convention the string is
          * terminated by '\0' only when shorter than buffer.
          */
-        strncpy(req->elem.in_sg[0].iov_base,
+        strncpy(req->elem->in_sg[0].iov_base,
                 s->blk.serial ? s->blk.serial : "",
-                MIN(req->elem.in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
+                MIN(req->elem->in_sg[0].iov_len, VIRTIO_BLK_ID_BYTES));
         virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
-        g_free(req);
+        virtio_blk_free_request(req);
     } else if (type & VIRTIO_BLK_T_OUT) {
-        qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
-                                 req->elem.out_num - 1);
+        qemu_iovec_init_external(&req->qiov, &req->elem->out_sg[1],
+                                 req->elem->out_num - 1);
         virtio_blk_handle_write(req, mrb);
     } else if (type == VIRTIO_BLK_T_IN || type == VIRTIO_BLK_T_BARRIER) {
         /* VIRTIO_BLK_T_IN is 0, so we can't just & it. */
-        qemu_iovec_init_external(&req->qiov, &req->elem.in_sg[0],
-                                 req->elem.in_num - 1);
+        qemu_iovec_init_external(&req->qiov, &req->elem->in_sg[0],
+                                 req->elem->in_num - 1);
         virtio_blk_handle_read(req);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
-        g_free(req);
+        virtio_blk_free_request(req);
     }
 }
 
@@ -598,7 +603,8 @@ static void virtio_blk_save(QEMUFile *f, void *opaque)
     
     while (req) {
         qemu_put_sbyte(f, 1);
-        qemu_put_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
+        qemu_put_buffer(f, (unsigned char *)req->elem,
+                        sizeof(VirtQueueElement));
         req = req->next;
     }
     qemu_put_sbyte(f, 0);
@@ -620,14 +626,15 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
 
     while (qemu_get_sbyte(f)) {
         VirtIOBlockReq *req = virtio_blk_alloc_request(s);
-        qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
+        qemu_get_buffer(f, (unsigned char *)req->elem,
+                        sizeof(VirtQueueElement));
         req->next = s->rq;
         s->rq = req;
 
-        virtqueue_map_sg(req->elem.in_sg, req->elem.in_addr,
-            req->elem.in_num, 1);
-        virtqueue_map_sg(req->elem.out_sg, req->elem.out_addr,
-            req->elem.out_num, 0);
+        virtqueue_map_sg(req->elem->in_sg, req->elem->in_addr,
+            req->elem->in_num, 1);
+        virtqueue_map_sg(req->elem->out_sg, req->elem->out_addr,
+            req->elem->out_num, 0);
     }
 
     return 0;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 6fc43f1..1932502 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -136,7 +136,7 @@ typedef struct VirtIOBlock {
 
 typedef struct VirtIOBlockReq {
     VirtIOBlock *dev;
-    VirtQueueElement elem;
+    VirtQueueElement *elem;
     struct virtio_blk_inhdr *in;
     struct virtio_blk_outhdr *out;
     QEMUIOVector qiov;
-- 
2.0.0

[Qemu-devel] [PATCH v2 3/8] virtio-blk: Drop bounce buffer from dataplane code

[Fam Zheng]

The block layer will handle the unaligned request.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index d81652f..4873726 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -29,8 +29,6 @@ typedef struct {
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
     VirtQueueElement *elem;         /* saved data from the virtqueue */
     QEMUIOVector qiov;              /* original request iovecs */
-    struct iovec bounce_iov;        /* used if guest buffers are unaligned */
-    QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
     bool read;                      /* read or write? */
 } VirtIOBlockRequest;
 
@@ -84,14 +82,6 @@ static void complete_rdwr(void *opaque, int ret)
 
     trace_virtio_blk_data_plane_complete_request(req->s, req->elem->index, ret);
 
-    if (req->read && req->bounce_iov.iov_base) {
-        qemu_iovec_from_buf(&req->qiov, 0, req->bounce_iov.iov_base, len);
-    }
-
-    if (req->bounce_iov.iov_base) {
-        qemu_vfree(req->bounce_iov.iov_base);
-    }
-
     qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
     qemu_iovec_destroy(req->inhdr);
     g_slice_free(QEMUIOVector, req->inhdr);
@@ -151,21 +141,6 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
 
     qiov = &req->qiov;
 
-    if (!bdrv_qiov_is_aligned(s->blk->conf.bs, qiov)) {
-        void *bounce_buffer = qemu_blockalign(s->blk->conf.bs, qiov->size);
-
-        /* Populate bounce buffer with data for writes */
-        if (!read) {
-            qemu_iovec_to_buf(qiov, 0, bounce_buffer, qiov->size);
-        }
-
-        /* Redirect I/O to aligned bounce buffer */
-        req->bounce_iov.iov_base = bounce_buffer;
-        req->bounce_iov.iov_len = qiov->size;
-        qemu_iovec_init_external(&req->bounce_qiov, &req->bounce_iov, 1);
-        qiov = &req->bounce_qiov;
-    }
-
     nb_sectors = qiov->size / BDRV_SECTOR_SIZE;
 
     if (read) {
-- 
2.0.0

[Qemu-devel] [PATCH v2 4/8] virtio-blk: Drop VirtIOBlockRequest.read

[Fam Zheng]

Since it's set but not used.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 4873726..8393ecf 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -29,7 +29,6 @@ typedef struct {
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
     VirtQueueElement *elem;         /* saved data from the virtqueue */
     QEMUIOVector qiov;              /* original request iovecs */
-    bool read;                      /* read or write? */
 } VirtIOBlockRequest;
 
 struct VirtIOBlockDataPlane {
@@ -136,7 +135,6 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
     req->s = s;
     req->elem = elem;
     req->inhdr = inhdr;
-    req->read = read;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
-- 
2.0.0

[Qemu-devel] [PATCH v2 5/8] virtio-blk: Replace VirtIOBlockRequest with VirtIOBlockReq

[Fam Zheng]

Field "inhdr" is added temporarily for a more mechanical change, and
will be dropped in the next commit.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 34 +++++++++++++++-------------------
 include/hw/virtio/virtio-blk.h  |  4 ++++
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 8393ecf..860cb48 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -24,13 +24,6 @@
 #include "hw/virtio/virtio-bus.h"
 #include "qom/object_interfaces.h"
 
-typedef struct {
-    VirtIOBlockDataPlane *s;
-    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
-    VirtQueueElement *elem;         /* saved data from the virtqueue */
-    QEMUIOVector qiov;              /* original request iovecs */
-} VirtIOBlockRequest;
-
 struct VirtIOBlockDataPlane {
     bool started;
     bool starting;
@@ -67,7 +60,7 @@ static void notify_guest(VirtIOBlockDataPlane *s)
 
 static void complete_rdwr(void *opaque, int ret)
 {
-    VirtIOBlockRequest *req = opaque;
+    VirtIOBlockReq *req = opaque;
     struct virtio_blk_inhdr hdr;
     int len;
 
@@ -79,7 +72,8 @@ static void complete_rdwr(void *opaque, int ret)
         len = 0;
     }
 
-    trace_virtio_blk_data_plane_complete_request(req->s, req->elem->index, ret);
+    trace_virtio_blk_data_plane_complete_request(req->dev->dataplane,
+                                                 req->elem->index, ret);
 
     qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
     qemu_iovec_destroy(req->inhdr);
@@ -89,9 +83,9 @@ static void complete_rdwr(void *opaque, int ret)
      * written to, but for virtio-blk it seems to be the number of bytes
      * transferred plus the status bytes.
      */
-    vring_push(&req->s->vring, req->elem, len + sizeof(hdr));
-    notify_guest(req->s);
-    g_slice_free(VirtIOBlockRequest, req);
+    vring_push(&req->dev->dataplane->vring, req->elem, len + sizeof(hdr));
+    notify_guest(req->dev->dataplane);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 static void complete_request_early(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
@@ -127,14 +121,15 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
                         int64_t sector_num, VirtQueueElement *elem,
                         QEMUIOVector *inhdr)
 {
-    VirtIOBlockRequest *req = g_slice_new0(VirtIOBlockRequest);
+    VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
+    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
     QEMUIOVector *qiov;
     int nb_sectors;
 
     /* Fill in virtio block metadata needed for completion */
-    req->s = s;
     req->elem = elem;
     req->inhdr = inhdr;
+    req->dev = dev;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
@@ -152,7 +147,7 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
 
 static void complete_flush(void *opaque, int ret)
 {
-    VirtIOBlockRequest *req = opaque;
+    VirtIOBlockReq *req = opaque;
     unsigned char status;
 
     if (ret == 0) {
@@ -161,15 +156,16 @@ static void complete_flush(void *opaque, int ret)
         status = VIRTIO_BLK_S_IOERR;
     }
 
-    complete_request_early(req->s, req->elem, req->inhdr, status);
-    g_slice_free(VirtIOBlockRequest, req);
+    complete_request_early(req->dev->dataplane, req->elem, req->inhdr, status);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
                          QEMUIOVector *inhdr)
 {
-    VirtIOBlockRequest *req = g_slice_new(VirtIOBlockRequest);
-    req->s = s;
+    VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
+    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
+    req->dev = dev;
     req->elem = elem;
     req->inhdr = inhdr;
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1932502..9ce0957 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -142,6 +142,10 @@ typedef struct VirtIOBlockReq {
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
+
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
+#endif
 } VirtIOBlockReq;
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-- 
2.0.0

[Qemu-devel] [PATCH v2 6/8] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr

[Fam Zheng]

In current virtio spec, inhdr is a single byte, and is unlikely to
change for both functionality and compatibility considerations.
Non-dataplane uses .in, and we are on the way to converge them. So
let's unify it to get cleaner code.

Remove .inhdr and use .in.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 49 +++++++++++++----------------------------
 include/hw/virtio/virtio-blk.h  |  4 ----
 2 files changed, 15 insertions(+), 38 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 860cb48..96068cd 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -75,9 +75,7 @@ static void complete_rdwr(void *opaque, int ret)
     trace_virtio_blk_data_plane_complete_request(req->dev->dataplane,
                                                  req->elem->index, ret);
 
-    qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
-    qemu_iovec_destroy(req->inhdr);
-    g_slice_free(QEMUIOVector, req->inhdr);
+    stb_p(&req->in->status, hdr.status);
 
     /* According to the virtio specification len should be the number of bytes
      * written to, but for virtio-blk it seems to be the number of bytes
@@ -89,24 +87,20 @@ static void complete_rdwr(void *opaque, int ret)
 }
 
 static void complete_request_early(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                                   QEMUIOVector *inhdr, unsigned char status)
+                                   struct virtio_blk_inhdr *inhdr,
+                                   unsigned char status)
 {
-    struct virtio_blk_inhdr hdr = {
-        .status = status,
-    };
+    stb_p(&inhdr->status, status);
 
-    qemu_iovec_from_buf(inhdr, 0, &hdr, sizeof(hdr));
-    qemu_iovec_destroy(inhdr);
-    g_slice_free(QEMUIOVector, inhdr);
-
-    vring_push(&s->vring, elem, sizeof(hdr));
+    vring_push(&s->vring, elem, sizeof(*inhdr));
     notify_guest(s);
 }
 
 /* Get disk serial number */
 static void do_get_id_cmd(VirtIOBlockDataPlane *s,
                           struct iovec *iov, unsigned int iov_cnt,
-                          VirtQueueElement *elem, QEMUIOVector *inhdr)
+                          VirtQueueElement *elem,
+                          struct virtio_blk_inhdr *inhdr)
 {
     char id[VIRTIO_BLK_ID_BYTES];
 
@@ -119,7 +113,7 @@ 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,
-                        QEMUIOVector *inhdr)
+                        struct virtio_blk_inhdr *inhdr)
 {
     VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
     VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
@@ -128,8 +122,8 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
 
     /* Fill in virtio block metadata needed for completion */
     req->elem = elem;
-    req->inhdr = inhdr;
     req->dev = dev;
+    req->in = inhdr;
     qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
 
     qiov = &req->qiov;
@@ -156,24 +150,24 @@ static void complete_flush(void *opaque, int ret)
         status = VIRTIO_BLK_S_IOERR;
     }
 
-    complete_request_early(req->dev->dataplane, req->elem, req->inhdr, status);
+    complete_request_early(req->dev->dataplane, req->elem, req->in, status);
     g_slice_free(VirtIOBlockReq, req);
 }
 
 static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                         QEMUIOVector *inhdr)
+                         struct virtio_blk_inhdr *inhdr)
 {
     VirtIOBlock *dev = VIRTIO_BLK(s->vdev);
     VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
     req->dev = dev;
     req->elem = elem;
-    req->inhdr = inhdr;
+    req->in = inhdr;
 
     bdrv_aio_flush(s->blk->conf.bs, complete_flush, req);
 }
 
 static void do_scsi_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
-                        QEMUIOVector *inhdr)
+                        struct virtio_blk_inhdr *inhdr)
 {
     int status;
 
@@ -188,8 +182,7 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
     unsigned out_num = elem->out_num;
     unsigned in_num = elem->in_num;
     struct virtio_blk_outhdr outhdr;
-    QEMUIOVector *inhdr;
-    size_t in_size;
+    struct virtio_blk_inhdr *inhdr;
 
     /* Copy in outhdr */
     if (unlikely(iov_to_buf(iov, out_num, 0, &outhdr,
@@ -200,17 +193,7 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
     iov_discard_front(&iov, &out_num, sizeof(outhdr));
 
     /* Grab inhdr for later */
-    in_size = iov_size(in_iov, in_num);
-    if (in_size < sizeof(struct virtio_blk_inhdr)) {
-        error_report("virtio_blk request inhdr too short");
-        return -EFAULT;
-    }
-    inhdr = g_slice_new(QEMUIOVector);
-    qemu_iovec_init(inhdr, 1);
-    qemu_iovec_concat_iov(inhdr, in_iov, in_num,
-            in_size - sizeof(struct virtio_blk_inhdr),
-            sizeof(struct virtio_blk_inhdr));
-    iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
+    inhdr = (void *)in_iov[in_num - 1].iov_base;
 
     /* TODO Linux sets the barrier bit even when not advertised! */
     outhdr.type &= ~VIRTIO_BLK_T_BARRIER;
@@ -242,8 +225,6 @@ static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
 
     default:
         error_report("virtio-blk unsupported request type %#x", outhdr.type);
-        qemu_iovec_destroy(inhdr);
-        g_slice_free(QEMUIOVector, inhdr);
         return -EFAULT;
     }
 }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 9ce0957..1932502 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -142,10 +142,6 @@ typedef struct VirtIOBlockReq {
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
-
-#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
-    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
-#endif
 } VirtIOBlockReq;
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-- 
2.0.0

Re: [Qemu-devel] [PATCH v2 6/8] virtio-blk: Use VirtIOBlockReq.in to drop VirtIOBlockReq.inhdr

[Paolo Bonzini]

Il 05/06/2014 03:58, Fam Zheng ha scritto:
>      /* Grab inhdr for later */
> -    in_size = iov_size(in_iov, in_num);
> -    if (in_size < sizeof(struct virtio_blk_inhdr)) {
> -        error_report("virtio_blk request inhdr too short");
> -        return -EFAULT;
> -    }
> -    inhdr = g_slice_new(QEMUIOVector);
> -    qemu_iovec_init(inhdr, 1);
> -    qemu_iovec_concat_iov(inhdr, in_iov, in_num,
> -            in_size - sizeof(struct virtio_blk_inhdr),
> -            sizeof(struct virtio_blk_inhdr));
> -    iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
> +    inhdr = (void *)in_iov[in_num - 1].iov_base;

This would assume a particular layout for the virtio buffers.  You need 
to add in_iov[in_num - 1].iov_len - sizeof(struct virtio_blk_header).

It's probably also good to ahve an assertion that in_iov[in_num - 
1].iov_len > sizeof(struct virtio_blk_header), and a comment saying that 
the assertion is always true because the struct is only 1-byte long.

Paolo

[Qemu-devel] [PATCH v2 7/8] virtio-blk: Convert VirtIOBlockReq.out to structrue

[Fam Zheng]

The virtio code currently assumes that the outhdr is in its own iovec.
This is not guaranteed by the spec, so we should relax this assumption.

Convert the VirtIOBlockReq.out field to structrue so that we can use
iov_to_buf and then discard the header from the beginning of iovec.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/block/virtio-blk.c          | 20 ++++++++++++++------
 include/hw/virtio/virtio-blk.h |  2 +-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 388741e..2282e61 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu-common.h"
+#include "qemu/iov.h"
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "hw/block/block.h"
@@ -81,7 +82,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
     trace_virtio_blk_rw_complete(req, ret);
 
     if (ret) {
-        bool is_read = !(ldl_p(&req->out->type) & VIRTIO_BLK_T_OUT);
+        bool is_read = !(ldl_p(&req->out.type) & VIRTIO_BLK_T_OUT);
         if (virtio_blk_handle_rw_error(req, -ret, is_read))
             return;
     }
@@ -287,7 +288,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     BlockRequest *blkreq;
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = ldq_p(&req->out.sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_WRITE);
 
@@ -321,7 +322,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
 {
     uint64_t sector;
 
-    sector = ldq_p(&req->out->sector);
+    sector = ldq_p(&req->out.sector);
 
     bdrv_acct_start(req->dev->bs, &req->acct, req->qiov.size, BDRV_ACCT_READ);
 
@@ -344,22 +345,29 @@ static void virtio_blk_handle_request(VirtIOBlockReq *req,
     MultiReqBuffer *mrb)
 {
     uint32_t type;
+    struct iovec *iov = req->elem->out_sg;
+    unsigned out_num = req->elem->out_num;
 
     if (req->elem->out_num < 1 || req->elem->in_num < 1) {
         error_report("virtio-blk missing headers");
         exit(1);
     }
 
-    if (req->elem->out_sg[0].iov_len < sizeof(*req->out) ||
+    if (req->elem->out_sg[0].iov_len < sizeof(req->out) ||
         req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
         error_report("virtio-blk header not in correct element");
         exit(1);
     }
 
-    req->out = (void *)req->elem->out_sg[0].iov_base;
+    if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
+                            sizeof(req->out)) != sizeof(req->out))) {
+        error_report("virtio-blk request outhdr too short");
+        exit(1);
+    }
+    iov_discard_front(&iov, &out_num, sizeof(req->out));
     req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;
 
-    type = ldl_p(&req->out->type);
+    type = ldl_p(&req->out.type);
 
     if (type & VIRTIO_BLK_T_FLUSH) {
         virtio_blk_handle_flush(req, mrb);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1932502..df9b6f9 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -138,7 +138,7 @@ typedef struct VirtIOBlockReq {
     VirtIOBlock *dev;
     VirtQueueElement *elem;
     struct virtio_blk_inhdr *in;
-    struct virtio_blk_outhdr *out;
+    struct virtio_blk_outhdr out;
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
-- 
2.0.0

Re: [Qemu-devel] [PATCH v2 7/8] virtio-blk: Convert VirtIOBlockReq.out to structrue

[Paolo Bonzini]

Il 05/06/2014 03:58, Fam Zheng ha scritto:
>
> -    if (req->elem->out_sg[0].iov_len < sizeof(*req->out) ||
> +    if (req->elem->out_sg[0].iov_len < sizeof(req->out) ||

This is not needed anymore.

>          req->elem->in_sg[req->elem->in_num - 1].iov_len < sizeof(*req->in)) {
>          error_report("virtio-blk header not in correct element");
>          exit(1);
>      }
>
> -    req->out = (void *)req->elem->out_sg[0].iov_base;
> +    if (unlikely(iov_to_buf(iov, out_num, 0, &req->out,
> +                            sizeof(req->out)) != sizeof(req->out))) {
> +        error_report("virtio-blk request outhdr too short");
> +        exit(1);
> +    }
> +    iov_discard_front(&iov, &out_num, sizeof(req->out));
>      req->in = (void *)req->elem->in_sg[req->elem->in_num - 1].iov_base;

Here, in a separate patch, you can add iov_len-1 as suggested in the 
previous review, and then use iov_discard_back on in_num.  You can then 
drop the other part of the "virtio-blk header not in correct element" check.

Paolo

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

[Fam Zheng]

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:
-- 
2.0.0

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

[Paolo Bonzini]

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.

Paolo

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

[Fam Zheng]

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

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

[Paolo Bonzini]

Il 05/06/2014 05:50, Fam Zheng ha scritto:
>> > 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.

They don't need it, but using req there and freeing it in 
complete_request_early perhaps could simplify the code.

After all, the first three arguments of complete_request_early (s, elem, 
inhdr) are a duplicate of VirtIOBlockReq and do_flush_cmd is already 
doing a free after complete_request_early.

Paolo

> 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.

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

[Fam Zheng]

On Thu, 06/05 06:09, Paolo Bonzini wrote:
> Il 05/06/2014 05:50, Fam Zheng ha scritto:
> >>> 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.
> 
> They don't need it, but using req there and freeing it in
> complete_request_early perhaps could simplify the code.
> 
> After all, the first three arguments of complete_request_early (s, elem,
> inhdr) are a duplicate of VirtIOBlockReq and do_flush_cmd is already doing a
> free after complete_request_early.
> 

Yes. Although It's not really "early" complete from do_flush_cmd, it's actually
a cb complete.

But the point makes sense for me. I'll do it.

Fam