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

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

[Fam Zheng]

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               | 14 +++++++++++++-
 block/qapi.c          | 25 ++++++++++++++++++-------
 hmp.c                 |  2 +-
 include/block/block.h |  2 ++
 qapi/block-core.json  |  9 +++++++--
 qmp-commands.hx       |  2 +-
 6 files changed, 42 insertions(+), 12 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 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>
---
 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

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

[Max Reitz]

On 2014-10-29 at 06:04, Fam Zheng wrote:
> 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>
> ---
>   block.c               | 8 ++++++++
>   include/block/block.h | 1 +
>   2 files changed, 9 insertions(+)

You could reuse this for bdrv_named_nodes_list(), but that's optional.

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

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

[Eric Blake]

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

On 10/28/2014 11:04 PM, Fam Zheng wrote:
> 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>
> ---
>  block.c               | 8 ++++++++
>  include/block/block.h | 1 +
>  2 files changed, 9 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH 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>
---
 block.c               | 6 +++++-
 include/block/block.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index c92a913..1448e38 100644
--- a/block.c
+++ b/block.c
@@ -3791,7 +3791,11 @@ BlockDriverState *bdrv_next(BlockDriverState *bs)
     return QTAILQ_NEXT(bs, device_list);
 }
 
-/* TODO check what callers really want: bs->node_name or blk_name() */
+const char *bdrv_get_node_name(const BlockDriverState *bs)
+{
+    return bs->node_name;
+}
+
 const char *bdrv_get_device_name(const BlockDriverState *bs)
 {
     return bs->blk ? blk_name(bs->blk) : "";
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

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

[Max Reitz]

On 2014-10-29 at 06:04, 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>
> ---
>   block.c               | 6 +++++-
>   include/block/block.h | 1 +
>   2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index c92a913..1448e38 100644
> --- a/block.c
> +++ b/block.c
> @@ -3791,7 +3791,11 @@ BlockDriverState *bdrv_next(BlockDriverState *bs)
>       return QTAILQ_NEXT(bs, device_list);
>   }
>   
> -/* TODO check what callers really want: bs->node_name or blk_name() */

Well, we still need to check this, don't we? cscope tells me there are 
61 callers of bdrv_get_device_name() and not all of them look like they 
always query a BDS with a BB.

Max

> +const char *bdrv_get_node_name(const BlockDriverState *bs)
> +{
> +    return bs->node_name;
> +}
> +
>   const char *bdrv_get_device_name(const BlockDriverState *bs)
>   {
>       return bs->blk ? blk_name(bs->blk) : "";
> 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,

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

[Eric Blake]

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

On 10/28/2014 11:04 PM, 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>
> ---
>  block.c               | 6 +++++-
>  include/block/block.h | 1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 

Can still return NULL because we don't have Jeff's patches for
auto-assigning a node name everywhere, right?  But that doesn't matter
in the context of this patch, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH 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 good.

Signed-off-by: Fam Zheng <famz@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..c4beb17 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

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

[Max Reitz]

On 2014-10-29 at 06:04, Fam Zheng wrote:
> 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 good.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block/qapi.c         | 5 +++++
>   qapi/block-core.json | 5 ++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)

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

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

[Eric Blake]

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

On 10/28/2014 11:04 PM, Fam Zheng wrote:
> 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 good.

s/do the good/do the trick/

> +++ 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)

Yeah, we missed soft freeze and this is a feature, so your versioning is
correct.

The node-name would become non-optional once Jeff's patches auto-assign
a node name, right?

> +#
>  # @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',

Indentation looks odd here.  Fix that, and:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH 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 |  4 +++-
 qmp-commands.hx      |  2 +-
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a4d1a20..e26033e 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 intialized */
+    query_nodes = query_nodes && has_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 c4beb17..a662137 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -434,7 +434,9 @@
 #
 # 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 4/4] qmp: Add optional switch "query-nodes" in query-blockstats

[Max Reitz]

On 2014-10-29 at 06:04, 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 |  4 +++-
>   qmp-commands.hx      |  2 +-
>   4 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a4d1a20..e26033e 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);
>       }

Is there a specific reason why you're not querying the backing chain but 
still recurse to bs->file?

>       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 intialized */
> +    query_nodes = query_nodes && has_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 c4beb17..a662137 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -434,7 +434,9 @@
>   #
>   # Since: 0.14.0
>   ##
> -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> +{ 'command': 'query-blockstats',
> +  'data': {'*query-nodes': 'bool' },

No space after the opening shwoopy bracket (took that from Wikipedia, I 
love it), but a space before the closing one.

Also, the new parameter should be documented, I think (and the 
documentation should include that the backing chain is not queried 
recursively).

Max

> +  '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,
>       },
>   

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

[Fam Zheng]

On Wed, 10/29 10:11, Max Reitz wrote:
> On 2014-10-29 at 06:04, 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 |  4 +++-
> >  qmp-commands.hx      |  2 +-
> >  4 files changed, 18 insertions(+), 10 deletions(-)
> >
> >diff --git a/block/qapi.c b/block/qapi.c
> >index a4d1a20..e26033e 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);
> >      }
> 
> Is there a specific reason why you're not querying the backing chain but
> still recurse to bs->file?

