[Qemu-devel] [PATCH v2 0/4] block: Allow query stats for drive-mirror target

[Qemu-devel] [PATCH v2 0/4] block: Allow query stats for drive-mirror target

[Fam Zheng]

v2: Add Max's and Eric's rev-by's for patch 1~3.
    Fix spelling, documentation and indentation on 4.
    Thanks, Max and Eric!

This series adds an optional bool parameter "query-nodes" to query-blockstats.

By default, if omitted, the behavior is unchanged.

If set to "true", the command will iterate through all named nodes in BDS graph
and report the statistics in a list, similarly. But the backing chain is not
built.

This provides a way for libvirt to watch the allocation status
(wr_highest_offset) of target image. Now, libvirt can start drive-mirror job
specifying a node-name parameter. Thus the created target image gets a node
name that can be queried with the new query-blockstats.


Fam Zheng (4):
  block: Add bdrv_next_node
  block: Add bdrv_get_node_name
  block: Include "node-name" if present in query-blockstats
  qmp: Add optional switch "query-nodes" in query-blockstats

 block.c               | 13 +++++++++++++
 block/qapi.c          | 25 ++++++++++++++++++-------
 hmp.c                 |  2 +-
 include/block/block.h |  2 ++
 qapi/block-core.json  | 16 ++++++++++++++--
 qmp-commands.hx       |  2 +-
 6 files changed, 49 insertions(+), 11 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 1/4] block: Add bdrv_next_node

[Fam Zheng]

Similar to bdrv_next, this traverses through graph_bdrv_states. Will be
useful to enumerate all the named nodes.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               | 8 ++++++++
 include/block/block.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/block.c b/block.c
index 88f6d9b..c92a913 100644
--- a/block.c
+++ b/block.c
@@ -3775,6 +3775,14 @@ bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
     return top != NULL;
 }
 
+BlockDriverState *bdrv_next_node(BlockDriverState *bs)
+{
+    if (!bs) {
+        return QTAILQ_FIRST(&graph_bdrv_states);
+    }
+    return QTAILQ_NEXT(bs, node_list);
+}
+
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
     if (!bs) {
diff --git a/include/block/block.h b/include/block/block.h
index 341054d..25acd81 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -365,6 +365,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
                                  const char *node_name,
                                  Error **errp);
 bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
+BlockDriverState *bdrv_next_node(BlockDriverState *bs);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
 int bdrv_is_encrypted(BlockDriverState *bs);
 int bdrv_key_required(BlockDriverState *bs);
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/4] block: Add bdrv_get_node_name

[Fam Zheng]

This returns the node name of a BDS. Remove the TODO comment and expect
the callers to be explicit.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               | 5 +++++
 include/block/block.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block.c b/block.c
index c92a913..479b045 100644
--- a/block.c
+++ b/block.c
@@ -3791,6 +3791,11 @@ BlockDriverState *bdrv_next(BlockDriverState *bs)
     return QTAILQ_NEXT(bs, device_list);
 }
 
+const char *bdrv_get_node_name(const BlockDriverState *bs)
+{
+    return bs->node_name;
+}
+
 /* TODO check what callers really want: bs->node_name or blk_name() */
 const char *bdrv_get_device_name(const BlockDriverState *bs)
 {
diff --git a/include/block/block.h b/include/block/block.h
index 25acd81..71fff2c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -373,6 +373,7 @@ int bdrv_set_key(BlockDriverState *bs, const char *key);
 int bdrv_query_missing_keys(void);
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
                          void *opaque);
+const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
 int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-- 
1.9.3

[Qemu-devel] [PATCH v2 3/4] block: Include "node-name" if present in query-blockstats

[Fam Zheng]

Node name is a better identifier of BDS.

We will want to query statistics of a BDS node buried in the BDS graph,
so reporting the node's name if there is one will do the trick.

Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/qapi.c         | 5 +++++
 qapi/block-core.json | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/block/qapi.c b/block/qapi.c
index 1301144..a4d1a20 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -333,6 +333,11 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
         s->device = g_strdup(bdrv_get_device_name(bs));
     }
 
+    if (bdrv_get_node_name(bs)[0]) {
+        s->has_node_name = true;
+        s->node_name = g_strdup(bdrv_get_node_name(bs));
+    }
+
     s->stats = g_malloc0(sizeof(*s->stats));
     s->stats->rd_bytes = bs->stats.nr_bytes[BLOCK_ACCT_READ];
     s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE];
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f7089e..6b4b96c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -408,6 +408,8 @@
 # @device: #optional If the stats are for a virtual block device, the name
 #          corresponding to the virtual block device.
 #
