John Snow <js...@redhat.com> writes: > On 6/5/19 8:46 AM, Markus Armbruster wrote: >> John Snow <js...@redhat.com> 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.