Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow]

On 6/5/19 8:46 AM, Markus Armbruster wrote:
> John Snow <address@hidden> writes:
> 
>> On 5/31/19 10:55 AM, Eric Blake wrote:
>>> On 5/30/19 11:26 AM, John Snow wrote:
>>>>
>>>>
>>>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>>
>>>
>>>>> +++ b/block/qapi.c
>>>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend 
>>>>> *blk,
>>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>>      }
>>>>>  
>>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>>> +        info->has_dirty_bitmaps = true;
>>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>>> +    }
>>>>> +
>>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>>  
>>>>>      if (blk && 
>>>>> blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>>>
>>>>
>>>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>>>> so we'll duplicate the bitmap output when doing the old-fashioned block
>>>> query, but that's probably harmless overall.
>>>
>>> We already know that none of our existing query- interfaces are sane
>>> (either too little information, or too much).  Duplication starts to
>>> push an interface towards too much (it takes processor time to bundle up
>>> the extra JSON, especially if the other end is not going to care if it
>>> was present). I know Kevin still has somewhere on his to-do list the
>>> implementation of a saner query- command for the information we really
>>> want (about each block, without redundant information, and where we
>>> don't repeat information in a nested manner, but where we also don't
>>> omit information that would otherwise require multiple existing query-
>>> to reconstruct).
>>>
>>>>
>>>> We can continue to support the output in both places, or we could opt to
>>>> deprecate the older interface; I think this is one of the last chances
>>>> we'd get to do so before libvirt and wider adoption.
>>>>
>>>> I think that's probably Eric's choice.
>>>
>>> If you want to try to deprecate the old location, introspection at least
>>> works to allow libvirt to know which place to look for it on a given
>>> qemu. If you don't think deprecation is necessary, the duplication is
>>> probably tolerable for now (as ideally we'd be deprecating ALL of our
>>> not-quite-perfect query- block interfaces in favor of whatever sane
>>> interface Kevin comes up with).
>>>
>>
>> It sounds like it's probably the right move to deprecate the entire
>> legacy interface, but still... If you have 20 or 30 bitmaps on a root
>> node, you will see 40 or 60 entries.
>>
>> What's the smart way to deprecate it? We're not adding new flags or
>> showing new arguments or anything. There might not be bitmaps, so you
>> can't rely on that field being present or absent.
>>
>> Recommendations?
> 
> Kevin's "[PATCH v4 0/6] file-posix: Add dynamic-auto-read-only QAPI
> feature" adds "feature flags" to the QAPI schema language, limited to
> struct types, because that's what he needs.  They're visible in
> introspection.  I intend to complete his work, so we can tack
> "deprecated" feature flags to pretty much anything
> 
> Could that address your need?
> 

Hi Markus, digging this up again.

In brief, we are displaying bitmap info in the "wrong" part of the query
result (attached to drive instead of node) and would like to change it.
I'd like to avoid reporting bitmaps in both locations permanently, so if
we have a plan to deprecate reporting bitmaps in the old location, I
will tolerate the duplicated output temporarily.

Keeping in mind the bitmap fields are optional (so they can be absent
from both the new and old locations), what plan can we implement?

Perhaps I can add a feature flag "has-node-bitmaps" for 4.2. Then, for
the next three versions, I will report bitmaps from both locations.
Then, in 5.2+ I will remove the old location.

A client knows it can find bitmaps (if there are any) in the new
location if the feature flag is set. Otherwise, it should look in the
old location.

I think I've convinced myself that this is correct, so correct me if I
am wrong.

--js