[Qemu-devel] [PATCH V2 0/3] Show backing file ancestors count in HMP

[Qemu-devel] [PATCH V2 0/3] Show backing file ancestors count in HMP

[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 ancestors
count and take the good decision (streaming).

in v2:

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

Benoît Canet (3):
  block: create bdrv_get_backing_file_ancestors_count()
  block: Use bdrv_get_backing_file_ancestors_count()
  hmp: show the backing file ancestors count

 block.c          |   15 +++++++++++++++
 block.h          |    1 +
 hmp.c            |    2 ++
 qapi-schema.json |    9 ++++++---
 4 files changed, 24 insertions(+), 3 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH V2 1/3] block: create bdrv_get_backing_file_ancestors_count()

[benoit.canet]

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

Create bdrv_get_backing_file_ancestors_count() 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..03e0860 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_ancestors_count(BlockDriverState *bs)
+{
+    if (!bs->drv) {
+        return 0;
+    }
+
+    if (!bs->backing_hd) {
+        return 0;
+    }
+
+    return 1 + bdrv_get_backing_file_ancestors_count(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..5b1ba2b 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_ancestors_count(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 V2 2/3] block: Use bdrv_get_backing_file_ancestors_count()

[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 count to HMP.

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

diff --git a/block.c b/block.c
index 03e0860..4aa3ea9 100644
--- a/block.c
+++ b/block.c
@@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
             if (bs->backing_file[0]) {
                 info->value->inserted->has_backing_file = true;
                 info->value->inserted->backing_file = g_strdup(bs->backing_file);
+                info->value->inserted->backing_file_ancestors_count =
+                    bdrv_get_backing_file_ancestors_count(bs);
             }
 
             if (bs->io_limits_enabled) {
diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
+#
 # @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_ancestors_count': '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 V2 3/3] hmp: show the backing file ancestors count

[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..19dcb65 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_ancestors_count=%" PRId64,
+                    info->value->inserted->backing_file_ancestors_count);
             }
             monitor_printf(mon, " ro=%d drv=%s encrypted=%d",
                            info->value->inserted->ro,
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH V2 2/3] block: Use bdrv_get_backing_file_ancestors_count()

[Kevin Wolf]

Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> From: Benoît Canet <benoit@irqsave.net>
> 
> Use the dedicated counting function in qmp_query_block in order to
> propagate the backing file count to HMP.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  block.c          |    2 ++
>  qapi-schema.json |    9 ++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 03e0860..4aa3ea9 100644
> --- a/block.c
> +++ b/block.c
> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
>              if (bs->backing_file[0]) {
>                  info->value->inserted->has_backing_file = true;
>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
> +                info->value->inserted->backing_file_ancestors_count =
> +                    bdrv_get_backing_file_ancestors_count(bs);
>              }
>  
>              if (bs->io_limits_enabled) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
> +#

Why is it optional? Would it be omitted rather than set to 0 if there
are no backing files?

Could also use a "(since: 1.2)" note.

Kevin

Re: [Qemu-devel] [PATCH V2 3/3] hmp: show the backing file ancestors count

[Kevin Wolf]

Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> 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..19dcb65 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_ancestors_count=%" PRId64,
> +                    info->value->inserted->backing_file_ancestors_count);

This is not what a user interface looks like. Maybe something like "%d
backing files", and omit the message if there aren't any backing files.

Kevin

Re: [Qemu-devel] [PATCH V2 2/3] block: Use bdrv_get_backing_file_ancestors_count()

[Benoît Canet]

Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> > From: Benoît Canet <benoit@irqsave.net>
> > 
> > Use the dedicated counting function in qmp_query_block in order to
> > propagate the backing file count to HMP.
> > 
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  block.c          |    2 ++
> >  qapi-schema.json |    9 ++++++---
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 03e0860..4aa3ea9 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
> >              if (bs->backing_file[0]) {
> >                  info->value->inserted->has_backing_file = true;
> >                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
> > +                info->value->inserted->backing_file_ancestors_count =
> > +                    bdrv_get_backing_file_ancestors_count(bs);
> >              }
> >  
> >              if (bs->io_limits_enabled) {
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
> > +#
> 
> Why is it optional? Would it be omitted rather than set to 0 if there
> are no backing files?

I made it optional because backing_file=something is optional.
So It seemed coherent to make it also optional.
However I'll change it if you confirm it should be changed.

> 
> Could also use a "(since: 1.2)" note.
> 
> Kevin

Re: [Qemu-devel] [PATCH V2 2/3] block: Use bdrv_get_backing_file_ancestors_count()

[Kevin Wolf]

Am 25.07.2012 13:15, schrieb Benoît Canet:
> Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
>> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
>>> From: Benoît Canet <benoit@irqsave.net>
>>>
>>> Use the dedicated counting function in qmp_query_block in order to
>>> propagate the backing file count to HMP.
>>>
>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>> ---
>>>  block.c          |    2 ++
>>>  qapi-schema.json |    9 ++++++---
>>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 03e0860..4aa3ea9 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
>>>              if (bs->backing_file[0]) {
>>>                  info->value->inserted->has_backing_file = true;
>>>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
>>> +                info->value->inserted->backing_file_ancestors_count =
>>> +                    bdrv_get_backing_file_ancestors_count(bs);
>>>              }
>>>  
>>>              if (bs->io_limits_enabled) {
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
>>> +#
>>
>> Why is it optional? Would it be omitted rather than set to 0 if there
>> are no backing files?
> 
> I made it optional because backing_file=something is optional.
> So It seemed coherent to make it also optional.
> However I'll change it if you confirm it should be changed.

The reason why the backing file naem is optional is because there is no
valid value to use when there is no backing file. But for the count, 0
makes perfect sense.

Kevin

Re: [Qemu-devel] [PATCH V2 3/3] hmp: show the backing file ancestors count

[Kevin Wolf]

Am 25.07.2012 13:17, schrieb Benoît Canet:
> Le Wednesday 25 Jul 2012 à 12:59:00 (+0200), Kevin Wolf a écrit :
>> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
>>> 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..19dcb65 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_ancestors_count=%" PRId64,
>>> +                    info->value->inserted->backing_file_ancestors_count);
>>
>> This is not what a user interface looks like. Maybe something like "%d
>> backing files", and omit the message if there aren't any backing files.
> 
> The whole line of info block look like this:
> virtio0: removable=0 io-status=ok file=/home/benoit/images/gruik.qed backing_file=/home/benoit/images/snap.qed backing_file_ancestors_count=1 ro=0 drv=qed encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0
> Again I tried to make something coherent.

I see. I wasn't really aware that it's already that bad today. The whole
output of this command should be changed, but it's not in the scope of
your series.

> It's already ommited when there is no backing file (the field is optional).

Yeah, sorry, I missed the context.

Kevin

Re: [Qemu-devel] [PATCH V2 2/3] block: Use bdrv_get_backing_file_ancestors_count()

[Benoît Canet]

Le Wednesday 25 Jul 2012 à 13:19:15 (+0200), Kevin Wolf a écrit :
> Am 25.07.2012 13:15, schrieb Benoît Canet:
> > Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
> >> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
> >>> From: Benoît Canet <benoit@irqsave.net>
> >>>
> >>> Use the dedicated counting function in qmp_query_block in order to
> >>> propagate the backing file count to HMP.
> >>>
> >>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> >>> ---
> >>>  block.c          |    2 ++
> >>>  qapi-schema.json |    9 ++++++---
> >>>  2 files changed, 8 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index 03e0860..4aa3ea9 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
> >>>              if (bs->backing_file[0]) {
> >>>                  info->value->inserted->has_backing_file = true;
> >>>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
> >>> +                info->value->inserted->backing_file_ancestors_count =
> >>> +                    bdrv_get_backing_file_ancestors_count(bs);
> >>>              }
> >>>  
> >>>              if (bs->io_limits_enabled) {
> >>> diff --git a/qapi-schema.json b/qapi-schema.json
> >>> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
> >>> +#
> >>
> >> Why is it optional? Would it be omitted rather than set to 0 if there
> >> are no backing files?
> > 
> > I made it optional because backing_file=something is optional.
> > So It seemed coherent to make it also optional.
> > However I'll change it if you confirm it should be changed.
> 
> The reason why the backing file naem is optional is because there is no
> valid value to use when there is no backing file. But for the count, 0
> makes perfect sense.

Should I also make is alway present in the HMP output ?

Benoît

> 
> Kevin
> 

Re: [Qemu-devel] [PATCH V2 2/3] block: Use bdrv_get_backing_file_ancestors_count()

[Kevin Wolf]

Am 25.07.2012 13:42, schrieb Benoît Canet:
> Le Wednesday 25 Jul 2012 à 13:19:15 (+0200), Kevin Wolf a écrit :
>> Am 25.07.2012 13:15, schrieb Benoît Canet:
>>> Le Wednesday 25 Jul 2012 à 12:57:05 (+0200), Kevin Wolf a écrit :
>>>> Am 25.07.2012 10:11, schrieb benoit.canet@gmail.com:
>>>>> From: Benoît Canet <benoit@irqsave.net>
>>>>>
>>>>> Use the dedicated counting function in qmp_query_block in order to
>>>>> propagate the backing file count to HMP.
>>>>>
>>>>> Signed-off-by: Benoit Canet <benoit@irqsave.net>
>>>>> ---
>>>>>  block.c          |    2 ++
>>>>>  qapi-schema.json |    9 ++++++---
>>>>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index 03e0860..4aa3ea9 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -2448,6 +2448,8 @@ BlockInfoList *qmp_query_block(Error **errp)
>>>>>              if (bs->backing_file[0]) {
>>>>>                  info->value->inserted->has_backing_file = true;
>>>>>                  info->value->inserted->backing_file = g_strdup(bs->backing_file);
>>>>> +                info->value->inserted->backing_file_ancestors_count =
>>>>> +                    bdrv_get_backing_file_ancestors_count(bs);
>>>>>              }
>>>>>  
>>>>>              if (bs->io_limits_enabled) {
>>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>>> index a92adb1..eb72c16 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_ancestors_count: #optional the count of ancestors backing files (for copy-on-write)
>>>>> +#
>>>>
>>>> Why is it optional? Would it be omitted rather than set to 0 if there
>>>> are no backing files?
>>>
>>> I made it optional because backing_file=something is optional.
>>> So It seemed coherent to make it also optional.
>>> However I'll change it if you confirm it should be changed.
>>
>> The reason why the backing file naem is optional is because there is no
>> valid value to use when there is no backing file. But for the count, 0
>> makes perfect sense.
> 
> Should I also make is alway present in the HMP output ?

I think I would hide it in HMP if it is 0, but this isn't a very strong
opinion. You can do it as you like.

Kevin