Re: [Qemu-block] [Qemu-devel] [PATCH v8 04/14] block/dirty-bitmap: add bdrv_dirty_bitmap_set_frozen

[John Snow]

On 10/30/2017 12:32 PM, Vladimir Sementsov-Ogievskiy wrote:
> Make it possible to set bitmap 'frozen' without a successor.
> This is needed to protect the bitmap during outgoing bitmap postcopy
> migration.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/dirty-bitmap.h |  1 +
>  block/dirty-bitmap.c         | 22 ++++++++++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index a9e2a92e4f..ae6d697850 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -39,6 +39,7 @@ uint32_t 
> bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>  uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen);
>  const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>  int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 7578863aa1..67fc6bd6e0 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -40,6 +40,8 @@ struct BdrvDirtyBitmap {
>      QemuMutex *mutex;
>      HBitmap *bitmap;            /* Dirty bitmap implementation */
>      HBitmap *meta;              /* Meta dirty bitmap */
> +    bool frozen;                /* Bitmap is frozen, it can't be modified
> +                                   through QMP */

I hesitate, because this now means that we have two independent bits of
state we need to update and maintain consistency with.

Before:

Frozen: "Bitmap has a successor and is no longer itself recording
writes, though we are guaranteed to have a successor doing so on our
behalf."

After:

Frozen: "Bitmap may or may not have a successor, but it is disabled with
an even more limited subset of tasks than a traditionally disabled bitmap."

This changes the meaning of "frozen" a little, and I am not sure that is
a problem as such, but it does make the interface seem a little
"fuzzier" than it did prior.

>      BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
>      char *name;                 /* Optional non-empty unique ID */
>      int64_t size;               /* Size of the bitmap, in bytes */
> @@ -183,13 +185,22 @@ const char *bdrv_dirty_bitmap_name(const 
> BdrvDirtyBitmap *bitmap)
>  /* Called with BQL taken.  */
>  bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
>  {
> -    return bitmap->successor;
> +    return bitmap->frozen;
> +}

Are there any cases where we will be unfrozen, but actually have a
successor now? If so, under what circumstances does that happen?

> +
> +/* Called with BQL taken.  */
> +void bdrv_dirty_bitmap_set_frozen(BdrvDirtyBitmap *bitmap, bool frozen)
> +{
> +    qemu_mutex_lock(bitmap->mutex);
> +    assert(bitmap->successor == NULL);
> +    bitmap->frozen = frozen;
> +    qemu_mutex_unlock(bitmap->mutex);
>  }
>  

OK, so we can only "set frozen" (or unset) if we don't have a successor.

>  /* Called with BQL taken.  */
>  bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
>  {
> -    return !(bitmap->disabled || bitmap->successor);
> +    return !(bitmap->disabled || (bitmap->successor != NULL));
>  }
>  

I guess this just makes 'successor' more obviously non-boolean, which is
fine.

>  /* Called with BQL taken.  */
> @@ -234,6 +245,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState 
> *bs,
>  
>      /* Install the successor and freeze the parent */
>      bitmap->successor = child;
> +    bitmap->frozen = true;
>      return 0;
>  }
>  
> @@ -266,6 +278,8 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>      name = bitmap->name;
>      bitmap->name = NULL;
>      successor->name = name;
> +    assert(bitmap->frozen);
> +    bitmap->frozen = false;
>      bitmap->successor = NULL;
>      successor->persistent = bitmap->persistent;
>      bitmap->persistent = false;
> @@ -298,6 +312,8 @@ BdrvDirtyBitmap 
> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
>          return NULL;
>      }
>      bdrv_release_dirty_bitmap(bs, successor);
> +    assert(parent->frozen);
> +    parent->frozen = false;
>      parent->successor = NULL;
>  
>      return parent;
> @@ -439,6 +455,8 @@ void bdrv_dirty_bitmap_release_successor(BlockDriverState 
> *bs,
>  
>      if (parent->successor) {
>          bdrv_release_dirty_bitmap_locked(bs, parent->successor);
> +        assert(parent->frozen);
> +        parent->frozen = false;
>          parent->successor = NULL;
>      }
>  
> 

Tentatively:

Reviewed-by: John Snow <address@hidden>

Fam, any thoughts?

--John