Re: [Qemu-devel] [Qemu-block] [PATCH v2] migration/dirty-bitmaps: change bitmap enumeration method

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

20.05.2019 12:27, Kevin Wolf wrote:
> Am 17.05.2019 um 12:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 16.05.2019 22:03, John Snow wrote:
>>> On 5/16/19 6:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> But, on the other, hand, if we have implicitly-filtered node on target, we were doing wrong thing anyway,
>>>> as dirty_bitmap_load_header don't skip implicit nodes.
>>>>
>>>>> +    for (bs = bdrv_next_all_states(NULL); bs; bs = bdrv_next_all_states(bs)) {
>>>>
>>>> As I understand, difference with bdrv_next_node is that we don't skip unnamed nodes [...]
>>>>
>>>
>>> The difference is that we iterate over states that aren't roots of
>>> trees; so not just unnamed nodes but rather intermediate nodes as well
>>> as nodes not attached to a storage device.
>>>
>>> bdrv_next says: `Iterates over all top-level BlockDriverStates, i.e.
>>> BDSs that are owned by the monitor or attached to a BlockBackend`
>>>
>>> So this loop is going to iterate over *everything*, not just top-level
>>> nodes. This lets me skip the tree-crawling loop that didn't work quite
>>> right.
>>
>> I meant not bdrv_next but bdrv_next_node, which iterates through graph nodes..
>> What is real difference between graph_bdrv_states and all_bdrv_states ?
> 
> I don't think there is any relevant difference any more since commit
> 15489c769b9 ('block: auto-generated node-names'). We can only see a
> difference if a BDS was created, but never opened. This means either
> that we are still in the process of opening an image or that we have a
> bug somewhere.
> 
> Maybe we should remove graph_bdrv_states and replace all of its uses
> with the more obviously correct all_bdrv_states (if we are sure that
> all users aren't called between creating and opening a BDS).
> 
>> Node is inserted to graph_bdrv_states in bdrv_assign_node_name(), and to
>> all_bdrv_states in bdrv_new().
>>
>> Three calls to bdrv_new:
>>
>> bdrv_new_open_driver, calls bdrv_new and then bdrv_open_driver, which calls bdrv_assign_node_name,
>> and if it fails new created node is released.
>>
>> bdrv_open_inherit
>>      bdrv_new
>>      ..
>>      bdrv_open_common
>>         bdrv_open_driver
>>             bdrv_assign_node_name
>>
>>
>> iscsi_co_create_opts
>>      bdrv_new
>>
>>      ... hmm.. looks like it a kind of temporary unnamed bs
>>
>> So, now, I'm not sure. Possibly we'd better use bdrv_next_node().
> 
> I wonder if the iscsi one can't be replaced with bdrv_new_open_driver().
> Manually building a half-opened BDS like it does currently looks
> suspicious.
> 
>> Kevin introduced all_bdrv_states in 0f12264e7a4145 , to use in drain instead of
>> bdrv_next... But I don't understand, why he didn't use graph_bdrv_states and
>> corresponding bdrv_next_node(), which is only skips some temporary or under-constuction
>> nodes..
> 
> I probably just didn't realise that graph_bdrv_states exists and is
> effectively the same. I knew that all_bdrv_states contains what I want,
> so I just wanted to access that.
> 
> But if anonymous BDSes did actually still exist, drain would want to
> wait for those as well, so it's the more natural choice anyway.
> 
> Kevin
> 

Thank you for clarification. Then, my r-b still stand here, and fixing/refacting
graph_nodes vs all_states should be a separate thing.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir