Re: [Qemu-devel] [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

24.10.2016 20:30, Max Reitz wrote:
> On 24.10.2016 17:12, Vladimir Sementsov-Ogievskiy wrote:
>> 10.10.2016 19:08, Max Reitz wrote:
>>> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
>>>
>> [...]
>>
>>>> +    }
>>>>       out:
>>>>        aio_context_release(aio_context);
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 31f9990..2bf56cd 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -1235,10 +1235,15 @@
>>>>    # @granularity: #optional the bitmap granularity, default is 64k for
>>>>    #               block-dirty-bitmap-add
>>>>    #
>>>> +# @persistent: #optional the bitmap is persistent, i.e. it will be
>>>> saved to
>>>> +#              corresponding block device on it's close. Default is
>>>> false.
>>>> +#              For block-dirty-bitmap-add. (Since 2.8)
>>> I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
>>> because this whole struct is for block-dirty-bitmap-add (and for
>>> block-dirty-bitmap-add transactions, to be exact, but @persistent will
>>> surely work there, too, won't it?).
>>>
>>> Also, I'd say "will be saved to the corresponding block device image
>>> file" instead of just "block device", because in my understanding a
>>> block device and its image file are two separate things.
>> Hmm.. But 'its close' is block device close, not file close.
> In my understanding, it is the file close.
>
>>                                                               And we call
>> common bdrv_* function to save it, so we actually save it to device, and
>> then the device puzzles out, how to actually save it.
> Well, OK, it depends on what you mean by "block device". There are many
> things we call a "block device", but nowadays I think it mostly refers
> to either a guest block device or a BlockBackend (and as of lately,
> we're more and more trying to hide the BlockBackend from the user, so
> you could argue that it's only the guest device now).
>
> The bdrv_* functions operate on block layer BDS nodes, and I don't think
> we call them "block devices" (at least not any more).
>
> In any case, I think for users the term "block device" refers to either
> the device presented to the guest or to all of the block layer stuff
> that's underneath, and it's not quite clear how you could save a bitmap
> to that, or what it's supposed to mean to "close" a block device (you
> can remove it, you can destroy it, you can delete it, but "close" it?).
>
> But saying that it will be saved to the image file that is attached to
> the block device will make it absolutely clear what we mean.
>
> So what you have called a "device" here is neither what I'd call a
> device (I'd call it a "node" or "BDS"), nor what I think users would
> call a device. Also, it's not the BDS that puzzles out how to save it
> but some block driver.
>

Ok, thank you.

>>>> +#
>>>>    # Since 2.4
>>>>    ##
>>>>    { 'struct': 'BlockDirtyBitmapAdd',
>>>> -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
>>>> +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>>>> +  '*persistent': 'bool' } }
>>> I think normally we'd align the new line so that the opening ' of
>>> '*persistent' is under the opening ' of 'node'.
>>>
>>>>      ##
>>>>    # @block-dirty-bitmap-add
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index ba2a916..434b418 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -1441,7 +1441,7 @@ EQMP
>>>>          {
>>>>            .name       = "block-dirty-bitmap-add",
>>>> -        .args_type  = "node:B,name:s,granularity:i?",
>>>> +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
>>>>            .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
>>>>        },
>>>>    @@ -1458,6 +1458,9 @@ Arguments:
>>>>    - "node": device/node on which to create dirty bitmap (json-string)
>>>>    - "name": name of the new dirty bitmap (json-string)
>>>>    - "granularity": granularity to track writes with (int, optional)
>>>> +- "persistent": bitmap will be saved to corresponding block device
>>>> +                on it's close. Block driver should maintain
>>>> persistent bitmaps
>>>> +                (json-bool, optional, default false) (Since 2.8)
>>> And I don't know what the user is supposed to make of the information
>>> that block drivers will take care of maintaining persistent bitmaps. All
>>> they care about is that it will be stored in the corresponding image
>>> file, so in my opinion it would be better to just omit the last sentence
>>> here.
>> Users shoud know, that only qcow2 supports persistent bitmaps. Instead
>> of last sentence I can write "For now only Qcow2 disks supports
>> persistent bitmaps".
> s/supports/support/, but yes, that sounds preferable to me.
>
> Max
>


-- 
Best regards,
Vladimir