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

[Qemu-devel] [PATCH 0/5] 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 the "[PATCH v3 0/2] dataplane: Enable "scsi=on" series.


Fam Zheng (5):
  virtio-blk: Move VirtIOBlockReq to header
  virtio-blk: Convert VirtIOBlockReq.elem to pointer
  virtio-blk: Merge VirtIOBlockRequest into VirtIOBlockReq
  virtio-blk: Drop bounce buffer from dataplane code
  virtio-blk: Drop VirtIOBlockReq.read

 hw/block/dataplane/virtio-blk.c | 45 +++-----------------
 hw/block/virtio-blk.c           | 92 ++++++++++++++++++++---------------------
 include/hw/virtio/virtio-blk.h  | 16 +++++++
 3 files changed, 66 insertions(+), 87 deletions(-)

-- 
1.9.2

[Qemu-devel] [PATCH 1/5] 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)
 
-- 
1.9.2

[Qemu-devel] [PATCH 2/5] 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;
-- 
1.9.2

[Qemu-devel] [PATCH 3/5] virtio-blk: Merge VirtIOBlockRequest into VirtIOBlockReq

[Fam Zheng]

This puts all the necessary fields into VirtIOBlockReq and replace
VirtIOBlockRequest.

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index d81652f..968f824 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -24,15 +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 */
-    struct iovec bounce_iov;        /* used if guest buffers are unaligned */
-    QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
-    bool read;                      /* read or write? */
-} VirtIOBlockRequest;
 
 struct VirtIOBlockDataPlane {
     bool started;
@@ -70,7 +61,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;
 
@@ -102,7 +93,7 @@ static void complete_rdwr(void *opaque, int ret)
      */
     vring_push(&req->s->vring, req->elem, len + sizeof(hdr));
     notify_guest(req->s);
-    g_slice_free(VirtIOBlockRequest, req);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 static void complete_request_early(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
@@ -138,7 +129,7 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
                         int64_t sector_num, VirtQueueElement *elem,
                         QEMUIOVector *inhdr)
 {
-    VirtIOBlockRequest *req = g_slice_new0(VirtIOBlockRequest);
+    VirtIOBlockReq *req = g_slice_new0(VirtIOBlockReq);
     QEMUIOVector *qiov;
     int nb_sectors;
 
@@ -179,7 +170,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) {
@@ -189,13 +180,13 @@ static void complete_flush(void *opaque, int ret)
     }
 
     complete_request_early(req->s, req->elem, req->inhdr, status);
-    g_slice_free(VirtIOBlockRequest, req);
+    g_slice_free(VirtIOBlockReq, req);
 }
 
 static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
                          QEMUIOVector *inhdr)
 {
-    VirtIOBlockRequest *req = g_slice_new(VirtIOBlockRequest);
+    VirtIOBlockReq *req = g_slice_new(VirtIOBlockReq);
     req->s = s;
     req->elem = elem;
     req->inhdr = inhdr;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 1932502..d9ff1c9 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -142,6 +142,14 @@ typedef struct VirtIOBlockReq {
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
+
+#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
+    struct VirtIOBlockDataPlane *s;
+    QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
+    struct iovec bounce_iov;        /* used if guest buffers are unaligned */
+    QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
+    bool read;                      /* read or write? */
+#endif
 } VirtIOBlockReq;
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-- 
1.9.2

[Qemu-devel] [PATCH 4/5] 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 | 23 -----------------------
 include/hw/virtio/virtio-blk.h  |  2 --
 2 files changed, 25 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 968f824..b88eee9 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -75,14 +75,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);
