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

[John Snow <jsnow@redhat.com>]

On 11/17/2017 09:46 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 <vsementsov@virtuozzo.com>
>>> ---
>>>   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?
> "
> 
> 

I was afraid you'd say that :(

It's okay, anyway. I shouldn't let myself go so long between reviews
like this, because you catch me changing my mind. Anyway, please go
ahead with it. I don't want to delay you on something that works because
I can't make up *my* mind.