| Message ID | 20180411185425.2461-7-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/mirror: Add active-sync mirroring | expand |
On Wed 11 Apr 2018 08:54:18 PM CEST, Max Reitz wrote: > Currently, bdrv_replace_node() refuses to create loops from one BDS to > itself if the BDS to be replaced is the backing node of the BDS to > replace it: Say there is a node A and a node B. Replacing B by A means > making all references to B point to A. If B is a child of A (i.e. A has > a reference to B), that would mean we would have to make this reference > point to A itself -- so we'd create a loop. > > bdrv_replace_node() (through should_update_child()) refuses to do so if > B is the backing node of A. There is no reason why we should create > loops if B is not the backing node of A, though. The BDS graph should > never contain loops, so we should always refuse to create them. > > If B is a child of A and B is to be replaced by A, we should simply > leave B in place there because it is the most sensible choice. > > A more specific argument would be: Putting filter drivers into the BDS > graph is basically the same as appending an overlay to a backing chain. > But the main child BDS of a filter driver is not "backing" but "file", > so restricting the no-loop rule to backing nodes would fail here. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > Reviewed-by: Fam Zheng <famz@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
On Wed 11 Apr 2018 08:54:18 PM CEST, Max Reitz wrote: > index c4dd1d4bb8..8d63a1b0c1 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -616,6 +616,8 @@ struct BdrvChild { > QLIST_ENTRY(BdrvChild) next_parent; > }; > > +typedef QLIST_HEAD(BdrvChildList, BdrvChild) BdrvChildList; > + I forgot to mention this in a previous e-mail, but what's this used for? Berto
On 2018-04-20 10:36, Alberto Garcia wrote: > On Wed 11 Apr 2018 08:54:18 PM CEST, Max Reitz wrote: >> index c4dd1d4bb8..8d63a1b0c1 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -616,6 +616,8 @@ struct BdrvChild { >> QLIST_ENTRY(BdrvChild) next_parent; >> }; >> >> +typedef QLIST_HEAD(BdrvChildList, BdrvChild) BdrvChildList; >> + > > I forgot to mention this in a previous e-mail, but what's this used for? Extremely good question. grep says it isn't used for anything. I'm inclined to believe grep (especially considering the fact that everything compiles without error after removing it). I suppose I had some local version where I needed such a type (probably because I didn't deem QLIST_FOREACH_SAFE() to be sufficiently safe, so I needed to copy the list somewhere else, or I don't know...), and then I found a way around it, removed the code, but forgot the type in the header. Will remove in v5. Thank you for reviewing! Max
diff --git a/include/block/block_int.h b/include/block/block_int.h index c4dd1d4bb8..8d63a1b0c1 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -616,6 +616,8 @@ struct BdrvChild { QLIST_ENTRY(BdrvChild) next_parent; }; +typedef QLIST_HEAD(BdrvChildList, BdrvChild) BdrvChildList; + /* * Note: the function bdrv_append() copies and swaps contents of * BlockDriverStates, so if you add new fields to this struct, please diff --git a/block.c b/block.c index a2caadf0a0..9294b89eb7 100644 --- a/block.c +++ b/block.c @@ -3383,16 +3383,39 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to) return false; } - if (c->role == &child_backing) { - /* If @from is a backing file of @to, ignore the child to avoid - * creating a loop. We only want to change the pointer of other - * parents. */ - QLIST_FOREACH(to_c, &to->children, next) { - if (to_c == c) { - break; - } - } - if (to_c) { + /* If the child @c belongs to the BDS @to, replacing the current + * c->bs by @to would mean to create a loop. + * + * Such a case occurs when appending a BDS to a backing chain. + * For instance, imagine the following chain: + * + * guest device -> node A -> further backing chain... + * + * Now we create a new BDS B which we want to put on top of this + * chain, so we first attach A as its backing node: + * + * node B + * | + * v + * guest device -> node A -> further backing chain... + * + * Finally we want to replace A by B. When doing that, we want to + * replace all pointers to A by pointers to B -- except for the + * pointer from B because (1) that would create a loop, and (2) + * that pointer should simply stay intact: + * + * guest device -> node B + * | + * v + * node A -> further backing chain... + * + * In general, when replacing a node A (c->bs) by a node B (@to), + * if A is a child of B, that means we cannot replace A by B there + * because that would create a loop. Silently detaching A from B + * is also not really an option. So overall just leaving A in + * place there is the most sensible choice. */ + QLIST_FOREACH(to_c, &to->children, next) { + if (to_c == c) { return false; } } @@ -3418,6 +3441,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, /* Put all parents into @list and calculate their cumulative permissions */ QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { + assert(c->bs == from); if (!should_update_child(c, to)) { continue; }