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 <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
