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

[Vladimir Sementsov-Ogievskiy]

14.11.2017 02:32, John Snow wrote:
> 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.

it was your proposal)))

https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg01172.html

"
Your new use case here sounds like Frozen to me, but it simply does not
have an anonymous successor to force it to be recognized as "frozen." We
can add a `bool protected` or `bool frozen` field to force recognition
of this status and adjust the json documentation accordingly.

I think then we'd have four recognized states:

FROZEN: Cannot be reset/deleted. Bitmap is in-use by a block job or
other internal process. Bitmap is otherwise ACTIVE.
DISABLED: Not recording any writes (by choice.)
READONLY: Not able to record any writes (by necessity.)
ACTIVE: Normal bitmap status.

Sound right?
"


> 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


--
Best regards,
Vladimir