Re: [PATCH v7 19/25] parallels: report the stored dirty bitmaps in qemu-img info

[Markus Armbruster <armbru@redhat.com>]

"Denis V. Lunev" <den@openvz.org> writes:

> From: Denis V. Lunev <den@openvz.org>
>
> Nothing tells which persistent bitmaps an image carries. qemu-img info
> says nothing about them, and the only other way to see one is to export
> the image over NBD and ask for a bitmap by name, which needs the name
> beforehand.
>
> Add ImageInfoSpecificParallels with the bitmaps and their granularity,
> in the same way as qcow2 reports the contents of its bitmap directory:
>
>     Format specific information:
>         bitmaps:
>             [0]:
>                 name: b2c9e1a4-5d3f-4e8b-9a7c-6f0d1e2b3a45
>                 granularity: 65536
>
> For running VM process the list is built on the fly from in-memory
> list. This is the best we can do. FormatExtension could be dead in
> the image while VMs are running as all bitmaps are cleared on
> non-clean VM stop.

What's "FormatExtension"?
>
> A bitmap of an image which was not closed correctly is inconsistent
> and can not be used. Report that as well, the way qcow2 reports its
> in-use flag, so such a bitmap is not listed as a valid one.

This is helpful, thanks!

> The section is left empty for an image with no persistent dirty
> bitmaps, and an empty section is not printed, so the human readable
> output of "qemu-img info" and "info block" is unchanged.

Perfectly clear now.

> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  block/parallels-ext.c                         | 26 +++++++++++
>  block/parallels.c                             | 20 ++++++++
>  block/parallels.h                             |  3 ++
>  qapi/block-core.json                          | 46 ++++++++++++++++++-
>  tests/qemu-iotests/tests/parallels-checks     |  6 +++
>  tests/qemu-iotests/tests/parallels-checks.out | 19 ++++++++
>  6 files changed, 118 insertions(+), 2 deletions(-)
>
> diff --git a/block/parallels-ext.c b/block/parallels-ext.c
> index f687f2da7c..8b8035af91 100644
> --- a/block/parallels-ext.c
> +++ b/block/parallels-ext.c
> @@ -736,3 +736,29 @@ parallels_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>  
>      return ret;
>  }
> +
> +void parallels_get_bitmap_info_list(BlockDriverState *bs,
> +                                    ParallelsBitmapInfoList **info_list)
> +{
> +    BdrvDirtyBitmap *bitmap;
> +    ParallelsBitmapInfoList **tail = info_list;
> +
> +    *info_list = NULL;
> +
> +    FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> +        ParallelsBitmapInfo *info;
> +
> +        if (!bdrv_dirty_bitmap_get_persistence(bitmap)) {
> +            continue;
> +        }
> +
> +        info = g_new0(ParallelsBitmapInfo, 1);
> +        info->name = g_strdup(bdrv_dirty_bitmap_name(bitmap));
> +        info->granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +        if (bdrv_dirty_bitmap_inconsistent(bitmap)) {
> +            info->has_inconsistent = true;
> +            info->inconsistent = true;
> +        }
> +        QAPI_LIST_APPEND(tail, info);
> +    }
> +}
> diff --git a/block/parallels.c b/block/parallels.c
> index 2a5ceb8978..e7d65d0458 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -1624,6 +1624,25 @@ static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs)
>      return 1;
>  }
>  
> +static ImageInfoSpecific * GRAPH_RDLOCK
> +parallels_get_specific_info(BlockDriverState *bs, Error **errp)
> +{
> +    ImageInfoSpecificParallels *parallels_info;
> +    ImageInfoSpecific *spec_info;
> +
> +    parallels_info = g_new0(ImageInfoSpecificParallels, 1);
> +    parallels_get_bitmap_info_list(bs, &parallels_info->bitmaps);
> +    parallels_info->has_bitmaps = !!parallels_info->bitmaps;
> +
> +    spec_info = g_new(ImageInfoSpecific, 1);
> +    *spec_info = (ImageInfoSpecific){
> +        .type = IMAGE_INFO_SPECIFIC_KIND_PARALLELS,
> +        .u.parallels.data = parallels_info,
> +    };
> +
> +    return spec_info;
> +}
> +
>  static BlockDriver bdrv_parallels = {
>      .format_name                = "parallels",
>      .instance_size              = sizeof(BDRVParallelsState),
> @@ -1653,6 +1672,7 @@ static BlockDriver bdrv_parallels = {
>                                    parallels_co_can_store_new_dirty_bitmap,
>      .bdrv_co_remove_persistent_dirty_bitmap =
>                                    parallels_co_remove_persistent_dirty_bitmap,
> +    .bdrv_get_specific_info     = parallels_get_specific_info,
>  };
>  
>  static void bdrv_parallels_init(void)
> diff --git a/block/parallels.h b/block/parallels.h
> index 27d8c3ac83..012f47320b 100644
> --- a/block/parallels.h
> +++ b/block/parallels.h
> @@ -32,6 +32,7 @@
>  #ifndef BLOCK_PARALLELS_H
>  #define BLOCK_PARALLELS_H
>  #include "qemu/coroutine.h"
> +#include "qapi/qapi-types-block-core.h"
>  
>  #define HEADS_NUMBER 16
>  #define SEC_IN_CYL 32
> @@ -111,5 +112,7 @@ parallels_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
>  int coroutine_fn GRAPH_RDLOCK
>  parallels_co_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>                                              const char *name, Error **errp);
> +void parallels_get_bitmap_info_list(BlockDriverState *bs,
> +                                    ParallelsBitmapInfoList **info_list);
>  
>  #endif
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 199efc1e00..7499aea641 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -188,6 +188,35 @@
>        '*extent-size-hint': 'size'
>    } }
>  
> +##
> +# @ParallelsBitmapInfo:
> +#
> +# Parallels dirty bitmap information.
> +#
> +# @name: the name of the bitmap
> +#
> +# @granularity: granularity of the bitmap in bytes
> +#
> +# @inconsistent: true if the bitmap was improperly stored and cannot
> +#     be used