@@ -142,21 +134,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) {
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index d9ff1c9..e406efa 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -146,8 +146,6 @@ typedef struct VirtIOBlockReq {
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     struct VirtIOBlockDataPlane *s;
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
-    struct iovec bounce_iov;        /* used if guest buffers are unaligned */
-    QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
     bool read;                      /* read or write? */
 #endif
 } VirtIOBlockReq;
-- 
1.9.2

Re: [Qemu-devel] [PATCH 4/5] virtio-blk: Drop bounce buffer from dataplane code

[Paolo Bonzini]

Il 03/06/2014 14:52, Fam Zheng ha scritto:
> The block layer will handle the unaligned request.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 23 -----------------------
>  include/hw/virtio/virtio-blk.h  |  2 --
>  2 files changed, 25 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 968f824..b88eee9 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -75,14 +75,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);
> @@ -142,21 +134,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) {
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index d9ff1c9..e406efa 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -146,8 +146,6 @@ typedef struct VirtIOBlockReq {
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>      struct VirtIOBlockDataPlane *s;
>      QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
> -    struct iovec bounce_iov;        /* used if guest buffers are unaligned */
> -    QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
>      bool read;                      /* read or write? */
>  #endif
>  } VirtIOBlockReq;
>

Can you move this before patch 3?

Paolo

[Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Fam Zheng]

Since it's set but not used.

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index b88eee9..c75141b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -129,7 +129,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;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index e406efa..74f0f32 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq {
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     struct VirtIOBlockDataPlane *s;
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
-    bool read;                      /* read or write? */
 #endif
 } VirtIOBlockReq;
 
-- 
1.9.2

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Paolo Bonzini]

I guess this is more of an RFC, but still a useful starting point for 
discussion.

Il 03/06/2014 14:52, Fam Zheng ha scritto:
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index e406efa..74f0f32 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq {
>  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
>      struct VirtIOBlockDataPlane *s;

This is just dev->dataplane, so it's trivial to remove.

>      QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */

This can be unified with the "in" field; the status is only one byte, so 
using a full-blown QEMUIOVector is overkill.  Stefan, what do you think?

For the sake of restarting requests, we also need dataplane to populate 
the "out" field for dataplane.  We can also take the occasion to change 
it from "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for 
non-dataplane and use iov_discard_front on the elem (see dataplane's 
process_request function).

Can you do it in v2 of this patch series?  With this in place we can 
look at the missing pieces:

- rerror/werror

- accounting (trivial)

- multiwrite (if desired).

Thanks,

Paolo

> -    bool read;                      /* read or write? */
>  #endif
>  } VirtIOBlockReq;
>
>

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Fam Zheng]

On Tue, 06/03 15:37, Paolo Bonzini wrote:
> I guess this is more of an RFC, but still a useful starting point for
> discussion.
> 
> Il 03/06/2014 14:52, Fam Zheng ha scritto:
> >diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> >index e406efa..74f0f32 100644
> >--- a/include/hw/virtio/virtio-blk.h
> >+++ b/include/hw/virtio/virtio-blk.h
> >@@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq {
> > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> >     struct VirtIOBlockDataPlane *s;
> 
> This is just dev->dataplane, so it's trivial to remove.
> 
> >     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
> 
> This can be unified with the "in" field; the status is only one byte, so
> using a full-blown QEMUIOVector is overkill.  Stefan, what do you think?
> 
> For the sake of restarting requests, we also need dataplane to populate the
> "out" field for dataplane.  We can also take the occasion to change it from
> "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane
> and use iov_discard_front on the elem (see dataplane's process_request
> function).
> 
> Can you do it in v2 of this patch series?  With this in place we can look at
> the missing pieces:
> 
> - rerror/werror
> 
> - accounting (trivial)
> 
> - multiwrite (if desired).
> 

Yes, good idea. I will do it!

Thanks,
Fam

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Fam Zheng]

