Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[Fam Zheng]

On Wed, 12/20 18:49, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/dirty-bitmap.h |  3 +++
>  block/dirty-bitmap.c         | 25 ++++++++++++++++---------
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 93d4336505..caf1f3d861 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -92,5 +92,8 @@ 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);
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> +                                                  BdrvDirtyBitmap *bitmap,
> +                                                  Error **errp);
>  
>  #endif
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index d83da077d5..fe27ddfb83 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -327,15 +327,11 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>   * The merged parent will be un-frozen, but not explicitly re-enabled.
>   * Called with BQL taken.

Maybe update the comment as

s/Called with BQL taken/Called within bdrv_dirty_bitmap_lock..unlock/

and ...

>   */
> -BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> -                                           BdrvDirtyBitmap *parent,
> -                                           Error **errp)
> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> +                                                  BdrvDirtyBitmap *parent,
> +                                                  Error **errp)
>  {
> -    BdrvDirtyBitmap *successor;
> -
> -    qemu_mutex_lock(parent->mutex);
> -
> -    successor = parent->successor;
> +    BdrvDirtyBitmap *successor = parent->successor;
>  
>      if (!successor) {
>          error_setg(errp, "Cannot reclaim a successor when none is present");
> @@ -349,9 +345,20 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>      bdrv_release_dirty_bitmap_locked(bs, successor);
>      parent->successor = NULL;
>  
> +    return parent;
> +}
> +

... move the "Called with BQL taken" comment here?

> +BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> +                                           BdrvDirtyBitmap *parent,
> +                                           Error **errp)
> +{
> +    BdrvDirtyBitmap *ret;
> +
> +    qemu_mutex_lock(parent->mutex);
> +    ret = bdrv_reclaim_dirty_bitmap_locked(bs, parent, errp);
>      qemu_mutex_unlock(parent->mutex);
>  
> -    return parent;
> +    return ret;
>  }
>  
>  /**
> -- 
> 2.11.1
>