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

[Markus Armbruster]

John Snow <address@hidden> writes:

> 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 lack context: which query command, which part of its result?

> 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.

How bulky is the bitmap report?

> 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?

"Optional" is a syntactic thing, which ought to be backed by a semantic
"present iff" condition.  Have we specified such conditions?

> 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.

For how long has the bitmap been in the "wrong" place?  Any known
consumers?

> 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.

Sounds like a valid use of feature flags to me.  However, feature flags
are best used as a last resort.  With answers to my questions, I should
be able to compare the feature flags solution to possible alternatives.