Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps

[John Snow <jsnow@redhat.com>]

On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-dirty-bitmap.c |  5 +++++
>  block/qcow2.c              | 13 +++++++++++--
>  block/qcow2.h              |  9 +++++++++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c
> index db83112..686a121 100644
> --- a/block/qcow2-dirty-bitmap.c
> +++ b/block/qcow2-dirty-bitmap.c
> @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverState *bs)
>  
>      s->dirty_bitmaps_offset = dirty_bitmaps_offset;
>      s->dirty_bitmaps_size = dirty_bitmaps_size;
> +    if (s->nb_dirty_bitmaps > 0) {
> +        s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
> +    } else {
> +        s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS;
> +    }
>      ret = qcow2_update_header(bs);
>      if (ret < 0) {
>          fprintf(stderr, "Could not update qcow2 header\n");
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 406e55d..f85a55a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>                  return ret;
>              }
>  
> +            if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) &&
> +                s->nb_dirty_bitmaps > 0) {
> +                ret = qcow2_delete_all_dirty_bitmaps(bs, errp);
> +                if (ret < 0) {
> +                    return ret;
> +                }
> +            }
> +
>  #ifdef DEBUG_EXT
>              printf("Qcow2: Got dirty bitmaps extension:"
>                     " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n",
> @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>  
>      /* Clear unknown autoclear feature bits */
> -    if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) {
> -        s->autoclear_features = 0;
> +    if (!bs->read_only && !(flags & BDRV_O_INCOMING) &&
> +        (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) {
> +        s->autoclear_features |= QCOW2_AUTOCLEAR_MASK;

Like Stefan already mentioned, fixing this |= to &= will fix iotest 036,
which is otherwise broken by this patch.

>          ret = qcow2_update_header(bs);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret, "Could not update qcow2 header");
> diff --git a/block/qcow2.h b/block/qcow2.h
> index b5e576c..14bd6f9 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -215,6 +215,15 @@ enum {
>      QCOW2_COMPAT_FEAT_MASK            = QCOW2_COMPAT_LAZY_REFCOUNTS,
>  };
>  
> +/* Autoclear feature bits */
> +enum {
> +    QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR = 0,
> +    QCOW2_AUTOCLEAR_DIRTY_BITMAPS       =
> +        1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR,
> +
> +    QCOW2_AUTOCLEAR_MASK                = QCOW2_AUTOCLEAR_DIRTY_BITMAPS,
> +};
> +

I find it a little awkward to have an enum with three different kinds of
data in it, unless I am reading this incorrectly. (bit position, bit
masks, and accumulated bit mask.)

Just enumerating the indices is probably sufficient:

enum {
  QCOW2_AUTOCLEAR_BEGIN = 0,
  QCOW2_AUTOCLEAR_DIRTY_BITMAPS = QCOW2_AUTOCLEAR_BEGIN,
  ...,
  QCOW2_AUTOCLEAR_END
}

and then the QCOW2_AUTOCLEAR_MASK can either be programmatically defined
via a function, or just pre-computed as a #define.

If you still want the mask definitions, you could do something cheeky
like this:

#define AUTOCLEAR_MASK(X) (1 << QCOW2_AUTOCLEAR_ ## X)

and then you can use things like AUTOCLEAR_MASK(DIRTY_BITMAPS) without
having to create and maintain two separate tables if you want both forms
easily available.

>  enum qcow2_discard_type {
>      QCOW2_DISCARD_NEVER = 0,
>      QCOW2_DISCARD_ALWAYS,
>