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

[John Snow]

On 7/16/19 11:26 AM, John Snow wrote:
> 
> 
> On 7/16/19 2:32 AM, Markus Armbruster wrote:
>> 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?
>>
> 
> query-block (or is it block-query? Well, you know the one.)
> 
> It's the optional *dirty-bitmaps field. It's present when there are
> bitmaps attached to the root of the device.
> 
>>> 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?
>>
> 
> @BlockDirtyInfo structure, four bools, a deprecated enum, uint32 and an in.
> 
> However, there can be any number of them. Possibly very many. If you
> have 30 of them on the root node, adding their output to the correct
> node means you will now see 60 bitmaps reported. (Augh.)
> 
> However, see below, if you add them to a node that doesn't qualify for
> this top-level output, you'll only see them once.
> 
> [Incremental backup paradigm: only one per backup chain.
>  Pull-mode checkpoint paradigm, at least n+1 bitmaps for n checkpoints.]
> 
>>> 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?
>>
> 
> The BlockDirtyInfo is present when there are dirty bitmaps attached to
> the root node of the "device".
> 
> (This special case, bitmaps attached to the "root" of a "device", may or
> may not work completely correctly with anonymous BlockBackend structures
> and other non-sugared syntaxes which seemingly create "special" trees
> that cannot be replicated precisely with QMP commands. I forget the
> particulars at the present moment.)
> 
>>> 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?
>>
> 
> 1. Since it was introduced.
> 
> It's just that most of our use cases revolved around bitmaps being
> attached in this way. With blockdev-backup and more flexible backup
> modes, we might be creating bitmaps on nodes that aren't traditional
> drives, and we need to be able to query those. We have no way to do so
> in 4.1.
> 
> So we have had the ability to create mysterious, unqueriable bitmaps for
> quite some time; since 2.4 at least.
> 
> 
> 2. Harder to answer. Yes, iotests 124 and 256 and 257 and others look
> for bitmaps via block-query. These are all the "named" style bitmaps,
> which I added in 2.4-ish timeframe. They've always been in this weird place.
> 
> However, dirty-bitmaps field has existed even before then, and was used
> for communicating information about the implicit dirty bitmap used by
> the mirror job. Paolo added that in 1.x, a long long time ago.
> 
> 21b56835086 (Nov 13 2013) changed this field from "dirty" to
> "dirty-bitmaps" with no justification for the break in compatibility.
> The "dirty" field was the one used exclusively for mirror job's dirty
> information, and this commit and series added support for named bitmaps,
> and is now used primarily user-created, named bitmap objects. (But it
> still reports the old, anonymous implicit type for mirror.)
> 
> Are there any users for this other kind of bitmap? Completely unknown.
> 
> I don't think we use it in iotests, and I don't see evidence of libvirt
> using it. (I just checked with pkrempa and he doesn't seem to think it's
> used that way either.)
> 
>>> 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.
>>
> 

Update: I convinced myself I'm wrong, because even if the *output* isn't
always present, it's enough that the field is present via introspection.

So I'll resend my other version of this patch without the feature flag.

--js