[Qemu-devel] [PATCH 0/6] virtio-blk: Unify request handling of dataplane

[Qemu-devel] [PATCH 0/6] virtio-blk: Unify request handling of dataplane

[Fam Zheng]

This is based on top of my request converging series:

[PATCH v4 0/9] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq

Most of the request handlings are already the same now between dataplane and
non-dataplane, except the missing IO accounting, error reporting and
multiwrite. With this series, dataplane pulls in all of them by reusing
non-dataplane handling code.

Thread safety of error reporting relies on Paolo's series:

    [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe

    [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane
    rerror/werror


Fam Zheng (4):
  virtio-blk: Make request completion function virtual
  virtio-blk: Export request handling functions to dataplane
  virtio-blk: Unify {non-,}dataplane's request handlings
  virtio-blk: Rename complete_request_early to complete_request_vring

Stefan Hajnoczi (2):
  block: make bdrv_query_stats() static
  block: acquire AioContext in qmp_query_blockstats()

 block/qapi.c                    |   6 +-
 hw/block/dataplane/virtio-blk.c | 180 ++++------------------------------------
 hw/block/virtio-blk.c           |  19 +++--
 include/block/qapi.h            |   1 -
 include/hw/virtio/virtio-blk.h  |  12 +++
 5 files changed, 42 insertions(+), 176 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH 1/6] block: make bdrv_query_stats() static

[Fam Zheng]

From: Stefan Hajnoczi <stefanha@redhat.com>

This function is only called from block/qapi.c.  There is no need to
keep it public.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qapi.c         | 2 +-
 include/block/qapi.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 97e1641..aeabaaf 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -293,7 +293,7 @@ void bdrv_query_info(BlockDriverState *bs,
     qapi_free_BlockInfo(info);
 }
 
-BlockStats *bdrv_query_stats(const BlockDriverState *bs)
+static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
 {
     BlockStats *s;
 
diff --git a/include/block/qapi.h b/include/block/qapi.h
index e92c00d..0374546 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -39,7 +39,6 @@ void bdrv_query_image_info(BlockDriverState *bs,
 void bdrv_query_info(BlockDriverState *bs,
                      BlockInfo **p_info,
                      Error **errp);
-BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 
 void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
                         QEMUSnapshotInfo *sn);
-- 
2.0.0

[Qemu-devel] [PATCH 2/6] block: acquire AioContext in qmp_query_blockstats()

[Fam Zheng]

From: Stefan Hajnoczi <stefanha@redhat.com>

Make query-blockstats safe for dataplane by acquiring the
BlockDriverState's AioContext.  This ensures that the dataplane IOThread
and the main loop's monitor code do not race.

Note the assumption that acquiring the drive's BDS AioContext also
protects ->file and ->backing_hd.  This assumption is made by other
aio_context_acquire() callers too.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qapi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/qapi.c b/block/qapi.c
index aeabaaf..f44f6b4 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -360,7 +360,11 @@ BlockStatsList *qmp_query_blockstats(Error **errp)
 
      while ((bs = bdrv_next(bs))) {
         BlockStatsList *info = g_malloc0(sizeof(*info));
+        AioContext *ctx = bdrv_get_aio_context(bs);
+
+        aio_context_acquire(ctx);
         info->value = bdrv_query_stats(bs);
+        aio_context_release(ctx);
 
         *p_next = info;
         p_next = &info->next;
-- 
2.0.0

[Qemu-devel] [PATCH 3/6] virtio-blk: Make request completion function virtual

[Fam Zheng]

virtio_blk_req_complete will call VirtIOBlock.complete_request() to push
data and notify guest. No functional change.

Later, this will allow dataplane to provide it's own (vring_) version.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index b9d0d0a..f32c663 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -44,7 +44,8 @@ static void virtio_blk_free_request(VirtIOBlockReq *req)
     }
 }
 
-static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
+static void virtio_blk_complete_request(VirtIOBlockReq *req,
+                                        unsigned char status)
 {
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -56,6 +57,11 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, int status)
     virtio_notify(vdev, s->vq);
 }
 
+static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
+{
+    req->dev->complete_request(req, status);
+}
+
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     bool is_read)
 {
@@ -751,6 +757,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     add_migration_state_change_notifier(&s->migration_state_notifier);
 #endif
 
+    s->complete_request = virtio_blk_complete_request;
     s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
     register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
                     virtio_blk_save, virtio_blk_load, s);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 2571e96..0398f4c 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -117,6 +117,7 @@ struct VirtIOBlkConf
 
 struct VirtIOBlockDataPlane;
 