I'd recommend "if the bitmap is in an inconsistent state and".

> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'ParallelsBitmapInfo',
> +  'data': { 'name': 'str', 'granularity': 'uint32',
> +            '*inconsistent': 'bool' } }
> +
> +##
> +# @ImageInfoSpecificParallels:
> +#
> +# @bitmaps: A list of the persistent dirty bitmaps of the image,
> +#     including the ones which are not written out yet

In review of v6, I asked whether "not written out yet" relevant to a
user / management application.  You explained what it means, but didn't
actually answer my question.  I failed to point that out then, sorry.
Can you answer it now?

I also inquired about the relation to ImageInfoSpecificQCow2's @bitmaps,
and you told me they're basically the same (correct me if I
misunderstood).  I then replied that the documentation should be
basically the same, and you agreed.  Did you forget?

> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'ImageInfoSpecificParallels',
> +  'data': { '*bitmaps': ['ParallelsBitmapInfo'] } }
> +
>  ##
>  # @ImageInfoSpecificKind:
>  #
> @@ -197,10 +226,12 @@
>  #
>  # @file: Since 8.0
>  #
> +# @parallels: Since 11.2
> +#
>  # Since: 1.7
>  ##
>  { 'enum': 'ImageInfoSpecificKind',
> -  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file' ] }
> +  'data': [ 'qcow2', 'vmdk', 'luks', 'rbd', 'file', 'parallels' ] }
>  
>  ##
>  # @ImageInfoSpecificQCow2Wrapper:
> @@ -255,6 +286,16 @@
>  { 'struct': 'ImageInfoSpecificFileWrapper',
>    'data': { 'data': 'ImageInfoSpecificFile' } }
>  
> +##
> +# @ImageInfoSpecificParallelsWrapper:
> +#
> +# @data: image information specific to Parallels
> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'ImageInfoSpecificParallelsWrapper',
> +  'data': { 'data': 'ImageInfoSpecificParallels' } }
> +
>  ##
>  # @ImageInfoSpecific:
>  #
> @@ -273,7 +314,8 @@
>        'vmdk': 'ImageInfoSpecificVmdkWrapper',
>        'luks': 'ImageInfoSpecificLUKSWrapper',
>        'rbd': 'ImageInfoSpecificRbdWrapper',
> -      'file': 'ImageInfoSpecificFileWrapper'
> +      'file': 'ImageInfoSpecificFileWrapper',
> +      'parallels': 'ImageInfoSpecificParallelsWrapper'
>    } }
>  
>  ##

[...]