Re: [Qemu-devel] [PATCH v2 6/9] block/qcow2-bitmap: do not remove bitmaps on reopen-ro

[John Snow]

On 5/31/19 12:31 PM, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_reopen_bitmaps_ro wants to store bitmaps and then mark them all
> readonly. But the latter don't work, as
> qcow2_store_persistent_dirty_bitmaps removes bitmaps after storing.
> It's OK for inactivation but bad idea for reopen-ro. And this leads to
> the following bug:
> 
> Assume we have persistent bitmap 'bitmap0'.
> Create external snapshot
>   bitmap0 is stored and therefore removed
> Commit snapshot
>   now we have no bitmaps
> Do some writes from guest (*)
>   they are not marked in bitmap
> Shutdown
> Start
>   bitmap0 is loaded as valid, but it is actually broken! It misses
>   writes (*)
> Incremental backup
>   it will be inconsistent
> 
> So, let's stop removing bitmaps on reopen-ro. But don't rejoice:
> reopening bitmaps to rw is broken too, so the whole scenario will not
> work after this patch and we can't enable corresponding test cases in
> 255 iotests still. Reopening bitmaps rw will be fixed in the following
> patches.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/qcow2.h        |  3 ++-
>  block/qcow2-bitmap.c | 46 +++++++++++++++++++++++++++++---------------
>  block/qcow2.c        |  2 +-
>  3 files changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 88a2030f54..4c8435141b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -734,7 +734,8 @@ Qcow2BitmapInfoList 
> *qcow2_get_bitmap_info_list(BlockDriverState *bs,
>                                                  Error **errp);
>  int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
>  int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error 
> **errp);
> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +                                          bool release_stored, Error **errp);
>  int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
>  bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>                                        const char *name,
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index fbeee37243..25b1e069a7 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1432,7 +1432,29 @@ fail:
>      bitmap_list_free(bm_list);
>  }
>  
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +/*
> + * qcow2_store_persistent_dirty_bitmaps
> + *
> + * Stores persistent BdrvDirtyBitmap's.
> + *

No apostrophe for plural's

> + * @release_stored: if true, release BdrvDirtyBitmap's after storing to the
> + * image. This is used in two cases, both via qcow2_inactivate:
> + * 1. bdrv_close: It's correct to remove bitmaps on close.
> + * 2. migration: If bitmaps are migrated through migration channel via
> + *    'dirty-bitmaps' migration capability they are not handled by this code.
> + *    Otherwise, it's OK to drop BdrvDirtyBitmap's and reload them on
> + *    invalidation.
> + *
> + * Anyway, it's correct to remove BdrvDirtyBitmap's on inactivation, as
> + * inactivation means that we lose control on disk, and therefore on bitmaps,
> + * we should sync them and do not touch more.
> + *
> + * Contrariwise, we don't want to release any bitmaps on just reopen-to-ro,
> + * when we need to store them, as image is still under our control, and it's
> + * good to keep all the bitmaps in read-only mode.
> + */

I have to admit that 'Contrariwise' is not an everyday term for me. You
should keep it in here just for fun, in my opinion.

Regarding "it's good to keep all the bitmaps in read-only mode":
More directly, keeping them read-only is correct because this is what
would happen if we opened the node readonly to begin with, and whether
we opened directly or reopened to that state shouldn't matter for the
state we get afterward.

> +void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +                                          bool release_stored, Error **errp)
>  {
>      BdrvDirtyBitmap *bitmap;
>      BDRVQcow2State *s = bs->opaque;
> @@ -1545,20 +1567,14 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>          g_free(tb);
>      }
>  
> -    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> -        /* For safety, we remove bitmap after storing.
> -         * We may be here in two cases:
> -         * 1. bdrv_close. It's ok to drop bitmap.
> -         * 2. inactivation. It means migration without 'dirty-bitmaps'
> -         *    capability, so bitmaps are not marked with
> -         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
> -         *    and reload on invalidation.
> -         */
> -        if (bm->dirty_bitmap == NULL) {
> -            continue;
> -        }
> +    if (release_stored) {
> +        QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +            if (bm->dirty_bitmap == NULL) {
> +                continue;
> +            }
>  
> -        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +            bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
> +        }
>      }
>  
>  success:
> @@ -1586,7 +1602,7 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error 
> **errp)
>      BdrvDirtyBitmap *bitmap;
>      Error *local_err = NULL;
>  
> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
> +    qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
>          return -EINVAL;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index f2cb131048..02d8ce7534 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2344,7 +2344,7 @@ static int qcow2_inactivate(BlockDriverState *bs)
>      int ret, result = 0;
>      Error *local_err = NULL;
>  
> -    qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
> +    qcow2_store_persistent_dirty_bitmaps(bs, true, &local_err);
>      if (local_err != NULL) {
>          result = -EINVAL;
>          error_reportf_err(local_err, "Lost persistent bitmaps during "
> 

code:
Reviewed-by: John Snow <address@hidden>

(You can adjust the docs as you need to on further review, if any, and
keep that RB. --js)