+struct VirtIOBlockReq;
 typedef struct VirtIOBlock {
     VirtIODevice parent_obj;
     BlockDriverState *bs;
@@ -128,6 +129,8 @@ typedef struct VirtIOBlock {
     unsigned short sector_mask;
     bool original_wce;
     VMChangeStateEntry *change;
+    /* Function to push to vq and notify guest */
+    void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     Notifier migration_state_notifier;
     struct VirtIOBlockDataPlane *dataplane;
-- 
2.0.0

[Qemu-devel] [PATCH 4/6] virtio-blk: Export request handling functions to dataplane

[Fam Zheng]

So that dataplane can use virtio_blk_handle_request and
virtio_submit_multiwrite.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f32c663..c3133a9 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -253,12 +253,7 @@ static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
     virtio_blk_free_request(req);
 }
 
-typedef struct MultiReqBuffer {
-    BlockRequest        blkreq[32];
-    unsigned int        num_writes;
-} MultiReqBuffer;
-
-static void virtio_submit_multiwrite(BlockDriverState *bs, MultiReqBuffer *mrb)
+void virtio_submit_multiwrite(BlockDriverState *bs, MultiReqBuffer *mrb)
 {
     int i, ret;
 
@@ -347,8 +342,7 @@ static void virtio_blk_handle_read(VirtIOBlockReq *req)
                    virtio_blk_rw_complete, req);
 }
 
-static void virtio_blk_handle_request(VirtIOBlockReq *req,
-    MultiReqBuffer *mrb)
+void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     uint32_t type;
     struct iovec *in_iov = req->elem->in_sg;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 0398f4c..d0fb26f 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -137,6 +137,11 @@ typedef struct VirtIOBlock {
 #endif
 } VirtIOBlock;
 
+typedef struct MultiReqBuffer {
+    BlockRequest        blkreq[32];
+    unsigned int        num_writes;
+} MultiReqBuffer;
+
 typedef struct VirtIOBlockReq {
     VirtIOBlock *dev;
     VirtQueueElement *elem;
@@ -172,4 +177,8 @@ void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk);
 int virtio_blk_handle_scsi_req(VirtIOBlock *blk,
                                VirtQueueElement *elem);
 
+void virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb);
+
+void virtio_submit_multiwrite(BlockDriverState *bs, MultiReqBuffer *mrb);
+
 #endif
-- 
2.0.0

[Qemu-devel] [PATCH 5/6] virtio-blk: Unify {non-, }dataplane's request handlings

[Fam Zheng]

This drops request handling code from dataplane, and uses code from
hw/block/virtio-blk.c.

It starts to use multiwrite as non-dataplane does.

Dataplane sets VirtIOBlock.complete_request to vring version, and calls
into non-dataplane's process handling. In complete_request_early,
qiov.size is added to vring push length, because it's also called in rw
completion now.

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 29e7ec3..47eb087 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -59,179 +59,27 @@ static void notify_guest(VirtIOBlockDataPlane *s)
     event_notifier_set(s->guest_notifier);
 }
 
