Re: [Qemu-devel] [PATCH v3 2/3] qapi: implement block-dirty-bitmap-remove transaction action

[Vladimir Sementsov-Ogievskiy]

09.07.2019 1:05, John Snow wrote:
> It is used to do transactional movement of the bitmap (which is
> possible in conjunction with merge command). Transactional bitmap
> movement is needed in scenarios with external snapshot, when we don't
> want to leave copy of the bitmap in the base image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> ---
>   block.c                        |  2 +-
>   block/dirty-bitmap.c           | 15 +++----
>   blockdev.c                     | 79 +++++++++++++++++++++++++++++++---
>   include/block/dirty-bitmap.h   |  2 +-
>   migration/block-dirty-bitmap.c |  2 +-
>   qapi/transaction.json          |  2 +
>   6 files changed, 85 insertions(+), 17 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c139540f2b..5195d4b910 100644
> --- a/block.c
> +++ b/block.c
> @@ -5316,7 +5316,7 @@ static void coroutine_fn 
> bdrv_co_invalidate_cache(BlockDriverState *bs,
>       for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
>            bm = bdrv_dirty_bitmap_next(bs, bm))
>       {
> -        bdrv_dirty_bitmap_set_migration(bm, false);
> +        bdrv_dirty_bitmap_skip_store(bm, false);
>       }
>   
>       ret = refresh_total_sectors(bs, bs->total_sectors);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 95a9c2a5d8..a308e1f84b 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap {
>       bool inconsistent;          /* bitmap is persistent, but inconsistent.
>                                      It cannot be used at all in any way, 
> except
>                                      a QMP user can remove it. */
> -    bool migration;             /* Bitmap is selected for migration, it 
> should
> -                                   not be stored on the next inactivation
> -                                   (persistent flag doesn't matter until next
> -                                   invalidation).*/
> +    bool skip_store;            /* We are either migrating or deleting this
> +                                 * bitmap; it should not be stored on the 
> next
> +                                 * inactivation. */
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>   };
>   
> @@ -757,16 +756,16 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap 
> *bitmap)
>   }
>   
>   /* Called with BQL taken. */
> -void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
> +void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip)
>   {
>       qemu_mutex_lock(bitmap->mutex);
> -    bitmap->migration = migration;
> +    bitmap->skip_store = skip;
>       qemu_mutex_unlock(bitmap->mutex);
>   }
>   
>   bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap)
>   {
> -    return bitmap->persistent && !bitmap->migration;
> +    return bitmap->persistent && !bitmap->skip_store;
>   }
>   
>   bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap)
> @@ -778,7 +777,7 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState 
> *bs)
>   {
>       BdrvDirtyBitmap *bm;
>       QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> -        if (bm->persistent && !bm->readonly && !bm->migration) {
> +        if (bm->persistent && !bm->readonly && !bm->skip_store) {
>               return true;
>           }
>       }
> diff --git a/blockdev.c b/blockdev.c
> index 01248252ca..800b3dcb42 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2134,6 +2134,51 @@ static void 
> block_dirty_bitmap_merge_prepare(BlkActionState *common,
>                                                   errp);
>   }
>   
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove(
> +        const char *node, const char *name, bool release,
> +        BlockDriverState **bitmap_bs, Error **errp);
> +
> +static void block_dirty_bitmap_remove_prepare(BlkActionState *common,
> +                                              Error **errp)
> +{
> +    BlockDirtyBitmap *action;
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (action_check_completion_mode(common, errp) < 0) {
> +        return;
> +    }
> +
> +    action = common->action->u.block_dirty_bitmap_remove.data;
> +
> +    state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name,
> +                                                 false, &state->bs, errp);
> +    if (state->bitmap) {
> +        bdrv_dirty_bitmap_skip_store(state->bitmap, true);
> +        bdrv_dirty_bitmap_set_busy(state->bitmap, true);
> +    }
> +}
> +
> +static void block_dirty_bitmap_remove_abort(BlkActionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +
> +    if (state->bitmap) {

Hmm, interesting, I thought, abort should not be called, if prepare failed, so 
the
following may be done unconditionally?

> +        bdrv_dirty_bitmap_skip_store(state->bitmap, false);
> +        bdrv_dirty_bitmap_set_busy(state->bitmap, false);
> +    }
> +}
> +

[..]

OK, I agree, good idea to reuse BUSY and migration field here and avoid new API.
Now release-prepare is less "release", but I don't see any real problems with 
it.
Maybe, we need something to be noted in docs.

Hmm, as we are not in a hurry now, we may wait for Nikolay to check this on his
scenarios.


-- 
Best regards,
Vladimir