Re: [PATCH qemu 1/4] drive-mirror: add support for sync=bitmap mode=never

[Markus Armbruster <armbru@redhat.com>]

Fabian Grünbichler <f.gruenbichler@proxmox.com> writes:

> On October 2, 2020 9:06 am, Markus Armbruster wrote:
>> Fabian Grünbichler <f.gruenbichler@proxmox.com> writes:
>> 
>>> From: John Snow <jsnow@redhat.com>
>>>
>>> This patch adds support for the "BITMAP" sync mode to drive-mirror and
>>> blockdev-mirror. It adds support only for the BitmapSyncMode "never,"
>>> because it's the simplest mode.
>>>
>>> This mode simply uses a user-provided bitmap as an initial copy
>>> manifest, and then does not clear any bits in the bitmap at the
>>> conclusion of the operation.
>>>
>>> Any new writes dirtied during the operation are copied out, in contrast
>>> to backup. Note that whether these writes are reflected in the bitmap
>>> at the conclusion of the operation depends on whether that bitmap is
>>> actually recording!
>>>
>>> This patch was originally based on one by Ma Haocong, but it has since
>>> been modified pretty heavily.
>>>
>>> Suggested-by: Ma Haocong <mahaocong@didichuxing.com>
>>> Signed-off-by: Ma Haocong <mahaocong@didichuxing.com>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>>> ---
>> [...]
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 2d94873ca0..dac5497084 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -1961,10 +1961,19 @@
>>>  #        (all the disk, only the sectors allocated in the topmost image, or
>>>  #        only new I/O).
>>>  #
>>> +# @bitmap: The name of a bitmap to use for sync=bitmap mode. This argument must
>>> +#          be present for bitmap mode and absent otherwise. The bitmap's
>> 
>> What is "bitmap mode"?  Do you mean "present when @bitmap-mode is, else
>> absent"?
>
> bitmap mode is sync=bitmap , as in the first sentence. if you set 
> sync=bitmap, you must specify a bitmap and a bitmap-mode. if you use 
> another sync mode, you must not specify a bitmap or a bitmap-mode.

Got it.

> there is also a 'sugar' sync mode 'incremental' that desugars to 
> sync=bitmap with bitmap-mode=on-success.  I guess that should also be 
> mentioned somewhere in QAPI, it's mainly there since MirrorSyncMode has 
> it as possible value, it's semantics are straight-forward to map onto 
> this combination, and it's how the sync modes are known from backup 
> jobs.
>
> maybe the following is easier to understand and more aligned with 
> bitmap-mode:
>
> The name of the bitmap to use for sync=bitmap/sync=incremental mode. 
> Must be present if sync is "bitmap" or "incremental". Must NOT be 
> present otherwise.

Better.

>>> +#          granularity is used instead of @granularity (since 5.2).
>>> +#
>>> +# @bitmap-mode: Specifies the type of data the bitmap should contain after
>>> +#               the operation concludes. Must be present if sync is "bitmap".
>>> +#               Must NOT be present otherwise. (Since 5.2)
>
> Specifies the type of data the bitmap should contain after the operation 
> concludes. Must be present if sync is "bitmap". Must be "on-success" or 
> absent if sync is "incremental". Must NOT be present otherwise.

Thanks for closing the gaps.

>>> +#
>>>  # @granularity: granularity of the dirty bitmap, default is 64K
>>>  #               if the image format doesn't have clusters, 4K if the clusters
>>>  #               are smaller than that, else the cluster size.  Must be a
>>> -#               power of 2 between 512 and 64M (since 1.4).
>>> +#               power of 2 between 512 and 64M. Must not be specified if
>>> +#               @bitmap is present (since 1.4).
>>>  #
>> 
>> Is @granularity forbidden with @bitmap because it makes no sense?
>
> yes.
>
>> 
>> If yes, then it doesn't actually default to anything then, does it?
>
> we must use the same granularity as the sync bitmap passed in via 
> 'bitmap', so the caller can't set a different one.

Contradicts the doc comment :)  Shouldn't be hard to fix.

>> Looks like we have
>> 
>>     sync            'bitmap'            anything else
>>     -------------------------------------------------
>>     bitmap          required            forbidden
>>     bitmap-mode     required            forbidden
>>     granularity     forbidden           optional
>> 
>> Correct?
>
> yes. with the addition of sync=incremental as subset of sync=bitmap, as 
> described above.

When you have members that make sense only for certain values of another
member, you should consider (flat) unions.  I'm not sure a flat union
makes sense here, but I wanted to mention it.

>>>  # @buf-size: maximum amount of data in flight from source to
>>>  #            target (since 1.4).
>>> @@ -2002,7 +2011,9 @@
>>>  { 'struct': 'DriveMirror',
>>>    'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
>>>              '*format': 'str', '*node-name': 'str', '*replaces': 'str',
>>> -            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
>>> +            'sync': 'MirrorSyncMode', '*bitmap': 'str',
>>> +            '*bitmap-mode': 'BitmapSyncMode',
>>> +            '*mode': 'NewImageMode',
>>>              '*speed': 'int', '*granularity': 'uint32',
>>>              '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
>>>              '*on-target-error': 'BlockdevOnError',
>> [Same for blockdev-mirror...]