-static void complete_rdwr(void *opaque, int ret)
-{
-    VirtIOBlockReq *req = opaque;
-    struct virtio_blk_inhdr hdr;
-    int len;
-
-    if (likely(ret == 0)) {
-        hdr.status = VIRTIO_BLK_S_OK;
-        len = req->qiov.size;
-    } else {
-        hdr.status = VIRTIO_BLK_S_IOERR;
-        len = 0;
-    }
-
-    trace_virtio_blk_data_plane_complete_request(req->dev->dataplane,
-                                                 req->elem->index, ret);
-
-    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
-     * transferred plus the status bytes.
-     */
-    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(VirtIOBlockReq *req, unsigned char status)
 {
     stb_p(&req->in->status, status);
 
-    vring_push(&req->dev->dataplane->vring, req->elem, sizeof(*req->in));
+    vring_push(&req->dev->dataplane->vring, req->elem,
+               req->qiov.size + sizeof(*req->in));
     notify_guest(req->dev->dataplane);
     g_slice_free(VirtIOBlockReq, req);
 }
 
-/* Get disk serial number */
-static void do_get_id_cmd(VirtIOBlockReq *req,
-                          struct iovec *iov, unsigned int iov_cnt)
-{
-    char id[VIRTIO_BLK_ID_BYTES];
-
-    /* Serial number not NUL-terminated when longer than buffer */
-    strncpy(id, req->dev->blk.serial ? req->dev->blk.serial : "", sizeof(id));
-    iov_from_buf(iov, iov_cnt, 0, id, sizeof(id));
-    complete_request_early(req, VIRTIO_BLK_S_OK);
-}
-
-
-static void do_rdwr_cmd(bool read, VirtIOBlockReq *req,
-                        struct iovec *iov, unsigned iov_cnt)
-{
-    QEMUIOVector *qiov;
-    int nb_sectors;
-    int64_t sector_num;
-
-    qemu_iovec_init_external(&req->qiov, iov, iov_cnt);
-
-    qiov = &req->qiov;
-
-    sector_num = req->out.sector * 512 / BDRV_SECTOR_SIZE;
-    nb_sectors = qiov->size / BDRV_SECTOR_SIZE;
-
-    if (read) {
-        bdrv_aio_readv(req->dev->blk.conf.bs,
-                       sector_num, qiov, nb_sectors,
-                       complete_rdwr, req);
-    } else {
-        bdrv_aio_writev(req->dev->blk.conf.bs,
-                        sector_num, qiov, nb_sectors,
-                        complete_rdwr, req);
-    }
-}
-
-static void complete_flush(void *opaque, int ret)
-{
-    VirtIOBlockReq *req = opaque;
-    unsigned char status;
-
-    if (ret == 0) {
-        status = VIRTIO_BLK_S_OK;
-    } else {
-        status = VIRTIO_BLK_S_IOERR;
-    }
-
-    complete_request_early(req, status);
-}
-
-static void do_flush_cmd(VirtIOBlockReq *req)
-{
-
-    bdrv_aio_flush(req->dev->blk.conf.bs, complete_flush, req);
-}
-
-static void do_scsi_cmd(VirtIOBlockReq *req)
-{
-    int status;
-
-    status = virtio_blk_handle_scsi_req(req->dev, req->elem);
-    complete_request_early(req, status);
-}
-
-static int process_request(VirtIOBlockDataPlane *s, VirtQueueElement *elem)
-{
-    struct iovec *iov = elem->out_sg;
-    struct iovec *in_iov = elem->in_sg;
-    unsigned out_num = elem->out_num;
-    unsigned in_num = elem->in_num;
-    VirtIOBlockReq *req;
-
-    req = g_slice_new(VirtIOBlockReq);
-    req->dev = VIRTIO_BLK(s->vdev);
-    req->elem = elem;
-    /* Copy in outhdr */
-    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");
-        g_slice_free(VirtIOBlockReq, req);
-        return -EFAULT;
-    }
-    iov_discard_front(&iov, &out_num, sizeof(req->out));
-
-    /* We are likely safe with the iov_len check, because inhdr is only 1 byte,
-     * but checking here in case the header gets bigger in the future. */
-    if (in_num < 1 || in_iov[in_num - 1].iov_len < sizeof(*req->in)) {
-        error_report("virtio-blk request inhdr too short");
-        return -EFAULT;
-    }
-
-    /* Grab inhdr for later */
-    req->in = (void *)in_iov[in_num - 1].iov_base
-            + in_iov[in_num - 1].iov_len - sizeof(*req->in);
-    iov_discard_back(in_iov, &in_num, sizeof(struct virtio_blk_inhdr));
-
-    /* TODO Linux sets the barrier bit even when not advertised! */
-    req->out.type &= ~VIRTIO_BLK_T_BARRIER;
-
-    switch (req->out.type) {
-    case VIRTIO_BLK_T_IN:
-        do_rdwr_cmd(true, req, in_iov, in_num);
-        return 0;
-
-    case VIRTIO_BLK_T_OUT:
-        do_rdwr_cmd(false, req, iov, out_num);
-        return 0;
-
-    case VIRTIO_BLK_T_SCSI_CMD:
-        do_scsi_cmd(req);
-        return 0;
-
-    case VIRTIO_BLK_T_FLUSH:
-        do_flush_cmd(req);
-        return 0;
-
-    case VIRTIO_BLK_T_GET_ID:
-        do_get_id_cmd(req, in_iov, in_num);
-        return 0;
-
-    default:
-        error_report("virtio-blk unsupported request type %#x", req->out.type);
-        g_slice_free(VirtIOBlockReq, req);
-        return -EFAULT;
-    }
-}
-
 static void handle_notify(EventNotifier *e)
 {
     VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane,
                                            host_notifier);
 
     VirtQueueElement *elem;
