Re: [Qemu-block] [PATCH 2/5] block/dirty-bitmap: Refactor bdrv_can_store_new_bitmap

[Vladimir Sementsov-Ogievskiy]

06.06.2019 21:41, John Snow wrote:
> Instead of bdrv_can_store_new_bitmap, rework this as
> bdrv_add_persistent_dirty_bitmap. This makes a more obvious symmetry
> with bdrv_remove_persistent_dirty_bitmap. Most importantly, we are free
> to modify the driver state because we know we ARE adding a bitmap
> instead of simply being asked if we CAN store one.
> 
> As part of this symmetry, move this function next to
> bdrv_remove_persistent_bitmap in block/dirty-bitmap.c.
> 
> To cement this semantic distinction, use a bitmap object instead of the
> (name, granularity) pair as this allows us to set persistence as a
> condition of the "add" succeeding.
> 
> Notably, the qcow2 implementation still does not actually store the new
> bitmap at add time, but merely ensures that it will be able to at store
> time.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/qcow2.h                |  5 ++---
>   include/block/block.h        |  2 --
>   include/block/block_int.h    |  5 ++---
>   include/block/dirty-bitmap.h |  3 +++
>   block.c                      | 22 ---------------------
>   block/dirty-bitmap.c         | 38 ++++++++++++++++++++++++++++++++++++
>   block/qcow2-bitmap.c         | 24 ++++++++++++++---------
>   block/qcow2.c                |  2 +-
>   blockdev.c                   | 19 +++++++-----------
>   9 files changed, 68 insertions(+), 52 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index fc1b0d3c1e..95d723d3c0 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -742,9 +742,8 @@ 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);
>   int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
> -                                      const char *name,
> -                                      uint32_t granularity,
> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> +                                      BdrvDirtyBitmap *bitmap,
>                                         Error **errp);
>   void qcow2_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>                                             const char *name,
> diff --git a/include/block/block.h b/include/block/block.h
> index f9415ed740..6d520f3b59 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -683,8 +683,6 @@ void bdrv_add_child(BlockDriverState *parent, 
> BlockDriverState *child,
>                       Error **errp);
>   void bdrv_del_child(BlockDriverState *parent, BdrvChild *child, Error 
> **errp);
>   
> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
> -                                     uint32_t granularity, Error **errp);
>   /**
>    *
>    * bdrv_register_buf/bdrv_unregister_buf:
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 06df2bda1b..93bbb66cd0 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -537,9 +537,8 @@ struct BlockDriver {
>        * field of BlockDirtyBitmap's in case of success.
>        */
>       int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
> -    bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs,
> -                                            const char *name,
> -                                            uint32_t granularity,
> +    int (*bdrv_add_persistent_dirty_bitmap)(BlockDriverState *bs,
> +                                            BdrvDirtyBitmap *bitmap,
>                                               Error **errp);
>       void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
>                                                   const char *name,
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8044ace63e..c37edbe05f 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -38,6 +38,9 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, 
> uint32_t flags,
>                               Error **errp);
>   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap 
> *bitmap);
>   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> +                                      BdrvDirtyBitmap *bitmap,
> +                                      Error **errp);
>   void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>                                            const char *name,
>                                            Error **errp);
> diff --git a/block.c b/block.c
> index e3e77feee0..6aec36b7c9 100644
> --- a/block.c
> +++ b/block.c
> @@ -6379,25 +6379,3 @@ void bdrv_del_child(BlockDriverState *parent_bs, 
> BdrvChild *child, Error **errp)
>   
>       parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
>   }
> -
> -bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
> -                                     uint32_t granularity, Error **errp)
> -{
> -    BlockDriver *drv = bs->drv;
> -
> -    if (!drv) {
> -        error_setg_errno(errp, ENOMEDIUM,
> -                         "Can't store persistent bitmaps to %s",
> -                         bdrv_get_device_or_node_name(bs));
> -        return false;
> -    }
> -
> -    if (!drv->bdrv_can_store_new_dirty_bitmap) {
> -        error_setg_errno(errp, ENOTSUP,
> -                         "Can't store persistent bitmaps to %s",
> -                         bdrv_get_device_or_node_name(bs));
> -        return false;
> -    }
> -
> -    return drv->bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp);
> -}
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 49646a30e6..615f8480b2 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -466,6 +466,44 @@ void 
> bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
>       }
>   }
>   
> +int bdrv_add_persistent_dirty_bitmap(BlockDriverState *bs,
> +                                     BdrvDirtyBitmap *bitmap,
> +                                     Error **errp)

strange thing for me: "bitmap" in function name is _not_ the same
thing as @bitmap argument.. as if it the same,
function adds "persistent bitmap", but @bitmap is not persistent yet,
so may be function, don't add persistent bitmap, but marks bitmap persistent?


Ok, I can think of it like "add persistent dirty bitmap to driver. if succeed, 
set
bitmap.persistent flag"


