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

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

[Fam Zheng]

v2: Address Paolo's comments:

  * Schedule restart BH in the right AioContext.
  * Restore ->complete_request when stopping dataplane.  

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 (5):
  virtio-blk: Make request completion function virtual
  virtio-blk: Export request handling functions to dataplane
  virtio-blk: Schedule BH in the right context
  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 | 185 +++++-----------------------------------
 hw/block/virtio-blk.c           |  22 ++---
 include/block/qapi.h            |   1 -
 include/hw/virtio/virtio-blk.h  |  12 +++
 5 files changed, 49 insertions(+), 177 deletions(-)

-- 
2.0.0

[Qemu-devel] [PATCH v2 1/7] 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 v2 2/7] 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 v2 3/7] 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..aed580c 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)
 {
@@ -740,6 +746,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
+    s->complete_request = virtio_blk_complete_request;
 #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
     virtio_blk_data_plane_create(vdev, blk, &s->dataplane, &err);
     if (err != NULL) {
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 v2 4/7] 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 aed580c..25c3812 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 v2 5/7] virtio-blk: Schedule BH in the right context

[Fam Zheng]

The BH must be called in the AioContext of bs. Currently it is only the
main loop, but with coming changes, it could also be a dataplane
IOThread.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 25c3812..ca09bb4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -469,7 +469,8 @@ static void virtio_blk_dma_restart_cb(void *opaque, int running,
     }
 
     if (!s->bh) {
-        s->bh = qemu_bh_new(virtio_blk_dma_restart_bh, s);
+        s->bh = aio_bh_new(bdrv_get_aio_context(s->blk.conf.bs),
+                           virtio_blk_dma_restart_bh, s);
         qemu_bh_schedule(s->bh);
     }
 }
-- 
2.0.0

[Qemu-devel] [PATCH v2 6/7] 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 | 183 +++++-----------------------------------
 1 file changed, 19 insertions(+), 164 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 29e7ec3..b6afa55 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -47,6 +47,8 @@ struct VirtIOBlockDataPlane {
 
     /* Operation blocker on BDS */
     Error *blocker;
+    void (*saved_complete_request)(struct VirtIOBlockReq *req,
+                                   unsigned char status);
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -59,179 +61,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 +98,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 +125,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 +168,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     bdrv_op_block_all(blk->conf.bs, s->blocker);
 
     *dataplane = s;
+    s->saved_complete_request = vblk->complete_request;
+    vblk->complete_request = complete_request_early;
 }
 
 /* Context: QEMU global mutex held */
@@ -391,10 +244,12 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
     if (!s->started || s->stopping) {
         return;
     }
     s->stopping = true;
+    vblk->complete_request = s->saved_complete_request;
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
-- 
2.0.0

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

[Max Reitz]

On 17.06.2014 08:32, Fam Zheng wrote:
> 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 | 183 +++++-----------------------------------
>   1 file changed, 19 insertions(+), 164 deletions(-)

Is it intended that qemu segfaults after this commit with x-data-plane=on?

$ ./qemu-img create -f qcow2 test.qcow2 64M
$ x86_64-softmmu/qemu-system-x86_64 -drive 
if=none,file=test.qcow2,id=drv0 -device 
virtio-blk-pci,drive=drv0,x-data-plane=on
[1]    4604 segmentation fault  x86_64-softmmu/qemu-system-x86_64 -drive 
if=none,file=test.qcow2,id=drv0

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffd75ff700 (LWP 5217)]
0x00005555556373af in virtio_blk_rw_complete (opaque=0x5555565ff5e0, 
ret=0) at hw/block/virtio-blk.c:99
99          bdrv_acct_done(req->dev->bs, &req->acct);
(gdb) print req
$1 = (VirtIOBlockReq *) 0x5555565ff5e0
(gdb) print req->dev
$2 = (VirtIOBlock *) 0x0
(gdb) bt
#0  0x00005555556373af in virtio_blk_rw_complete (opaque=0x5555565ff5e0, 
ret=0) at hw/block/virtio-blk.c:99
#1  0x0000555555840ebe in bdrv_co_em_bh (opaque=0x5555566152d0) at 
block.c:4675
#2  0x000055555583de77 in aio_bh_poll (ctx=ctx@entry=0x5555563a8150) at 
async.c:81
#3  0x000055555584b7a7 in aio_poll (ctx=0x5555563a8150, 
blocking=blocking@entry=true) at aio-posix.c:188
#4  0x00005555556e520e in iothread_run (opaque=0x5555563a7fd8) at 
iothread.c:41
#5  0x00007ffff42ba124 in start_thread () from /usr/lib/libpthread.so.0
#6  0x00007ffff16d14bd in clone () from /usr/lib/libc.so.6

Max

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2178 bytes --]

