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

[Kevin Wolf]

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