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

[John Snow]

On 5/20/19 6:37 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 <address@hidden>
> 

Great, thanks all :)

I think I will stage this through my tree -- on the premise that David
Gilbert won't want to stage a block patch, and that since it's not
Kevin's tree, he won't mind either.

--js