| Message ID | 20160610185750.30956-2-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 06/10 20:57, Max Reitz wrote: > change_parent_backing_link() asserts that the BDS to be replaced is not > used as a backing file. However, we may want to replace a BDS by its > overlay in which case that very link should not be redirected. > > For instance, when doing a sync=none drive-mirror operation, we may have > the following BDS/BB forest before block job completion: > > target > > base <- source <- BlockBackend > > During job completion, we want to establish the source BDS as the > target's backing node: > > target > | > v > base <- source <- BlockBackend > > This makes the target a valid replacement for the source: > > target <- BlockBackend > | > v > base <- source > > Without this modification to change_parent_backing_link() we have to > inject the target into the graph before the source is its backing node, > thus temporarily creating a wrong graph: > > target <- BlockBackend > > base <- source > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index f54bc25..dc76c159 100644 > --- a/block.c > +++ b/block.c > @@ -2224,9 +2224,23 @@ void bdrv_close_all(void) > static void change_parent_backing_link(BlockDriverState *from, > BlockDriverState *to) > { > - BdrvChild *c, *next; > + BdrvChild *c, *next, *to_c; > > QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { > + if (c->role == &child_backing) { > + /* @from is generally not allowed to be a backing file, except for > + * when @to is the overlay. In that case, @from may not be replaced > + * by @to as @to's backing node. */ > + QLIST_FOREACH(to_c, &to->children, next) { > + if (to_c == c) { > + break; > + } > + } Why not just test "c->bs == to" here, why is it necessary to iterate through to->children list? Fam > + if (to_c) { > + continue; > + } > + } > + > assert(c->role != &child_backing); > bdrv_ref(to); > bdrv_replace_child(c, to); > -- > 2.8.3 >
Am 12.06.2016 um 05:15 hat Fam Zheng geschrieben: > On Fri, 06/10 20:57, Max Reitz wrote: > > change_parent_backing_link() asserts that the BDS to be replaced is not > > used as a backing file. However, we may want to replace a BDS by its > > overlay in which case that very link should not be redirected. > > > > For instance, when doing a sync=none drive-mirror operation, we may have > > the following BDS/BB forest before block job completion: > > > > target > > > > base <- source <- BlockBackend > > > > During job completion, we want to establish the source BDS as the > > target's backing node: > > > > target > > | > > v > > base <- source <- BlockBackend > > > > This makes the target a valid replacement for the source: > > > > target <- BlockBackend > > | > > v > > base <- source > > > > Without this modification to change_parent_backing_link() we have to > > inject the target into the graph before the source is its backing node, > > thus temporarily creating a wrong graph: > > > > target <- BlockBackend > > > > base <- source > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > --- > > block.c | 16 +++++++++++++++- > > 1 file changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/block.c b/block.c > > index f54bc25..dc76c159 100644 > > --- a/block.c > > +++ b/block.c > > @@ -2224,9 +2224,23 @@ void bdrv_close_all(void) > > static void change_parent_backing_link(BlockDriverState *from, > > BlockDriverState *to) > > { > > - BdrvChild *c, *next; > > + BdrvChild *c, *next, *to_c; > > > > QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { > > + if (c->role == &child_backing) { > > + /* @from is generally not allowed to be a backing file, except for > > + * when @to is the overlay. In that case, @from may not be replaced > > + * by @to as @to's backing node. */ > > + QLIST_FOREACH(to_c, &to->children, next) { > > + if (to_c == c) { > > + break; > > + } > > + } > > Why not just test "c->bs == to" here, why is it necessary to iterate through > to->children list? Because c->bs == from. We want to check whether 'to' is the parent, not whether it is the child. Kevin
diff --git a/block.c b/block.c index f54bc25..dc76c159 100644 --- a/block.c +++ b/block.c @@ -2224,9 +2224,23 @@ void bdrv_close_all(void) static void change_parent_backing_link(BlockDriverState *from, BlockDriverState *to) { - BdrvChild *c, *next; + BdrvChild *c, *next, *to_c; QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) { + if (c->role == &child_backing) { + /* @from is generally not allowed to be a backing file, except for + * when @to is the overlay. In that case, @from may not be replaced + * by @to as @to's backing node. */ + QLIST_FOREACH(to_c, &to->children, next) { + if (to_c == c) { + break; + } + } + if (to_c) { + continue; + } + } + assert(c->role != &child_backing); bdrv_ref(to); bdrv_replace_child(c, to);
change_parent_backing_link() asserts that the BDS to be replaced is not used as a backing file. However, we may want to replace a BDS by its overlay in which case that very link should not be redirected. For instance, when doing a sync=none drive-mirror operation, we may have the following BDS/BB forest before block job completion: target base <- source <- BlockBackend During job completion, we want to establish the source BDS as the target's backing node: target | v base <- source <- BlockBackend This makes the target a valid replacement for the source: target <- BlockBackend | v base <- source Without this modification to change_parent_backing_link() we have to inject the target into the graph before the source is its backing node, thus temporarily creating a wrong graph: target <- BlockBackend base <- source Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)