Re: [Qemu-devel] [PATCH 2/2] qapi: deprecate implicit filters

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

29.08.2019 18:00, Vladimir Sementsov-Ogievskiy wrote:
> 28.08.2019 20:48, John Snow wrote:
>> (Peter: search for "pkrempa" down below.)
>>
>> On 8/28/19 5:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.08.2019 23:12, John Snow wrote:
>>>>
>>>>
>>>> On 8/23/19 5:22 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 14.08.2019 13:07, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> To get rid of implicit filters related workarounds in future let's
>>>>>> deprecate them now.
>>>>>
>>>>> Interesting, could we deprecate implicit filter without deprecation of unnecessity of
>>>>> parameter? As actually, it's good when this parameter is not necessary, in most cases
>>>>> user is not interested in node-name.
>>>>>
>>>>
>>>> https://en.wiktionary.org/wiki/unnecessity -- I am surprised to learn
>>>> that this a real word in the language I speak. :)
>>>>
>>>> I assume you're referring to making the optional argument mandatory.
>>>
>>> exactly, it's my a bit "google-translate-driven" English)
>>>
>>
>> It teaches me some fun words!
>>
>>>>
>>>>> Obviously we can do the following:
>>>>>
>>>>> 1. In 4.2 we deprecate unnecessity, which implies deprecation of implicit filters
>>>>> 2. After some releases in 4.x we can drop deprecated functionality, so we drop it together with
>>>>> implicit filters. And, in same release 4.x we return it back (as it's compatible change :)
>>>>> but without implicit filters (so, if filter-node-name not specified, we just create
>>>>> explicit filter with autogenerated node-name)
>>>>>
>>>>> So, effectively we just drop "deprecation mark" together with implicit filters, which is nice
>>>>> but actually confusing.
>>>>>
>>>>> Instead, we may do
>>>>> 1. In 4.2 deprecate
>>>>> 2. In 4.x drop optionality together with implicit filters
>>>>> 3. In 4.y (y > x of course) return optionality back
>>>>>
>>>>
>>>> Ah, I see what you're digging at here now...
>>>>
>>>>> It's a bit safer, but for users who miss releases [4.x, 4.y) it's no difference..
>>>>>
>>>>> Or we just write in spec, that implicit filters are deprecated? But we have nothing about implicit
>>>>> filters in spec. More over, we directly write that we have filter, and if parameter is omitted
>>>>> it's node-name is autogenerated. So actually, the fact the filter is hidden when filter-node-name is
>>>>> unspecified is _undocumented_.
>>>>>
>>>>> So, finally, it looks like nothing to deprecated in specification, we can just drop implicit filters :)
>>>>>
>>>>> What do you think?
>>>>>
>>>>
>>>> What exactly _IS_ an implicit filter? How does it differ today from an
>>>> explicit filter? I assumed the only difference was if it was named or
>>>> not; but I think I must be mistaken now if you're proposing leaving the
>>>> interface alone entirely.
>>>>
>>>> Are they instantiated differently?
>>>>
>>>
>>> As I understand, the only difference is their BlockDriverState.impicit field, and several places in code
>>> where we skip implicit filter when trying to find something in a chain starting from a device.
>>>
>>
>> Oh, oh, yes. I see.
>>
>>> Hmm, OK, let's see:
>>>
>>> 1. the only implicit filters are commit_top and mirror_top if user don't specify filter-node-name.
>>>
>>> Where it make sense, i.e., where implicit field used?
>>>
>>
>> `git grep -E '(->|\.)implicit'` is what I used to find usages.
>>
>>> 2. bdrv_query_info, bdrv_query_bds_stats, bdrv_block_device_info(only when called from bdrv_query_info), they'll
>>> report filter as top node if we don't mark it implicit.
>>>
>>
>> So that's a bit of a change, but only visually. The "reality" is still
>> the same, we just report it more "accurately." libvirt MIGHT need a
>> heads up here. I'm looping pkrempa back in for comment.
>>
>> <pkrempa>
>> Would libvirt be negatively impacted by the revelation of formerly
>> internal ("implicit") nodes created by mirror and commit via query block
>> commands? At the moment, QEMU hides them from you if you do not name them.
>> </pkrempa>
>>
>>> 3. bdrv_refresh_filename, bdrv_reopen_parse_backing, bdrv_drop_intermediate:
>>>     I think it's not a problem, just drop special case for implicit fitlers
>>>
>>
>> I'm much less certain about what the impact of this would be and would
>> need to audit it (and don't have the time to, personally.)
>>
>> Do you have a POC or RFC patch that demonstrates dropping these special
>> cases? It might be nice to see as proof that it's safe to deprecate.

I faced a problem with some iotest (it's not a surprise of course), but I think, I'll
come back with and RFC of course.

>>
>>> So, seems the only real change is query-block and query-blockstats output when mirror or commit is started
>>> without specifying filter-node-name (filter would be on top)
>>>
>>> So, how should we deprecate this, or can we just change it?
>>>
>>
>> I'm not sure if it's worth it yet, what does dropping the implicit field
>> buy us? Conceptually I understand that it's simpler without the notion
>> of implicit fields, but I imagine there's some cleanup in particular
>> that motivated this.
> 
> Reviewing Max's "block: Deal with filters" series motivated me.


Currently, we just mostly don't care about implicit filters.

your command on master:
# git grep -E '(->|\.)implicit'
block.c:    while (backing_bs(overlay_bs) && backing_bs(overlay_bs)->implicit) {
block.c:        assert(!old_backing_bs || !old_backing_bs->implicit);
block.c:    while (explicit_top && explicit_top->implicit) {
block.c:    if (bs->implicit) {
block/commit.c:        commit_top_bs->implicit = true;
block/mirror.c:        mirror_top_bs->implicit = true;
block/qapi.c:        while (blk && bs0->drv && bs0->implicit) {
block/qapi.c:    while (bs && bs->drv && bs->implicit) {
block/qapi.c:    while (blk_level && bs->drv && bs->implicit) {


on Max's series:

# git grep -E '(->|\.)implicit'
block.c:           bdrv_filtered_bs(overlay_bs)->implicit)
block.c:        assert(!old_backing_bs || !old_backing_bs->implicit);
block.c:    if (bs->implicit) {
block.c:    while (!(stop_on_explicit_filter && !bs->implicit)) {
block/commit.c:        commit_top_bs->implicit = true;
block/mirror.c:        mirror_top_bs->implicit = true;

seems better, but actually, we get new function bdrv_skip_implicit_filters:
# git grep bdrv_skip_implicit_filters
block.c:    explicit_top = bdrv_skip_implicit_filters(explicit_top);
block.c:BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs)
block/block-backend.c:            non_filter = bdrv_skip_implicit_filters(blk->root->bs);
block/qapi.c:            bs0 = bdrv_skip_implicit_filters(bs0);
block/qapi.c:        bs = bdrv_skip_implicit_filters(bs);
block/qapi.c:        bs = bdrv_skip_implicit_filters(bs);
blockdev.c:        bs = bdrv_skip_implicit_filters(blk_bs(blk));
blockdev.c:                bdrv_skip_implicit_filters(source);
blockdev.c:    unfiltered_bs = bdrv_skip_implicit_filters(bs);
blockdev.c:        BlockDriverState *explicit_backing = bdrv_skip_implicit_filters(source);
blockdev.c:    unfiltered_bs = bdrv_skip_implicit_filters(bs);
include/block/block_int.h:BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);


So, if we are going to really support implicit filters, we have to support them everywhere.
I'm not sure that Max's series covered all cases. We don't have strong definition of implicit
filters, or about have implicit and explicit filters should be interleaved on appending.

That's why I think it's better to drop them earlier, as it seems that we are going to increase
filter usage in block-layer.

> 
>>
>> I'd say to just change the behavior, we should:
>>
>> - Give a standard three-release warning that the behavior will change in
>> an incompatible way
>> - Demonstrate with an RFC patch that special cases around ->implicit in
>> block.c can be removed and do not make the code more complex,
>> - Get blessings from Peter Krempa.
>>
>> As always: Libvirt is not the end-all be-all of QEMU management, but if
>> libvirt is capable of working around design changes then I believe any
>> project out there today also could, so it's a good litmus test.
>>
>> --js
>>
> 
> 


-- 
Best regards,
Vladimir