On Tue, 06/03 15:37, Paolo Bonzini wrote:
> I guess this is more of an RFC, but still a useful starting point for
> discussion.
> 
> Il 03/06/2014 14:52, Fam Zheng ha scritto:
> >diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> >index e406efa..74f0f32 100644
> >--- a/include/hw/virtio/virtio-blk.h
> >+++ b/include/hw/virtio/virtio-blk.h
> >@@ -146,7 +146,6 @@ typedef struct VirtIOBlockReq {
> > #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> >     struct VirtIOBlockDataPlane *s;
> 
> This is just dev->dataplane, so it's trivial to remove.
> 
> >     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
> 
> This can be unified with the "in" field; the status is only one byte, so
> using a full-blown QEMUIOVector is overkill.  Stefan, what do you think?
> 
> For the sake of restarting requests, we also need dataplane to populate the
> "out" field for dataplane.  We can also take the occasion to change it from
> "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane
> and use iov_discard_front on the elem (see dataplane's process_request
> function).

Using pointer avoids copying, what's the advantage of converting to "sturct
virtio_blk_outhdr" for non-dataplane code? For thread-safety?

Fam

> 
> Can you do it in v2 of this patch series?  With this in place we can look at
> the missing pieces:
> 
> - rerror/werror
> 
> - accounting (trivial)
> 
> - multiwrite (if desired).
> 
> Thanks,
> 
> Paolo
> 
> >-    bool read;                      /* read or write? */
> > #endif
> > } VirtIOBlockReq;
> >
> >
> 

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Paolo Bonzini]

Il 04/06/2014 09:53, Fam Zheng ha scritto:
>> > For the sake of restarting requests, we also need dataplane to populate the
>> > "out" field for dataplane.  We can also take the occasion to change it from
>> > "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane
>> > and use iov_discard_front on the elem (see dataplane's process_request
>> > function).
> Using pointer avoids copying, what's the advantage of converting to "sturct
> virtio_blk_outhdr" for non-dataplane code? For thread-safety?

The virtio code currently assumes that the outhdr is in its own iovec. 
This is not guaranteed by the spec, it's just that the drivers do it 
because QEMU required it.

Paolo

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Fam Zheng]

On Wed, 06/04 10:42, Paolo Bonzini wrote:
> Il 04/06/2014 09:53, Fam Zheng ha scritto:
> >>> For the sake of restarting requests, we also need dataplane to populate the
> >>> "out" field for dataplane.  We can also take the occasion to change it from
> >>> "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane
> >>> and use iov_discard_front on the elem (see dataplane's process_request
> >>> function).
> >Using pointer avoids copying, what's the advantage of converting to "sturct
> >virtio_blk_outhdr" for non-dataplane code? For thread-safety?
> 
> The virtio code currently assumes that the outhdr is in its own iovec. This
> is not guaranteed by the spec, it's just that the drivers do it because QEMU
> required it.
> 

I see, thanks. Will convert it. IIUC, this also applies to inhdr? Do we need to
fix them as well?

Fam

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Paolo Bonzini]

Il 04/06/2014 13:03, Fam Zheng ha scritto:
> On Wed, 06/04 10:42, Paolo Bonzini wrote:
>> Il 04/06/2014 09:53, Fam Zheng ha scritto:
>>>>> For the sake of restarting requests, we also need dataplane to populate the
>>>>> "out" field for dataplane.  We can also take the occasion to change it from
>>>>> "struct virtio_blk_outhdr *" to "struct virtio_blk_outhdr" for non-dataplane
>>>>> and use iov_discard_front on the elem (see dataplane's process_request
>>>>> function).
>>> Using pointer avoids copying, what's the advantage of converting to "sturct
>>> virtio_blk_outhdr" for non-dataplane code? For thread-safety?
>>
>> The virtio code currently assumes that the outhdr is in its own iovec. This
>> is not guaranteed by the spec, it's just that the drivers do it because QEMU
>> required it.
>
> I see, thanks. Will convert it. IIUC, this also applies to inhdr? Do we need to
> fix them as well?

No, but only because inhdr is a single byte.

Paolo

Re: [Qemu-devel] [PATCH 5/5] virtio-blk: Drop VirtIOBlockReq.read

[Stefan Hajnoczi]

On Tue, Jun 03, 2014 at 03:37:29PM +0200, Paolo Bonzini wrote:
> >     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
> 
> This can be unified with the "in" field; the status is only one byte, so
> using a full-blown QEMUIOVector is overkill.  Stefan, what do you think?

I thought about this when initially writing the code, but it seemed like
a hack to assume virtio_blk_inhdr will always be 1 byte.

virtio-blk is unlikely to change much since the focus is on virtio-scsi
rather than piling on more virtio-blk feature.

I still prefer we treat it like a struct without making size
assumptions, but if the code turns out to be nicer then I don't mind.

Stefan