Unlike its backing, there can be some information in ->file which is
interesting to the node itself, because it is ->file carries out the actual
read/write on the image. Makes sense?

Fam

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

[Max Reitz]

On 2014-10-31 at 04:20, Fam Zheng wrote:
> On Wed, 10/29 10:11, Max Reitz wrote:
>> On 2014-10-29 at 06:04, 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 |  4 +++-
>>>   qmp-commands.hx      |  2 +-
>>>   4 files changed, 18 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/block/qapi.c b/block/qapi.c
>>> index a4d1a20..e26033e 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);
>>>       }
>> Is there a specific reason why you're not querying the backing chain but
>> still recurse to bs->file?
> Unlike its backing, there can be some information in ->file which is
> interesting to the node itself, because it is ->file carries out the actual
> read/write on the image. Makes sense?

Ah, I forgot to answer. Well, ->backing carries out reads, too. I 
thought you omitted it because you could give it a node-name as well and 
therefore you'd have some BDS appear twice; but the same applies  to 
->file, that's why I asked.

Max

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

[Eric Blake]

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

On 10/28/2014 11:04 PM, 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 |  4 +++-
>  qmp-commands.hx      |  2 +-
>  4 files changed, 18 insertions(+), 10 deletions(-)
> 

> -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 intialized */

s/intialized/initialized/

> +    query_nodes = query_nodes && has_query_nodes;

If things aren't initialized (was true a while ago, but we recently
fixed that to ensure 0 initialization, although no one yet really relies
on the guarantee), then valgrind could complain of a branch on an uninit
memory location.  Idiomatically, this is usually written:

query_nodes = has_query_nodes && query_nodes;

to pacify valgrind if we hadn't zero-initialized; although logically,
the result is identical, so I don't care if you leave it.

> +++ b/qapi/block-core.json
> @@ -434,7 +434,9 @@
>  #
>  # Since: 0.14.0
>  ##
> -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> +{ 'command': 'query-blockstats',
> +  'data': {'*query-nodes': 'bool' },
> +  'returns': ['BlockStats'] }

Max correctly pointed out that this is missing documentation.

The idea looks sane; it will visit all named nodes (whether or not those
nodes are also reachable from named devices), and omit any unnamed nodes
(right now, libvirt would have to be taught to name nodes, or Jeff's
patch to auto-name nodes will avoid that problem).

Hmm, I wonder - if we are adding an optional parameter that controls
what to iterate over, should we also add an optional parameter that says
to limit the output to a given input name?  Then again, we don't have
very many existing query-* commands that filter, and at any rate, adding
such a filter should be its own patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Fam Zheng]

On Thu, 10/30 15:47, Eric Blake wrote:
> On 10/28/2014 11:04 PM, 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 |  4 +++-
> >  qmp-commands.hx      |  2 +-
> >  4 files changed, 18 insertions(+), 10 deletions(-)
> > 
> 
> > -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 intialized */
> 
> s/intialized/initialized/
> 
> > +    query_nodes = query_nodes && has_query_nodes;
> 
> If things aren't initialized (was true a while ago, but we recently
> fixed that to ensure 0 initialization, although no one yet really relies
> on the guarantee), then valgrind could complain of a branch on an uninit
> memory location.  Idiomatically, this is usually written:
> 
> query_nodes = has_query_nodes && query_nodes;

This does read better. Will fix.

> 
> to pacify valgrind if we hadn't zero-initialized; although logically,
> the result is identical, so I don't care if you leave it.
> 
> > +++ b/qapi/block-core.json
> > @@ -434,7 +434,9 @@
> >  #
> >  # Since: 0.14.0
> >  ##
> > -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> > +{ 'command': 'query-blockstats',
> > +  'data': {'*query-nodes': 'bool' },
> > +  'returns': ['BlockStats'] }
> 
> Max correctly pointed out that this is missing documentation.
> 
> The idea looks sane; it will visit all named nodes (whether or not those
> nodes are also reachable from named devices), and omit any unnamed nodes
> (right now, libvirt would have to be taught to name nodes, or Jeff's
> patch to auto-name nodes will avoid that problem).
> 
> Hmm, I wonder - if we are adding an optional parameter that controls
> what to iterate over, should we also add an optional parameter that says
> to limit the output to a given input name?  Then again, we don't have
> very many existing query-* commands that filter, and at any rate, adding
> such a filter should be its own patch.

Or separate series :)

Thanks,
Fam

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

[Eric Blake]

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

On 10/28/2014 11:04 PM, Fam Zheng wrote:
> 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.

Well, only if libvirt actually starts naming the node at the destination
of a drive-mirror job (doesn't happen now, but this patch shifts the
blame from "qemu doesn't provide access to the information" over to
"libvirt hasn't yet been coded to interact with what qemu provides"), so
this is a step in the right direction.  The idea makes sense to me.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]