+# @device: #optional The node name of the device. (Since 2.3)
+#
 # @stats:  A @BlockDeviceStats for the device.
 #
 # @parent: #optional This describes the file block device if it has one.
@@ -418,7 +420,8 @@
 # Since: 0.14.0
 ##
 { 'type': 'BlockStats',
-  'data': {'*device': 'str', 'stats': 'BlockDeviceStats',
+  'data': {'*device': 'str', '*node-name': 'str',
+           'stats': 'BlockDeviceStats',
            '*parent': 'BlockStats',
            '*backing': 'BlockStats'} }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 4/4] qmp: Add optional switch "query-nodes" in query-blockstats

[Fam Zheng]

This bool option will allow query all the node names. It iterates all
the BDSes that are assigned a name, also in this case don't query up the
backing chain.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/qapi.c         | 20 +++++++++++++-------
 hmp.c                |  2 +-
 qapi/block-core.json | 11 ++++++++++-
 qmp-commands.hx      |  2 +-
 4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a4d1a20..a0a50b9 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -322,7 +322,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     qapi_free_BlockInfo(info);
 }
 
-static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
+static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
+                                    bool query_backing)
 {
     BlockStats *s;
 
@@ -352,12 +353,12 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
 
     if (bs->file) {
         s->has_parent = true;
-        s->parent = bdrv_query_stats(bs->file);
+        s->parent = bdrv_query_stats(bs->file, query_backing);
     }
 
-    if (bs->backing_hd) {
+    if (query_backing && bs->backing_hd) {
         s->has_backing = true;
-        s->backing = bdrv_query_stats(bs->backing_hd);
+        s->backing = bdrv_query_stats(bs->backing_hd, query_backing);
     }
 
     return s;
@@ -388,17 +389,22 @@ BlockInfoList *qmp_query_block(Error **errp)
     return NULL;
 }
 
-BlockStatsList *qmp_query_blockstats(Error **errp)
+BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
+                                     bool query_nodes,
+                                     Error **errp)
 {
     BlockStatsList *head = NULL, **p_next = &head;
     BlockDriverState *bs = NULL;
 
-     while ((bs = bdrv_next(bs))) {
+    /* Just to be safe if query_nodes is not always initialized */
+    query_nodes = has_query_nodes && query_nodes;
+
+    while ((bs = query_nodes ? bdrv_next_node(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);
+        info->value = bdrv_query_stats(bs, !query_nodes);
         aio_context_release(ctx);
 
         *p_next = info;
diff --git a/hmp.c b/hmp.c
index 63d7686..94b27df 100644
--- a/hmp.c
+++ b/hmp.c
@@ -403,7 +403,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
 {
     BlockStatsList *stats_list, *stats;
 
-    stats_list = qmp_query_blockstats(NULL);
+    stats_list = qmp_query_blockstats(false, false, NULL);
 
     for (stats = stats_list; stats; stats = stats->next) {
         if (!stats->value->has_device) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6b4b96c..db4e8e9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -430,11 +430,20 @@
 #
 # Query the @BlockStats for all virtual block devices.
 #
+# @query-nodes: #optional If true, the command will query all the block nodes
+#               that have a node name, in a list which will include "parent"
+#               information, but not "backing".
+#               If false or omitted, the behavior is as before - query all the
+#               device backends, recursively including their "parent" and #
+#               "backing". (Since 2.3)
+#
 # Returns: A list of @BlockStats for each virtual block devices.
 #
 # Since: 0.14.0
 ##
-{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
+{ 'command': 'query-blockstats',
+  'data': { '*query-nodes': 'bool' },
+  'returns': ['BlockStats'] }
 
 ##
 # @BlockdevOnError:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1abd619..44bfb9e 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2347,7 +2347,7 @@ EQMP
 
     {
         .name       = "query-blockstats",
-        .args_type  = "",
+        .args_type  = "query-nodes:b?",
         .mhandler.cmd_new = qmp_marshal_input_query_blockstats,
     },
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/4] block: Add bdrv_get_node_name

[Max Reitz]

On 2014-10-31 at 04:32, Fam Zheng wrote:
> This returns the node name of a BDS. Remove the TODO comment and expect
> the callers to be explicit.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block.c               | 5 +++++
>   include/block/block.h | 1 +
>   2 files changed, 6 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 4/4] qmp: Add optional switch "query-nodes" in query-blockstats

[Max Reitz]

On 2014-10-31 at 04:32, Fam Zheng wrote:
> This bool option will allow query all the node names. It iterates all
> the BDSes that are assigned a name, also in this case don't query up the
> backing chain.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/qapi.c         | 20 +++++++++++++-------
>   hmp.c                |  2 +-
>   qapi/block-core.json | 11 ++++++++++-
>   qmp-commands.hx      |  2 +-
>   4 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a4d1a20..a0a50b9 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -322,7 +322,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
>       qapi_free_BlockInfo(info);
>   }
>   
> -static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
> +static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
> +                                    bool query_backing)
>   {
>       BlockStats *s;
>   
> @@ -352,12 +353,12 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
>   
>       if (bs->file) {
>           s->has_parent = true;
> -        s->parent = bdrv_query_stats(bs->file);
> +        s->parent = bdrv_query_stats(bs->file, query_backing);
>       }
>   
> -    if (bs->backing_hd) {
> +    if (query_backing && bs->backing_hd) {
>           s->has_backing = true;
> -        s->backing = bdrv_query_stats(bs->backing_hd);
> +        s->backing = bdrv_query_stats(bs->backing_hd, query_backing);
>       }
>   
>       return s;
> @@ -388,17 +389,22 @@ BlockInfoList *qmp_query_block(Error **errp)
>       return NULL;
>   }
>   
> -BlockStatsList *qmp_query_blockstats(Error **errp)
> +BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
> +                                     bool query_nodes,
> +                                     Error **errp)
>   {
>       BlockStatsList *head = NULL, **p_next = &head;
>       BlockDriverState *bs = NULL;
>   
> -     while ((bs = bdrv_next(bs))) {
> +    /* Just to be safe if query_nodes is not always initialized */
> +    query_nodes = has_query_nodes && query_nodes;
> +
> +    while ((bs = query_nodes ? bdrv_next_node(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);
> +        info->value = bdrv_query_stats(bs, !query_nodes);
>           aio_context_release(ctx);
>   
>           *p_next = info;
> diff --git a/hmp.c b/hmp.c
> index 63d7686..94b27df 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -403,7 +403,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>   {
>       BlockStatsList *stats_list, *stats;
>   
> -    stats_list = qmp_query_blockstats(NULL);
> +    stats_list = qmp_query_blockstats(false, false, NULL);
>   
>       for (stats = stats_list; stats; stats = stats->next) {
>           if (!stats->value->has_device) {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6b4b96c..db4e8e9 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -430,11 +430,20 @@
>   #
>   # Query the @BlockStats for all virtual block devices.
>   #
> +# @query-nodes: #optional If true, the command will query all the block nodes
> +#               that have a node name, in a list which will include "parent"
> +#               information, but not "backing".
> +#               If false or omitted, the behavior is as before - query all the
> +#               device backends, recursively including their "parent" and #

Is there a reason for the # at the end of the line?

Fixable by a maintainer, though.

> +#               "backing". (Since 2.3)
> +#
>   # Returns: A list of @BlockStats for each virtual block devices.
>   #
>   # Since: 0.14.0
>   ##
> -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> +{ 'command': 'query-blockstats',
> +  'data': { '*query-nodes': 'bool' },
> +  'returns': ['BlockStats'] }
>   
>   ##
>   # @BlockdevOnError:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1abd619..44bfb9e 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2347,7 +2347,7 @@ EQMP
>   
>       {
>           .name       = "query-blockstats",
> -        .args_type  = "",
> +        .args_type  = "query-nodes:b?",
>           .mhandler.cmd_new = qmp_marshal_input_query_blockstats,
>       },
>   

As written in the reply I forgot to give to v1, I'm still concerned some 
nodes may appear twice (named protocol BDS) and maybe we don't want 
that. But it should not hurt.

With the # removed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/4] block: Allow query stats for drive-mirror target

[Max Reitz]

On 2014-10-31 at 04:32, Fam Zheng wrote:
> v2: Add Max's and Eric's rev-by's for patch 1~3.
>      Fix spelling, documentation and indentation on 4.
>      Thanks, Max and Eric!
>
> This series adds an optional bool parameter "query-nodes" to query-blockstats.
>
> By default, if omitted, the behavior is unchanged.
>
> If set to "true", the command will iterate through all named nodes in BDS graph
> and report the statistics in a list, similarly. But the backing chain is not
> built.
>
> This provides a way for libvirt to watch the allocation status
> (wr_highest_offset) of target image. Now, libvirt can start drive-mirror job
> specifying a node-name parameter. Thus the created target image gets a node
> name that can be queried with the new query-blockstats.
>
>
> Fam Zheng (4):
>    block: Add bdrv_next_node
>    block: Add bdrv_get_node_name
>    block: Include "node-name" if present in query-blockstats
>    qmp: Add optional switch "query-nodes" in query-blockstats
>
>   block.c               | 13 +++++++++++++
>   block/qapi.c          | 25 ++++++++++++++++++-------
>   hmp.c                 |  2 +-
>   include/block/block.h |  2 ++
>   qapi/block-core.json  | 16 ++++++++++++++--
>   qmp-commands.hx       |  2 +-
>   6 files changed, 49 insertions(+), 11 deletions(-)

As I seem to be the supporting submaintainer of the day:

Thanks, applied to my block-next tree with the / #$/ in patch 4 removed:

https://github.com/XanClic/qemu/commits/block-next

Re: [Qemu-devel] [PATCH v2 4/4] qmp: Add optional switch "query-nodes" in query-blockstats

[Fam Zheng]

On Tue, 11/04 09:54, Max Reitz wrote:
> On 2014-10-31 at 04:32, Fam Zheng wrote:
> >This bool option will allow query all the node names. It iterates all
> >the BDSes that are assigned a name, also in this case don't query up the
> >backing chain.
> >
> >Signed-off-by: Fam Zheng <famz@redhat.com>
> >---
> >  block/qapi.c         | 20 +++++++++++++-------
> >  hmp.c                |  2 +-
> >  qapi/block-core.json | 11 ++++++++++-
> >  qmp-commands.hx      |  2 +-
> >  4 files changed, 25 insertions(+), 10 deletions(-)
> >
> >diff --git a/block/qapi.c b/block/qapi.c
> >index a4d1a20..a0a50b9 100644
> >--- a/block/qapi.c
> >+++ b/block/qapi.c
> >@@ -322,7 +322,8 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
> >      qapi_free_BlockInfo(info);
> >  }
> >-static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
> >+static BlockStats *bdrv_query_stats(const BlockDriverState *bs,
> >+                                    bool query_backing)
> >  {
> >      BlockStats *s;
> >@@ -352,12 +353,12 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
> >      if (bs->file) {
> >          s->has_parent = true;
> >-        s->parent = bdrv_query_stats(bs->file);
> >+        s->parent = bdrv_query_stats(bs->file, query_backing);
> >      }
> >-    if (bs->backing_hd) {
> >+    if (query_backing && bs->backing_hd) {
> >          s->has_backing = true;
> >-        s->backing = bdrv_query_stats(bs->backing_hd);
> >+        s->backing = bdrv_query_stats(bs->backing_hd, query_backing);
> >      }
> >      return s;
> >@@ -388,17 +389,22 @@ BlockInfoList *qmp_query_block(Error **errp)
> >      return NULL;
> >  }
> >-BlockStatsList *qmp_query_blockstats(Error **errp)
> >+BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
> >+                                     bool query_nodes,
> >+                                     Error **errp)
> >  {
> >      BlockStatsList *head = NULL, **p_next = &head;
> >      BlockDriverState *bs = NULL;
> >-     while ((bs = bdrv_next(bs))) {
> >+    /* Just to be safe if query_nodes is not always initialized */
> >+    query_nodes = has_query_nodes && query_nodes;
> >+
> >+    while ((bs = query_nodes ? bdrv_next_node(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);
> >+        info->value = bdrv_query_stats(bs, !query_nodes);
> >          aio_context_release(ctx);
> >          *p_next = info;
> >diff --git a/hmp.c b/hmp.c
> >index 63d7686..94b27df 100644
> >--- a/hmp.c
> >+++ b/hmp.c
> >@@ -403,7 +403,7 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
> >  {
> >      BlockStatsList *stats_list, *stats;
> >-    stats_list = qmp_query_blockstats(NULL);
> >+    stats_list = qmp_query_blockstats(false, false, NULL);
> >      for (stats = stats_list; stats; stats = stats->next) {
> >          if (!stats->value->has_device) {
> >diff --git a/qapi/block-core.json b/qapi/block-core.json
> >index 6b4b96c..db4e8e9 100644
> >--- a/qapi/block-core.json
> >+++ b/qapi/block-core.json
> >@@ -430,11 +430,20 @@
> >  #
> >  # Query the @BlockStats for all virtual block devices.
> >  #
> >+# @query-nodes: #optional If true, the command will query all the block nodes
> >+#               that have a node name, in a list which will include "parent"
> >+#               information, but not "backing".
> >+#               If false or omitted, the behavior is as before - query all the
> >+#               device backends, recursively including their "parent" and #
> 
> Is there a reason for the # at the end of the line?

Badd vim "J".

Thanks for reviewing!

Fam