[Qemu-devel] [PATCH V5 0/4] Show backing file depth in HMP and QMP

[Qemu-devel] [PATCH V5 0/4] Show backing file depth in HMP and QMP

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

In some setups many backing files and snapshot are chained.
This lead to the formation of huge trees of snapshots all depending
on a common ancestor.

Hence if something bad happen to this common ancestor all the snapshot
of the tree will be broken.

This patch add an easy way for the user to monitor backing file depth
and take the good decision (streaming).

in v2:

lcapitulino: -Fix typo in qapi-schema.json
             -rename *file_ancestors_count to
              *backing_file_ancestor_count

in v3:

kwolf: qapi-schema.json backing_file_ancestor_count field not optional
       Add a (since 1.2) comment
       keep display in HMP optional

in v4:

eblake: rename backing_file.ancestors.count to backing.file.depth
        in qapi-schema.json use - instead of _
        Fix json docstring

in v5:

eblake: ditch completely the word ancestor in comment
        Add the corresponding patch to qmp-commands.hx

Benoît Canet (4):
  block: create bdrv_get_backing_file_depth()
  block: Use bdrv_get_backing_file_depth()
  qmp: Add backing-file-depth to qmp-commands.hx
  hmp: show the backing file depth

 block.c          |   16 ++++++++++++++++
 block.h          |    1 +
 hmp.c            |    2 ++
 qapi-schema.json |    9 ++++++---
 qmp-commands.hx  |    2 ++
 5 files changed, 27 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH V5 1/4] block: create bdrv_get_backing_file_depth()

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

Create bdrv_get_backing_file_depth() in order to be able to show
in QMP and HMP how many ancestors backing an image a block device
have.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c |   13 +++++++++++++
 block.h |    1 +
 2 files changed, 14 insertions(+)

diff --git a/block.c b/block.c
index ce7eb8f..22ebe49 100644
--- a/block.c
+++ b/block.c
@@ -2754,6 +2754,19 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     return NULL;
 }
 
+int bdrv_get_backing_file_depth(BlockDriverState *bs)
+{
+    if (!bs->drv) {
+        return 0;
+    }
+
+    if (!bs->backing_hd) {
+        return 0;
+    }
+
+    return 1 + bdrv_get_backing_file_depth(bs->backing_hd);
+}
+
 #define NB_SUFFIXES 4
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size)
diff --git a/block.h b/block.h
index c89590d..650d872 100644
--- a/block.h
+++ b/block.h
@@ -174,6 +174,7 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
                                             int nb_sectors, int *pnum);
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
+int bdrv_get_backing_file_depth(BlockDriverState *bs);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
-- 
1.7.9.5

[Qemu-devel] [PATCH V5 2/4] block: Use bdrv_get_backing_file_depth()

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

Use the dedicated counting function in qmp_query_block in order to
propagate the backing file depth to HMP.

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 block.c          |    3 +++
 qapi-schema.json |    9 ++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 22ebe49..a6fba1d 100644
--- a/block.c
+++ b/block.c
@@ -2450,6 +2450,9 @@ BlockInfoList *qmp_query_block(Error **errp)
                 info->value->inserted->backing_file = g_strdup(bs->backing_file);
             }
 
+            info->value->inserted->backing_file_depth =
+                bdrv_get_backing_file_depth(bs);
+
             if (bs->io_limits_enabled) {
                 info->value->inserted->bps =
                                bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..2b50f22 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -398,6 +398,8 @@
 #
 # @backing_file: #optional the name of the backing file (for copy-on-write)
 #
+# @backing_file_depth: number of files in the backing file chain (since: 1.2)
+#
 # @encrypted: true if the backing device is encrypted
 #
 # @bps: total throughput limit in bytes per second is specified
@@ -418,9 +420,10 @@
 ##
 { 'type': 'BlockDeviceInfo',
   'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
-            '*backing_file': 'str', 'encrypted': 'bool',
-            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
-            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
+            '*backing_file': 'str', 'backing-file-depth': 'int',
+            'encrypted': 'bool', 'bps': 'int', 'bps_rd': 'int',
+            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int',
+            'iops_wr': 'int'} }
 
 ##
 # @BlockDeviceIoStatus:
-- 
1.7.9.5