+    VirtIOBlockReq *req;
     int ret;
+    MultiReqBuffer mrb = {
+        .num_writes = 0,
+    };
 
     event_notifier_test_and_clear(&s->host_notifier);
     for (;;) {
@@ -248,14 +96,14 @@ static void handle_notify(EventNotifier *e)
             trace_virtio_blk_data_plane_process_request(s, elem->out_num,
                                                         elem->in_num, elem->index);
 
-            if (process_request(s, elem) < 0) {
-                vring_set_broken(&s->vring);
-                vring_free_element(elem);
-                ret = -EFAULT;
-                break;
-            }
+            req = g_slice_new(VirtIOBlockReq);
+            req->dev = VIRTIO_BLK(s->vdev);
+            req->elem = elem;
+            virtio_blk_handle_request(req, &mrb);
         }
 
+        virtio_submit_multiwrite(s->blk->conf.bs, &mrb);
+
         if (likely(ret == -EAGAIN)) { /* vring emptied */
             /* Re-enable guest->host notifies and stop processing the vring.
              * But if the guest has snuck in more descriptors, keep processing.
@@ -275,6 +123,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
                                   Error **errp)
 {
     VirtIOBlockDataPlane *s;
+    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
     Error *local_err = NULL;
 
     *dataplane = NULL;
@@ -317,6 +166,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     bdrv_op_block_all(blk->conf.bs, s->blocker);
 
     *dataplane = s;
+    vblk->complete_request = complete_request_early;
 }
 
 /* Context: QEMU global mutex held */
-- 
2.0.0

Re: [Qemu-devel] [PATCH 5/6] virtio-blk: Unify {non-, }dataplane's request handlings

[Paolo Bonzini]

Il 17/06/2014 05:45, Fam Zheng ha scritto:
> @@ -275,6 +123,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>                                    Error **errp)
>  {
>      VirtIOBlockDataPlane *s;
> +    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
>      Error *local_err = NULL;
>
>      *dataplane = NULL;
> @@ -317,6 +166,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>      bdrv_op_block_all(blk->conf.bs, s->blocker);
>
>      *dataplane = s;
> +    vblk->complete_request = complete_request_early;
>  }

This needs to be undone when dataplane is stopped.

Paolo

[Qemu-devel] [PATCH 6/6] virtio-blk: Rename complete_request_early to complete_request_vring

[Fam Zheng]

The old name is misleading in its new usage, so rename it.

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

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 47eb087..e79da1b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -59,7 +59,7 @@ static void notify_guest(VirtIOBlockDataPlane *s)
     event_notifier_set(s->guest_notifier);
 }
 
-static void complete_request_early(VirtIOBlockReq *req, unsigned char status)
+static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
 {
     stb_p(&req->in->status, status);
 
@@ -166,7 +166,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     bdrv_op_block_all(blk->conf.bs, s->blocker);
 
     *dataplane = s;
-    vblk->complete_request = complete_request_early;
+    vblk->complete_request = complete_request_vring;
 }
 
 /* Context: QEMU global mutex held */
-- 
2.0.0

Re: [Qemu-devel] [PATCH 0/6] virtio-blk: Unify request handling of dataplane

[Paolo Bonzini]

Il 17/06/2014 05:44, Fam Zheng ha scritto:
> This is based on top of my request converging series:
>
> [PATCH v4 0/9] virtio-blk: Converge VirtIOBlockRequest into VirtIOBlockReq
>
> Most of the request handlings are already the same now between dataplane and
> non-dataplane, except the missing IO accounting, error reporting and
> multiwrite. With this series, dataplane pulls in all of them by reusing
> non-dataplane handling code.
>
> Thread safety of error reporting relies on Paolo's series:
>
>     [PATCH 0/5] qemu-char/monitor: make monitor_puts thread safe
>
>     [PATCH v2 0/2] block: thread-safety patches for virtio-blk dataplane
>     rerror/werror

I left a comment on patch 5, and it is also missing a patch to create 
the restart BH in the right AioContext.

I suspect even more could be unified, but it can be left to 2.2.  This 
patch gets the features.

Paolo