[Qemu-devel] [PATCH 0/3] dataplane: add query-blockstats support

[Qemu-devel] [PATCH 0/3] dataplane: add query-blockstats support

[Stefan Hajnoczi]

This series adds blockstats support to virtio-blk data-plane and protects
query-blockstats from races with the dataplane IOThread.

Stefan Hajnoczi (3):
  dataplane: add bdrv_acct_*() accounting
  block: make bdrv_query_stats() static
  block: acquire AioContext is qmp_query_blockstats()

 block/qapi.c                    |  6 +++++-
 hw/block/dataplane/virtio-blk.c | 10 ++++++++++
 include/block/qapi.h            |  1 -
 3 files changed, 15 insertions(+), 2 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/3] dataplane: add bdrv_acct_*() accounting

[Stefan Hajnoczi]

bdrv_acct_start() and bdrv_acct_done() must be called in order to update
block device accounting information.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 8ac6049..c5c25cb 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -32,6 +32,7 @@ typedef struct {
     struct iovec bounce_iov;        /* used if guest buffers are unaligned */
     QEMUIOVector bounce_qiov;       /* bounce buffer iovecs */
     bool read;                      /* read or write? */
+    BlockAcctCookie acct;           /* block device accounting */
 } VirtIOBlockRequest;
 
 struct VirtIOBlockDataPlane {
@@ -103,6 +104,9 @@ static void complete_rdwr(void *opaque, int ret)
      */
     vring_push(&req->s->vring, req->elem, len + sizeof(hdr));
     notify_guest(req->s);
+    if (likely(ret == 0)) {
+        bdrv_acct_done(req->s->blk->conf.bs, &req->acct);
+    }
     g_slice_free(VirtIOBlockRequest, req);
 }
 
@@ -170,9 +174,13 @@ static void do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
     nb_sectors = qiov->size / BDRV_SECTOR_SIZE;
 
     if (read) {
+        bdrv_acct_start(s->blk->conf.bs, &req->acct,
+                        qiov->size, BDRV_ACCT_READ);
         bdrv_aio_readv(s->blk->conf.bs, sector_num, qiov, nb_sectors,
                        complete_rdwr, req);
     } else {
+        bdrv_acct_start(s->blk->conf.bs, &req->acct,
+                        qiov->size, BDRV_ACCT_WRITE);
         bdrv_aio_writev(s->blk->conf.bs, sector_num, qiov, nb_sectors,
                         complete_rdwr, req);
     }
@@ -185,6 +193,7 @@ static void complete_flush(void *opaque, int ret)
 
     if (ret == 0) {
         status = VIRTIO_BLK_S_OK;
+        bdrv_acct_done(req->s->blk->conf.bs, &req->acct);
     } else {
         status = VIRTIO_BLK_S_IOERR;
     }
@@ -201,6 +210,7 @@ static void do_flush_cmd(VirtIOBlockDataPlane *s, VirtQueueElement *elem,
     req->elem = elem;
     req->inhdr = inhdr;
 
+    bdrv_acct_start(s->blk->conf.bs, &req->acct, 0, BDRV_ACCT_FLUSH);
     bdrv_aio_flush(s->blk->conf.bs, complete_flush, req);
 }
 
-- 
1.9.3

[Qemu-devel] [PATCH 2/3] block: make bdrv_query_stats() static

[Stefan Hajnoczi]

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>
---
 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);
-- 
1.9.3

[Qemu-devel] [PATCH 3/3] block: acquire AioContext is qmp_query_blockstats()

[Stefan Hajnoczi]

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>
---
 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;
-- 
1.9.3

Re: [Qemu-devel] [PATCH 0/3] dataplane: add query-blockstats support

[Paolo Bonzini]

Il 10/06/2014 09:29, Stefan Hajnoczi ha scritto:
> This series adds blockstats support to virtio-blk data-plane and protects
> query-blockstats from races with the dataplane IOThread.
>
> Stefan Hajnoczi (3):
>   dataplane: add bdrv_acct_*() accounting
>   block: make bdrv_query_stats() static
>   block: acquire AioContext is qmp_query_blockstats()
>
>  block/qapi.c                    |  6 +++++-
>  hw/block/dataplane/virtio-blk.c | 10 ++++++++++
>  include/block/qapi.h            |  1 -
>  3 files changed, 15 insertions(+), 2 deletions(-)
>

ACK to patches 2 and 3.

Regarding patch 1 it's nice that the change is so trivial so I'm not 
objecting to the patch.

However, Fam's patches for VirtIOBlockReq{,uest} unification provide 
another way to achieve this.  If we could drop do_rdwr_cmd in favor of 
virtio_blk_handle_read and virtio_blk_handle_write, we would get for 
free both blockstats and rerror/werror support.  What do you think?

Paolo

Re: [Qemu-devel] [PATCH 0/3] dataplane: add query-blockstats support

[Stefan Hajnoczi]

On Tue, Jun 10, 2014 at 5:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/06/2014 09:29, Stefan Hajnoczi ha scritto:
>
>> This series adds blockstats support to virtio-blk data-plane and protects
>> query-blockstats from races with the dataplane IOThread.
>>
>> Stefan Hajnoczi (3):
>>   dataplane: add bdrv_acct_*() accounting
>>   block: make bdrv_query_stats() static
>>   block: acquire AioContext is qmp_query_blockstats()
>>
>>  block/qapi.c                    |  6 +++++-
>>  hw/block/dataplane/virtio-blk.c | 10 ++++++++++
>>  include/block/qapi.h            |  1 -
>>  3 files changed, 15 insertions(+), 2 deletions(-)
>>
>
> ACK to patches 2 and 3.
>
> Regarding patch 1 it's nice that the change is so trivial so I'm not
> objecting to the patch.
>
> However, Fam's patches for VirtIOBlockReq{,uest} unification provide another
> way to achieve this.  If we could drop do_rdwr_cmd in favor of
> virtio_blk_handle_read and virtio_blk_handle_write, we would get for free
> both blockstats and rerror/werror support.  What do you think?

Fam has sent out equivalent patches to my series that are based on the
unified dataplane/non-dataplane virtio code.

NACK, let's use Fam's patches instead.

Stefan