Re: [Qemu-block] [PATCH 2/3] block/qcow2-bitmap: get rid of bdrv_has_changed_persistent_bitmaps

[John Snow]

On 5/23/19 11:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> Firstly, no reason to optimize failure path. Then, function name is
> ambiguous: it checks for readonly and similar things, but someone may
> think that it will ignore normal bitmaps which was just unchanged, and
> this is in bad relation with the fact that we should drop IN_USE flag
> for unchanged bitmaps in the image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/dirty-bitmap.h |  1 -
>  block/dirty-bitmap.c         | 12 ------------
>  block/qcow2-bitmap.c         | 23 +++++++++++++----------
>  3 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 8044ace63e..816022972b 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -105,7 +105,6 @@ bool bdrv_has_readonly_bitmaps(BlockDriverState *bs);
>  bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap);
> -bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs);
>  BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>                                          BdrvDirtyBitmap *bitmap);
>  char *bdrv_dirty_bitmap_sha256(const BdrvDirtyBitmap *bitmap, Error **errp);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 59e6ebb861..eca2eed0bf 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -775,18 +775,6 @@ bool bdrv_dirty_bitmap_inconsistent(const 
> BdrvDirtyBitmap *bitmap)
>      return bitmap->inconsistent;
>  }
>  
> -bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
> -{
> -    BdrvDirtyBitmap *bm;
> -    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> -        if (bm->persistent && !bm->readonly && !bm->migration) {

The loop down below has these conditionals for skipping bitmaps:

        if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
            bdrv_dirty_bitmap_readonly(bitmap) ||
            bdrv_dirty_bitmap_inconsistent(bitmap)) {
            continue;
        }

It looks like a semantic change, but hiding inside of get_persistence is
this:

bitmap->persistent && !bitmap->migration;

So this is equivalent, actually. It's not readily apparent at a glance.

> -            return true;
> -        }
> -    }
> -
> -    return false;
> -}
> -
>  BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs,
>                                          BdrvDirtyBitmap *bitmap)
>  {
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 8a75366c92..2b84bfa007 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1457,16 +1457,7 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>      Qcow2Bitmap *bm;
>      QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
>      Qcow2BitmapTable *tb, *tb_next;
> -
> -    if (!bdrv_has_changed_persistent_bitmaps(bs)) {
> -        /* nothing to do */
> -        return;
> -    }
> -
> -    if (!can_write(bs)) {
> -        error_setg(errp, "No write access");
> -        return;
> -    }
> +    bool need_write = false;
>  
>      QSIMPLEQ_INIT(&drop_tables);
>  
> @@ -1494,6 +1485,8 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>              continue;
>          }
>  
> +        need_write = true;
> +
>          if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
>              error_prepend(errp, "Bitmap '%s' doesn't satisfy the 
> constraints: ",
>                            name);
> @@ -1532,6 +1525,15 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>          bm->dirty_bitmap = bitmap;
>      }
>  
> +    if (!need_write) {
> +        goto success;
> +    }
> +
> +    if (!can_write(bs)) {
> +        error_setg(errp, "No write access");
> +        goto fail;
> +    }
> +
>      /* allocate clusters and store bitmaps */
>      QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>          if (bm->dirty_bitmap == NULL) {
> @@ -1573,6 +1575,7 @@ void 
> qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>          bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
>      }
>  
> +success:
>      bitmap_list_free(bm_list);
>      return;
>  
> 

Alright, interesting. You're right that the function you're removing is
pretty badly named for what it actually does. I got confused by that not
too long ago.

The rationale against optimizing the error path works for me, too.

Seems like it's equivalent before and after, so:

Reviewed-by: John Snow <address@hidden>