Re: [Qemu-block] [PATCH v2] block/dirty-bitmap: Documentation and Comment fixups

[John Snow]

On 2/11/19 1:25 PM, Vladimir Sementsov-Ogievskiy wrote:
> 04.02.2019 22:26, John Snow wrote:
>>
>>
>> On 2/4/19 10:00 AM, Eric Blake wrote:
>>> On 2/1/19 7:10 PM, John Snow wrote:
>>>> The meaning of the states has changed subtly over time,
>>>> this should bring the understanding more in-line with the
>>>> current, actual usages.
>>>>
>>>> Reported-by: Eric Blake <address@hidden>
>>>> Signed-off-by: John Snow <address@hidden>
>>>>
>>>> ---
>>>> V2: Amended some wordings, though this is a little chatty now.
>>>>      Hopefully it's clearer, though.
>>>> ---
>>>
>>>>   /**
>>>> - * A BdrvDirtyBitmap can be in three possible states:
>>>> - * (1) successor is NULL and disabled is false: full r/w mode
>>>> - * (2) successor is NULL and disabled is true: read only mode ("disabled")
>>>> - * (3) successor is set: frozen mode.
>>>> - *     A frozen bitmap cannot be renamed, deleted, anonymized, cleared, 
>>>> set,
>>>> - *     or enabled. A frozen bitmap can only abdicate() or reclaim().
>>>> + * A BdrvDirtyBitmap can be in four possible user-visible states:
>>>> + * (1) Active:   successor is NULL, and disabled is false: full r/w mode
>>>> + * (2) Disabled: successor is NULL, and disabled is true: qualified r/w 
>>>> mode,
>>>> + *               guest writes are dropped, but monitor writes are 
>>>> possible,
>>>> + *               through commands like merge and clear.
>>>> + * (3) Frozen:   successor is not NULL.
>>>> + *               A frozen bitmap cannot be renamed, deleted, cleared, set,
>>>> + *               enabled, merged to, etc. A frozen bitmap can only 
>>>> abdicate()
>>>> + *               or reclaim().
>>>> + *               In this state, the anonymous successor bitmap may be 
>>>> either
>>>> + *               Active and recording writes from the guest (e.g. backup 
>>>> jobs),
>>>> + *               but it can be Disabled and not recording writes.
>>>
>>> s/but/or/
>>>
>>
>> I'll touch that up.
>>
>>>> + * (4) Locked:   Whether Active or Disabled, the user cannot modify this 
>>>> bitmap
>>>> + *               in any way from the monitor.
>>>
>>> Worth a parenthetical comment mentioning NBD export as a possible reason
>>> for Locked to be visible?
>>>
>>
>> Since this is effectively developer documentation, yes. How about:
>>
>> "Whether Active or Disabled, the user cannot modify this bitmap in any
>> way from the monitor. This mode is used to achieve a successor-less
>> locking of a bitmap for operations like point-in-time NBD export or for
>> incoming migration data."
>>
>>> With the one word fix,
>>> Reviewed-by: Eric Blake <address@hidden>
>>>
>>
>> If Vladimir agrees with these phrasings I'll stage it.
>>
> 
> Ooops, sorry for the long delay.
> 
> Honestly, I still don't like it very much, as is worded in manner that bitmap
> may be in two states (Active and Locked), and it is not very good for 
> something
> called "state".. But anyway it is understandable and a lot better than what 
> it was, so:
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> 

Yeah, understood -- I think the phrasings here make it clear that we
want to change how we track this. It's not ideal right now but I think
this explains what we already have as best as I can summarize it.

Thanks for your feedback.

--js