[Qemu-devel] [PATCH V5 3/4] qmp: Add backing-file-depth to qmp-commands.hx

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 qmp-commands.hx |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/qmp-commands.hx b/qmp-commands.hx
index e3cf3c5..5de3adf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1317,6 +1317,7 @@ Each json-object contain the following:
                                 "nbd", "parallels", "qcow", "qcow2", "raw",
                                 "tftp", "vdi", "vmdk", "vpc", "vvfat"
          - "backing_file": backing file name (json-string, optional)
+         - "backing-file-depth": number of files in the backing file chain (json-int)
          - "encrypted": true if encrypted, false otherwise (json-bool)
          - "bps": limit total bytes per second (json-int)
          - "bps_rd": limit read bytes per second (json-int)
@@ -1345,6 +1346,7 @@ Example:
                "drv":"qcow2",
                "encrypted":false,
                "file":"disks/test.img",
+               "backing-file-depth":0,
                "bps":1000000,
                "bps_rd":0,
                "bps_wr":0,
-- 
1.7.9.5

[Qemu-devel] [PATCH V5 4/4] hmp: show the backing file depth

[benoit.canet]

From: Benoît Canet <benoit@irqsave.net>

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 hmp.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/hmp.c b/hmp.c
index 6b72a64..25688ab 100644
--- a/hmp.c
+++ b/hmp.c
@@ -227,6 +227,8 @@ void hmp_info_block(Monitor *mon)
             if (info->value->inserted->has_backing_file) {
                 monitor_printf(mon, " backing_file=");
                 monitor_print_filename(mon, info->value->inserted->backing_file);
+                monitor_printf(mon, " backing_file_depth=%" PRId64,
+                    info->value->inserted->backing_file_depth);
             }
             monitor_printf(mon, " ro=%d drv=%s encrypted=%d",
                            info->value->inserted->ro,
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH V5 0/4] Show backing file depth in HMP and QMP

[Eric Blake]

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

On 07/26/2012 07:18 AM, benoit.canet@gmail.com wrote:
> From: Benoît Canet <benoit@irqsave.net>
> 
> In some setups many backing files and snapshot are chained.
> This lead to the formation of huge trees of snapshots all depending
> on a common ancestor.
> 
> Hence if something bad happen to this common ancestor all the snapshot
> of the tree will be broken.
> 
> This patch add an easy way for the user to monitor backing file depth
> and take the good decision (streaming).
> 
> 
> in v5:
> 
> eblake: ditch completely the word ancestor in comment
>         Add the corresponding patch to qmp-commands.hx

Series:
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: 620 bytes --]

Re: [Qemu-devel] [PATCH V5 2/4] block: Use bdrv_get_backing_file_depth()

[Luiz Capitulino]

On Thu, 26 Jul 2012 15:18:19 +0200
benoit.canet@gmail.com wrote:

> From: Benoît Canet <benoit@irqsave.net>
> 
> Use the dedicated counting function in qmp_query_block in order to
> propagate the backing file depth to HMP.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c          |    3 +++
>  qapi-schema.json |    9 ++++++---
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 22ebe49..a6fba1d 100644
> --- a/block.c
> +++ b/block.c
> @@ -2450,6 +2450,9 @@ BlockInfoList *qmp_query_block(Error **errp)
>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
>              }
>  
> +            info->value->inserted->backing_file_depth =
> +                bdrv_get_backing_file_depth(bs);
> +
>              if (bs->io_limits_enabled) {
>                  info->value->inserted->bps =
>                                 bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL];
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..2b50f22 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -398,6 +398,8 @@
>  #
>  # @backing_file: #optional the name of the backing file (for copy-on-write)
>  #
> +# @backing_file_depth: number of files in the backing file chain (since: 1.2)
> +#
>  # @encrypted: true if the backing device is encrypted
>  #
>  # @bps: total throughput limit in bytes per second is specified
> @@ -418,9 +420,10 @@
>  ##
>  { 'type': 'BlockDeviceInfo',
>    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> -            '*backing_file': 'str', 'encrypted': 'bool',
> -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
> +            '*backing_file': 'str', 'backing-file-depth': 'int',

Should use underscores, ie. should be backing_file_depth.

Also, the next patch and this can be squashed together.

Otherwise series looks good to me now.


> +            'encrypted': 'bool', 'bps': 'int', 'bps_rd': 'int',
> +            'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int',
> +            'iops_wr': 'int'} }
>  
>  ##
>  # @BlockDeviceIoStatus:

Re: [Qemu-devel] [PATCH V5 2/4] block: Use bdrv_get_backing_file_depth()

[Eric Blake]

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

On 07/30/2012 10:32 AM, Luiz Capitulino wrote:
> On Thu, 26 Jul 2012 15:18:19 +0200
> benoit.canet@gmail.com wrote:
> 
>> From: Benoît Canet <benoit@irqsave.net>
>>
>> Use the dedicated counting function in qmp_query_block in order to
>> propagate the backing file depth to HMP.
>>
>> Signed-off-by: Benoit Canet <benoit@irqsave.net>

>> +++ b/qapi-schema.json
>> @@ -398,6 +398,8 @@
>>  #
>>  # @backing_file: #optional the name of the backing file (for copy-on-write)
>>  #
>> +# @backing_file_depth: number of files in the backing file chain (since: 1.2)
>> +#
>>  # @encrypted: true if the backing device is encrypted
>>  #
>>  # @bps: total throughput limit in bytes per second is specified
>> @@ -418,9 +420,10 @@
>>  ##
>>  { 'type': 'BlockDeviceInfo',
>>    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
>> -            '*backing_file': 'str', 'encrypted': 'bool',
>> -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>> -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
>> +            '*backing_file': 'str', 'backing-file-depth': 'int',
> 
> Should use underscores, ie. should be backing_file_depth.

Really?  I thought we _want_ new interfaces to use '-', not '_', in QMP.
 For 'backing_file', we are stuck due to back-compat (unless Anthony's
proposed patch to parse names case-insensitively with '-' and '_' folded
together is taken), but for the new field, we have no back-compat
constraints, and I would prefer backing-file-depth.

At any rate, you definitely need to make sure you agree between the docs
above and the JSON statement below (as written, you have both spellings
in the same 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: 620 bytes --]

Re: [Qemu-devel] [PATCH V5 2/4] block: Use bdrv_get_backing_file_depth()

[Luiz Capitulino]

On Mon, 30 Jul 2012 10:48:49 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 07/30/2012 10:32 AM, Luiz Capitulino wrote:
> > On Thu, 26 Jul 2012 15:18:19 +0200
> > benoit.canet@gmail.com wrote:
> > 
> >> From: Benoît Canet <benoit@irqsave.net>
> >>
> >> Use the dedicated counting function in qmp_query_block in order to
> >> propagate the backing file depth to HMP.
> >>
> >> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> 
> >> +++ b/qapi-schema.json
> >> @@ -398,6 +398,8 @@
> >>  #
> >>  # @backing_file: #optional the name of the backing file (for copy-on-write)
> >>  #
> >> +# @backing_file_depth: number of files in the backing file chain (since: 1.2)
> >> +#
> >>  # @encrypted: true if the backing device is encrypted
> >>  #
> >>  # @bps: total throughput limit in bytes per second is specified
> >> @@ -418,9 +420,10 @@
> >>  ##
> >>  { 'type': 'BlockDeviceInfo',
> >>    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
> >> -            '*backing_file': 'str', 'encrypted': 'bool',
> >> -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
> >> -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
> >> +            '*backing_file': 'str', 'backing-file-depth': 'int',
> > 
> > Should use underscores, ie. should be backing_file_depth.
> 
> Really?  I thought we _want_ new interfaces to use '-', not '_', in QMP.

The problem is mixing them. BlockDeviceInfo already uses "_", and it's
not only for backing_file.

For new commands or existing commands that don't use "_", we certainly should
use "-", but mixing them for the same command is probably a bad thing.

>  For 'backing_file', we are stuck due to back-compat (unless Anthony's
> proposed patch to parse names case-insensitively with '-' and '_' folded
> together is taken),

We can't take Anthony's patch for what we emit. We could use it for input,
but I don't think it's worth it.

> but for the new field, we have no back-compat
> constraints, and I would prefer backing-file-depth.

It's like not I can't be convinced, but I think we shouldn't mix "-" and
"_" for the same command.

> 
> At any rate, you definitely need to make sure you agree between the docs
> above and the JSON statement below (as written, you have both spellings
> in the same patch).
>