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