> +{
> +    BlockDriver *drv = bs->drv;
> +    int ret;
> +
> +    if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
> +        error_setg(errp, "Bitmap '%s' is already persistent, "
> +                   "and cannot be re-added to node '%s'",
> +                   bdrv_dirty_bitmap_name(bitmap),
> +                   bdrv_get_device_or_node_name(bs));
> +        return -EINVAL;
> +    }
> +
> +    if (!drv) {
> +        error_setg_errno(errp, ENOMEDIUM,
> +                         "Can't store persistent bitmaps to %s",
> +                         bdrv_get_device_or_node_name(bs));
> +        return -ENOMEDIUM;
> +    }
> +
> +    if (!drv->bdrv_add_persistent_dirty_bitmap) {
> +        error_setg_errno(errp, ENOTSUP,
> +                         "Can't store persistent bitmaps to %s",
> +                         bdrv_get_device_or_node_name(bs));
> +        return -ENOTSUP;
> +    }
> +
> +    ret = drv->bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp);
> +    if (ret == 0) {
> +        bdrv_dirty_bitmap_set_persistence(bitmap, true);
> +    }
> +
> +    return ret;
> +}
> +
> +
>   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>   {
>       bdrv_dirty_bitmap_lock(bitmap);
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 83aee1a42a..c3a72ca782 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1607,14 +1607,16 @@ int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, 
> Error **errp)
>       return 0;
>   }
>   
> -bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
> -                                      const char *name,
> -                                      uint32_t granularity,
> +int qcow2_add_persistent_dirty_bitmap(BlockDriverState *bs,
> +                                      BdrvDirtyBitmap *bitmap,
>                                         Error **errp)
>   {
>       BDRVQcow2State *s = bs->opaque;
>       bool found;
>       Qcow2BitmapList *bm_list;
> +    const char *name = bdrv_dirty_bitmap_name(bitmap);
> +    uint32_t granularity = bdrv_dirty_bitmap_granularity(bitmap);
> +    int ret = 0;

dead assignment

>   
>       if (s->qcow_version < 3) {
>           /* Without autoclear_features, we would always have to assume
> @@ -1623,20 +1625,23 @@ bool 
> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>            * have to drop all dirty bitmaps (defeating their purpose).
>            */
>           error_setg(errp, "Cannot store dirty bitmaps in qcow2 v2 files");
> +        ret = -ENOTSUP;
>           goto fail;
>       }
>   
> -    if (check_constraints_on_bitmap(bs, name, granularity, errp) != 0) {
> +    ret = check_constraints_on_bitmap(bs, name, granularity, errp);
> +    if (ret) {

hmm, in other places you prefere
if ((ret = ...)) {
syntax

>           goto fail;
>       }
>   
>       if (s->nb_bitmaps == 0) {
> -        return true;
> +        return 0;
>       }
>   
>       if (s->nb_bitmaps >= QCOW2_MAX_BITMAPS) {
>           error_setg(errp,
>                      "Maximum number of persistent bitmaps is already 
> reached");
> +        ret = -EOVERFLOW;
>           goto fail;
>       }
>   
> @@ -1644,24 +1649,25 @@ bool 
> qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>           QCOW2_MAX_BITMAP_DIRECTORY_SIZE)
>       {
>           error_setg(errp, "Not enough space in the bitmap directory");
> +        ret = -EOVERFLOW;
>           goto fail;
>       }
>   
> -    if (bitmap_list_load(bs, &bm_list, errp)) {
> +    if ((ret = bitmap_list_load(bs, &bm_list, errp))) {

interesting, now we load all bitmaps, so, may be, we don't need to load list 
every time,
but may use bs.dirty_bitmaps to check such things.. But it seems unsafe

>           goto fail;
>       }
>   
>       found = find_bitmap_by_name(bm_list, name);
>       bitmap_list_free(bm_list);
>       if (found) {
> +        ret = -EEXIST;
>           error_setg(errp, "Bitmap with the same name is already stored");
>           goto fail;
>       }
>   
> -    return true;
> -
> +    return 0;
>   fail:
>       error_prepend(errp, "Can't make bitmap '%s' persistent in '%s': ",
>                     name, bdrv_get_device_or_node_name(bs));

didn't you consider to move this error_prepend to caller and drop all these 
gotos?

> -    return false;
> +    return ret;
>   }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 9396d490d5..1c78eba71b 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5229,7 +5229,7 @@ BlockDriver bdrv_qcow2 = {
>       .bdrv_attach_aio_context  = qcow2_attach_aio_context,
>   
>       .bdrv_reopen_bitmaps_rw = qcow2_reopen_bitmaps_rw,
> -    .bdrv_can_store_new_dirty_bitmap = qcow2_can_store_new_dirty_bitmap,
> +    .bdrv_add_persistent_dirty_bitmap = qcow2_add_persistent_dirty_bitmap,
>       .bdrv_remove_persistent_dirty_bitmap = 
> qcow2_remove_persistent_dirty_bitmap,
>   };
>   
> diff --git a/blockdev.c b/blockdev.c
> index 3f44b891eb..169a8da831 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2851,26 +2851,21 @@ void qmp_block_dirty_bitmap_add(const char *node, 
> const char *name,
>           disabled = false;
>       }
>   
> -    if (persistent) {
> -        aio_context = bdrv_get_aio_context(bs);
> -        aio_context_acquire(aio_context);
> -        if (!bdrv_can_store_new_dirty_bitmap(bs, name, granularity, errp)) {
> -            goto out;
> -        }
> -    }
> -
>       bitmap = bdrv_create_dirty_bitmap(bs, granularity, name, errp);
>       if (bitmap == NULL) {
> -        goto out;
> +        return;
>       }
>   
>       if (disabled) {
>           bdrv_disable_dirty_bitmap(bitmap);
>       }
>   
> -    bdrv_dirty_bitmap_set_persistence(bitmap, persistent);
> - out:
> -    if (aio_context) {
> +    if (persistent) {

Good thing. I suggest to define aio_context in this block too.

> +        aio_context = bdrv_get_aio_context(bs);
> +        aio_context_acquire(aio_context);
> +        if (bdrv_add_persistent_dirty_bitmap(bs, bitmap, errp)) {
> +            bdrv_release_dirty_bitmap(bs, bitmap);
> +        }
>           aio_context_release(aio_context);
>       }
>   }
> 

With some my suggestions or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

-- 
Best regards,
Vladimir