Message ID | 20160606144212.24074-2-mreitz@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Am 06.06.2016 um 16:42 hat Max Reitz geschrieben: > 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. > > Signed-off-by: Max Reitz <mreitz@redhat.com> So the scenario is like this? +---- to v base <- from <- top And we want to change it into this: base <- from <- to <- top Okay, makes sense. (Hm, put ASCII art in the commit message? I'd be all for it.) > block.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/block.c b/block.c > index f54bc25..16463aa 100644 > --- a/block.c > +++ b/block.c > @@ -2224,9 +2224,22 @@ 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) { > + /* Allow @from to be in a backing chain, but only if it is @to's > + * backing chain. Do not replace @from by @to there. */ The comment suggests bdrv_chain_contains(), but you only accept it as a direct child of top and you accept non-backing-file children. Intuitively I would say that anywhere in the backing chain would make sense, but we can always allow that later when we actually need it. Accepting all types of children sounds right when I think about inserting a quorum node as 'to'. So I guess the code is fine and the comment should be changed to correctly reflect what the code does. > + 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); The other thing is that I'm unsure whether this function makes any sense at all. "Replace in all parents" is kind of arbitrary. In the long term, we may want to allow the user to specify the exact graph modifications on (block-)job-complete. Kevin
On 08.06.2016 10:58, Kevin Wolf wrote: > Am 06.06.2016 um 16:42 hat Max Reitz geschrieben: >> 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. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> > > So the scenario is like this? > > +---- to > v > base <- from <- top > > And we want to change it into this: > > base <- from <- to <- top > > Okay, makes sense. Doesn't the assert(c->role != &child_backing) prevent this scenario? (which is the reason for the first sentence in my commit message.) So the scenario is rather: +----- target v base <- source <- BB to: base <- source <- target <- BB > (Hm, put ASCII art in the commit message? I'd be all for it.) Well, depending on whether a v3 will come or not, I can put it there, of course. > >> block.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/block.c b/block.c >> index f54bc25..16463aa 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2224,9 +2224,22 @@ 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) { >> + /* Allow @from to be in a backing chain, but only if it is @to's >> + * backing chain. Do not replace @from by @to there. */ > > The comment suggests bdrv_chain_contains(), but you only accept it as a > direct child of top and you accept non-backing-file children. > > Intuitively I would say that anywhere in the backing chain would make > sense, but we can always allow that later when we actually need it. > Accepting all types of children sounds right when I think about > inserting a quorum node as 'to'. > > So I guess the code is fine and the comment should be changed to > correctly reflect what the code does. OK, will do. >> + 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); > > The other thing is that I'm unsure whether this function makes any sense > at all. "Replace in all parents" is kind of arbitrary. In the long term, > we may want to allow the user to specify the exact graph modifications > on (block-)job-complete. Well, as a replacement of bdrv_swap(), this function definitely does make sense. Whether we can do even better in the long run... Is a question for the long run, I think. Max
diff --git a/block.c b/block.c index f54bc25..16463aa 100644 --- a/block.c +++ b/block.c @@ -2224,9 +2224,22 @@ 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) { + /* Allow @from to be in a backing chain, but only if it is @to's + * backing chain. Do not replace @from by @to there. */ + 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. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)