[v3,1/5] block: Allow replacement of a BDS by its overlay
diff mbox

Message ID 20160610185750.30956-2-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz June 10, 2016, 6:57 p.m. UTC
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(-)

Comments

Fam Zheng June 12, 2016, 3:15 a.m. UTC | #1
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
>
Kevin Wolf June 13, 2016, 3:13 p.m. UTC | #2
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

Patch
diff mbox

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);