On Tue, Jul 01, 2014 at 01:29:36PM +0200, Max Reitz wrote:
> On 17.06.2014 08:32, Fam Zheng wrote:
> >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 | 183 +++++-----------------------------------
> >  1 file changed, 19 insertions(+), 164 deletions(-)
> 
> Is it intended that qemu segfaults after this commit with x-data-plane=on?
> 
> $ ./qemu-img create -f qcow2 test.qcow2 64M
> $ x86_64-softmmu/qemu-system-x86_64 -drive if=none,file=test.qcow2,id=drv0
> -device virtio-blk-pci,drive=drv0,x-data-plane=on
> [1]    4604 segmentation fault  x86_64-softmmu/qemu-system-x86_64 -drive
> if=none,file=test.qcow2,id=drv0
> 
> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7fffd75ff700 (LWP 5217)]
> 0x00005555556373af in virtio_blk_rw_complete (opaque=0x5555565ff5e0, ret=0)
> at hw/block/virtio-blk.c:99
> 99          bdrv_acct_done(req->dev->bs, &req->acct);
> (gdb) print req
> $1 = (VirtIOBlockReq *) 0x5555565ff5e0
> (gdb) print req->dev
> $2 = (VirtIOBlock *) 0x0
> (gdb) bt
> #0  0x00005555556373af in virtio_blk_rw_complete (opaque=0x5555565ff5e0,
> ret=0) at hw/block/virtio-blk.c:99
> #1  0x0000555555840ebe in bdrv_co_em_bh (opaque=0x5555566152d0) at
> block.c:4675
> #2  0x000055555583de77 in aio_bh_poll (ctx=ctx@entry=0x5555563a8150) at
> async.c:81
> #3  0x000055555584b7a7 in aio_poll (ctx=0x5555563a8150,
> blocking=blocking@entry=true) at aio-posix.c:188
> #4  0x00005555556e520e in iothread_run (opaque=0x5555563a7fd8) at
> iothread.c:41
> #5  0x00007ffff42ba124 in start_thread () from /usr/lib/libpthread.so.0
> #6  0x00007ffff16d14bd in clone () from /usr/lib/libc.so.6

I'm looking into this and will send a patch to fix it.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH v2 7/7] 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 b6afa55..09bd2c7 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -61,7 +61,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);
 
@@ -169,7 +169,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
 
     *dataplane = s;
     s->saved_complete_request = vblk->complete_request;
-    vblk->complete_request = complete_request_early;
+    vblk->complete_request = complete_request_vring;
 }
 
 /* Context: QEMU global mutex held */
-- 
2.0.0

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

[Paolo Bonzini]

Il 17/06/2014 08:32, Fam Zheng ha scritto:
> v2: Address Paolo's comments:
>
>   * Schedule restart BH in the right AioContext.
>   * Restore ->complete_request when stopping dataplane.
>
> 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 (5):
>   virtio-blk: Make request completion function virtual
>   virtio-blk: Export request handling functions to dataplane
>   virtio-blk: Schedule BH in the right context
>   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 | 185 +++++-----------------------------------
>  hw/block/virtio-blk.c           |  22 ++---
>  include/block/qapi.h            |   1 -
>  include/hw/virtio/virtio-blk.h  |  12 +++
>  5 files changed, 49 insertions(+), 177 deletions(-)
>

Tested-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On Tue, Jun 17, 2014 at 02:32:03PM +0800, Fam Zheng wrote:
> v2: Address Paolo's comments:
> 
>   * Schedule restart BH in the right AioContext.
>   * Restore ->complete_request when stopping dataplane.  
> 
> 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 (5):
>   virtio-blk: Make request completion function virtual
>   virtio-blk: Export request handling functions to dataplane
>   virtio-blk: Schedule BH in the right context
>   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 | 185 +++++-----------------------------------
>  hw/block/virtio-blk.c           |  22 ++---
>  include/block/qapi.h            |   1 -
>  include/hw/virtio/virtio-blk.h  |  12 +++
>  5 files changed, 49 insertions(+), 177 deletions(-)
> 
> -- 
> 2.0.0
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

Am 27.06.2014 um 18:01 hat Stefan Hajnoczi geschrieben:
> On Tue, Jun 17, 2014 at 02:32:03PM +0800, Fam Zheng wrote:
> > v2: Address Paolo's comments:
> > 
> >   * Schedule restart BH in the right AioContext.
> >   * Restore ->complete_request when stopping dataplane.  
> > 
> > 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 (5):
> >   virtio-blk: Make request completion function virtual
> >   virtio-blk: Export request handling functions to dataplane
> >   virtio-blk: Schedule BH in the right context
> >   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 | 185 +++++-----------------------------------
> >  hw/block/virtio-blk.c           |  22 ++---
> >  include/block/qapi.h            |   1 -
> >  include/hw/virtio/virtio-blk.h  |  12 +++
> >  5 files changed, 49 insertions(+), 177 deletions(-)
> > 
> > -- 
> > 2.0.0
> > 
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks, applied all to the block branch.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]