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

[John Snow]

On 11/17/2017 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:
> 17.11.2017 20:20, John Snow wrote:
>>
>> 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 <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?
>>> "
>>>
>>>
>> 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.
> 
> Hm, if you remember, reusing "frozen" state was strange for me too. And
> it's not
> late to move to
> 1. make a new state: MIGRATION, which disallows qmp operations on bitmap
> 2. or just make them unnamed, so they can't be touched by qmp

"Migrating" is fine as a state name. You could probably announce this by
having it be "frozen" in the usual way (it has a successor) and a new
bool that lets you do whatever special handling you need to do for it.

> 
> anything is ok for me as well as leaving it as is. It's all little
> things, the core is patch 10.
> 
> "frozen" sounds like unchanged, but user will see dirty-count changing
> in query-block.

I guess it's a strange misnomer now... or maybe just always was a bad
name, since it's not really "frozen" but rather "locked" in a way that
the QMP user cannot interfere with it -- but it's still a live,
functioning object.

> 

I'm remembering what I was talking about, but I think my preference is
illustrably worse. I was trying to avoid boolean bloat by advocating
re-use, but the re-use is kind of confusing...

I think I was hoping that a bitmap on the receiving end could simply be
"frozen" in the usual way, in that it has a successor recording writes.

I think the way you want to handle it though is with different semantics
for cleanup and recovery which makes it not quite the same state, which
needs either a new bool or some such.

Go with what you think is cleanest at this point, including if you want
to leave it alone. I don't want to cause you to respin it over bikeshedding.

--js