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

Reply via email to