[Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list

[Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list

[Alberto Garcia]

I've been debugging a couple of problems related to the recently
merged bdrv_reopen() overhaul code.

1. bs->children is not updated correctly
----------------------------------------
The problem is described in this e-mail:

   https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06813.html

In short, changing an image's backing hd does not replace the pointer
in the bs->children list.

The children list is a feature added recently in 6e93e7c41fdfdee30.
In addition to bs->backing_hd and bs->file it also includes other
driver-specific children for cases like Quorum.

However there's no way to remove items from that list. It seems that
this was discussed when the patch was first published, but no one saw
a case where this could break:

   https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02284.html

The problem is that it can break: the block-stream command removes a
BDS's backing image (optionally replacing it with a new one), so the
pointer in bs->children becomes invalid.

I wrote a patch that updates bs->children when bdrv_set_backing_hd()
is called. It also makes sure that the same children is not added
twice to the same parent (that can happen due to bdrv_set_backing_hd()
being called in bdrv_open_backing_file()).

I think this is enough to solve this problem, but I haven't checked
all other cases of chidren added using bdrv_attach_child(). Anyway the
assumption that once a BDS is added to that list it will always be
valid seems very broad to me.


2. bdrv_reopen_queue() includes the complete backing chain
----------------------------------------------------------
Calling bdrv_reopen() on a particular BlockDriverState now adds its
whole backing chain to the queue (formerly I think it was just
bs->file).

I don't know why this behavior is necessary, but there are surely
things that I'm overlooking.

However this breaks one of the features of my intermediate block
streaming patchset: the ability to start several block-stream
operations in parallel as long as the affected chains don't overlap.

This breaks iotest 030, as described here:

   https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06273.html

Now, this feature was just a nice side effect of the ability to stream
to intermediate images, and is of secondary importance to me; so if I
can no longer assume that bdrv_reopen() is not going to touch the
whole backing chain, I can just remove it very easily and still leave
the main functionality intact.

Comments are welcome.

Thanks,

Berto

Alberto Garcia (1):
  block: update BlockDriverState's children in bdrv_set_backing_hd()

 block.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Alberto Garcia]

When a backing image is opened using bdrv_open_inherit(), it is added
to the parent image's list of children. However there's no way to
remove it from there.

In particular, changing a BlockDriverState's backing image does not
add the new one to the list nor removes the old one. If the latter is
closed then the pointer in the list becomes invalid. This can be
reproduced easily using the block-stream command.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Cc: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 7e130cc..eaf3ad0 100644
--- a/block.c
+++ b/block.c
@@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
                              const BdrvChildRole *child_role,
                              BlockDriver *drv, Error **errp);
 
+static void bdrv_attach_child(BlockDriverState *parent_bs,
+                              BlockDriverState *child_bs,
+                              const BdrvChildRole *child_role);
+
+static void bdrv_detach_child(BlockDriverState *parent_bs,
+                              BlockDriverState *child_bs);
+
 static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
 /* If non-zero, use only whitelisted block drivers */
 static int use_bdrv_whitelist;
@@ -1108,6 +1115,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     if (bs->backing_hd) {
         assert(bs->backing_blocker);
         bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
+        bdrv_detach_child(bs, bs->backing_hd);
     } else if (backing_hd) {
         error_setg(&bs->backing_blocker,
                    "node is used as backing hd of '%s'",
@@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
         bs->backing_blocker = NULL;
         goto out;
     }
+
+    bdrv_attach_child(bs, backing_hd, &child_backing);
+    backing_hd->inherits_from = bs;
+    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);
+
     bs->open_flags &= ~BDRV_O_NO_BACKING;
     pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
     pstrcpy(bs->backing_format, sizeof(bs->backing_format),
@@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
                               BlockDriverState *child_bs,
                               const BdrvChildRole *child_role)
 {
-    BdrvChild *child = g_new(BdrvChild, 1);
+    BdrvChild *child;
+
+    /* Don't attach the child if it's already attached */
+    QLIST_FOREACH(child, &parent_bs->children, next) {
+        if (child->bs == child_bs) {
+            return;
+        }
+    }
+
+    child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
         .bs     = child_bs,
         .role   = child_role,
@@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
     QLIST_INSERT_HEAD(&parent_bs->children, child, next);
 }
 
+static void bdrv_detach_child(BlockDriverState *parent_bs,
+                              BlockDriverState *child_bs)
+{
+    BdrvChild *child, *next_child;
+    QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) {
+        if (child->bs == child_bs) {
+            if (child->bs->inherits_from == parent_bs) {
+                child->bs->inherits_from = NULL;
+            }
+            QLIST_REMOVE(child, next);
+            g_free(child);
+        }
+    }
+}
+
 /*
  * Opens a disk image (raw, qcow2, vmdk, ...)
  *
@@ -2116,7 +2153,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     /* The contents of 'tmp' will become bs_top, as we are
      * swapping bs_new and bs_top contents. */
     bdrv_set_backing_hd(bs_top, bs_new);
-    bdrv_attach_child(bs_top, bs_new, &child_backing);
 }
 
 static void bdrv_delete(BlockDriverState *bs)
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Max Reitz]

On 01.07.2015 16:21, Alberto Garcia wrote:
> When a backing image is opened using bdrv_open_inherit(), it is added
> to the parent image's list of children. However there's no way to
> remove it from there.
>
> In particular, changing a BlockDriverState's backing image does not
> add the new one to the list nor removes the old one. If the latter is
> closed then the pointer in the list becomes invalid. This can be
> reproduced easily using the block-stream command.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/block.c b/block.c
> index 7e130cc..eaf3ad0 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>                                const BdrvChildRole *child_role,
>                                BlockDriver *drv, Error **errp);
>
> +static void bdrv_attach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs,
> +                              const BdrvChildRole *child_role);
> +
> +static void bdrv_detach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs);
> +
>   static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>   /* If non-zero, use only whitelisted block drivers */
>   static int use_bdrv_whitelist;
> @@ -1108,6 +1115,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>       if (bs->backing_hd) {
>           assert(bs->backing_blocker);
>           bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> +        bdrv_detach_child(bs, bs->backing_hd);
>       } else if (backing_hd) {
>           error_setg(&bs->backing_blocker,
>                      "node is used as backing hd of '%s'",
> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>           bs->backing_blocker = NULL;
>           goto out;
>       }
> +
> +    bdrv_attach_child(bs, backing_hd, &child_backing);
> +    backing_hd->inherits_from = bs;
> +    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);

Do we really want this, unconditionally? ... After looking through the 
code, I can't find a place where we wouldn't. It just seems strange to 
have it here.

> +
>       bs->open_flags &= ~BDRV_O_NO_BACKING;
>       pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
>       pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
>                                 BlockDriverState *child_bs,
>                                 const BdrvChildRole *child_role)
>   {
> -    BdrvChild *child = g_new(BdrvChild, 1);
> +    BdrvChild *child;
> +
> +    /* Don't attach the child if it's already attached */
> +    QLIST_FOREACH(child, &parent_bs->children, next) {
> +        if (child->bs == child_bs) {
> +            return;
> +        }
> +    }

Hm, it may have been attached with a different role, though... I guess 
that would be a bug, however. But if it's the same role, trying to 
re-attach it seems wrong, too. So where could this happen?

Max

> +
> +    child = g_new(BdrvChild, 1);
>       *child = (BdrvChild) {
>           .bs     = child_bs,
>           .role   = child_role,
> @@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
>       QLIST_INSERT_HEAD(&parent_bs->children, child, next);
>   }
>
> +static void bdrv_detach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs)
> +{
> +    BdrvChild *child, *next_child;
> +    QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) {
> +        if (child->bs == child_bs) {
> +            if (child->bs->inherits_from == parent_bs) {
> +                child->bs->inherits_from = NULL;
> +            }
> +            QLIST_REMOVE(child, next);
> +            g_free(child);
> +        }
> +    }
> +}
> +
>   /*
>    * Opens a disk image (raw, qcow2, vmdk, ...)
>    *
> @@ -2116,7 +2153,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>       /* The contents of 'tmp' will become bs_top, as we are
>        * swapping bs_new and bs_top contents. */
>       bdrv_set_backing_hd(bs_top, bs_new);
> -    bdrv_attach_child(bs_top, bs_new, &child_backing);
>   }
>
>   static void bdrv_delete(BlockDriverState *bs)
>

Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Alberto Garcia]

On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <mreitz@redhat.com> wrote:

>> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>>           bs->backing_blocker = NULL;
>>           goto out;
>>       }
>> +
>> +    bdrv_attach_child(bs, backing_hd, &child_backing);
>> +    backing_hd->inherits_from = bs;
>> +    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);
>
> Do we really want this, unconditionally? ... After looking through the
> code, I can't find a place where we wouldn't. It just seems strange to
> have it here.

Yeah, I understand. In general I think that the API for handling
bs->children is rather unclear and I wanted to avoid that callers need
to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately.

>> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
>>                                 BlockDriverState *child_bs,
>>                                 const BdrvChildRole *child_role)
>>   {
>> -    BdrvChild *child = g_new(BdrvChild, 1);
>> +    BdrvChild *child;
>> +
>> +    /* Don't attach the child if it's already attached */
>> +    QLIST_FOREACH(child, &parent_bs->children, next) {
>> +        if (child->bs == child_bs) {
>> +            return;
>> +        }
>> +    }
>
> Hm, it may have been attached with a different role, though... I guess
> that would be a bug, however. But if it's the same role, trying to
> re-attach it seems wrong, too. So where could this happen?

The reason I'm doing this is because of bdrv_open_backing_file(). That
function attaches the backing file to the parent file twice: once in
bdrv_open_inherit() and the second time in bdrv_set_backing_hd().

One alternative would be not to attach it in bdrv_set_backing_hd(), but
since that function is used in many other places we would have to add
new calls to bdrv_attach_child() everywhere.

That's one example of the situation I mentioned earlier: it seems
logical that bdrv_set_backing_hd() and bdrv_attach_child() go together,
but none of the solutions that came to my mind feels 100% right.

Berto

Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Max Reitz]

On 01.07.2015 21:41, Alberto Garcia wrote:
> On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>
>>> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>>>            bs->backing_blocker = NULL;
>>>            goto out;
>>>        }
>>> +
>>> +    bdrv_attach_child(bs, backing_hd, &child_backing);
>>> +    backing_hd->inherits_from = bs;
>>> +    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);
>>
>> Do we really want this, unconditionally? ... After looking through the
>> code, I can't find a place where we wouldn't. It just seems strange to
>> have it here.
>
> Yeah, I understand. In general I think that the API for handling
> bs->children is rather unclear and I wanted to avoid that callers need
> to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately.

Oh, sorry, I was unclear. The bdrv_attach_child() is fine, but I find 
unconditionally inheriting the flags from the backed BDS strange.

>>> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
>>>                                  BlockDriverState *child_bs,
>>>                                  const BdrvChildRole *child_role)
>>>    {
>>> -    BdrvChild *child = g_new(BdrvChild, 1);
>>> +    BdrvChild *child;
>>> +
>>> +    /* Don't attach the child if it's already attached */
>>> +    QLIST_FOREACH(child, &parent_bs->children, next) {
>>> +        if (child->bs == child_bs) {
>>> +            return;
>>> +        }
>>> +    }
>>
>> Hm, it may have been attached with a different role, though... I guess
>> that would be a bug, however. But if it's the same role, trying to
>> re-attach it seems wrong, too. So where could this happen?
>
> The reason I'm doing this is because of bdrv_open_backing_file(). That
> function attaches the backing file to the parent file twice: once in
> bdrv_open_inherit() and the second time in bdrv_set_backing_hd().

Okay, that's fine then.

> One alternative would be not to attach it in bdrv_set_backing_hd(), but
> since that function is used in many other places we would have to add
> new calls to bdrv_attach_child() everywhere.
>
> That's one example of the situation I mentioned earlier: it seems
> logical that bdrv_set_backing_hd() and bdrv_attach_child() go together,
> but none of the solutions that came to my mind feels 100% right.

I think putting it into bdrv_set_backing_hd() is fine.

Still feeling a bit bad about overwriting the backing BDS's flags and 
making it inherit its flags from the backed BDS in 
bdrv_set_backing_hd(), but anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>

(I do think it is fine and can't think of any better solution)

Re: [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd()

[Kevin Wolf]

Am 01.07.2015 um 16:21 hat Alberto Garcia geschrieben:
> When a backing image is opened using bdrv_open_inherit(), it is added
> to the parent image's list of children. However there's no way to
> remove it from there.
> 
> In particular, changing a BlockDriverState's backing image does not
> add the new one to the list nor removes the old one. If the latter is
> closed then the pointer in the list becomes invalid. This can be
> reproduced easily using the block-stream command.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Cc: Kevin Wolf <kwolf@redhat.com>

I think I have a fix for this as part of a larger series I've been
working on before I left for holidays. I intended to send it out before
that, but I couldn't get it finished in time.

http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/bdrv_swap
Commit cde08581 'block: Fix backing file child when modifying graph'

It seems cleaner to me than this patch, though I haven't tried yet
to split the series so that it could be applied to 2.4.

>  block.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 7e130cc..eaf3ad0 100644
> --- a/block.c
> +++ b/block.c
> @@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
>                               const BdrvChildRole *child_role,
>                               BlockDriver *drv, Error **errp);
>  
> +static void bdrv_attach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs,
> +                              const BdrvChildRole *child_role);
> +
> +static void bdrv_detach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs);
> +
>  static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>  /* If non-zero, use only whitelisted block drivers */
>  static int use_bdrv_whitelist;
> @@ -1108,6 +1115,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>      if (bs->backing_hd) {
>          assert(bs->backing_blocker);
>          bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
> +        bdrv_detach_child(bs, bs->backing_hd);
>      } else if (backing_hd) {
>          error_setg(&bs->backing_blocker,
>                     "node is used as backing hd of '%s'",
> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
>          bs->backing_blocker = NULL;
>          goto out;
>      }
> +
> +    bdrv_attach_child(bs, backing_hd, &child_backing);
> +    backing_hd->inherits_from = bs;
> +    backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags);

This looks wrong to me. Attaching a BDS as a backing file doesn't mean
that the (potentially explicitly set) options and flags for that BDS
should be changed. It's perfectly fine for children to not inherit
options from their parent.

>      bs->open_flags &= ~BDRV_O_NO_BACKING;
>      pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
>      pstrcpy(bs->backing_format, sizeof(bs->backing_format),
> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
>                                BlockDriverState *child_bs,
>                                const BdrvChildRole *child_role)
>  {
> -    BdrvChild *child = g_new(BdrvChild, 1);
> +    BdrvChild *child;
> +
> +    /* Don't attach the child if it's already attached */
> +    QLIST_FOREACH(child, &parent_bs->children, next) {
> +        if (child->bs == child_bs) {
> +            return;
> +        }
> +    }

In theory, it could be valid to attach the same BDS for two different
roles of the same parent.

> +    child = g_new(BdrvChild, 1);
>      *child = (BdrvChild) {
>          .bs     = child_bs,
>          .role   = child_role,
> @@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState *parent_bs,
>      QLIST_INSERT_HEAD(&parent_bs->children, child, next);
>  }
>  
> +static void bdrv_detach_child(BlockDriverState *parent_bs,
> +                              BlockDriverState *child_bs)
> +{
> +    BdrvChild *child, *next_child;
> +    QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) {
> +        if (child->bs == child_bs) {
> +            if (child->bs->inherits_from == parent_bs) {
> +                child->bs->inherits_from = NULL;
> +            }
> +            QLIST_REMOVE(child, next);
> +            g_free(child);
> +        }
> +    }
> +}

For the same reason, BlockDriverState *child_bs is a bad interface. My
patches use BdrvChild *child instead.

Kevin

Re: [Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list

[Markus Armbruster]

Alberto Garcia <berto@igalia.com> writes:

> I've been debugging a couple of problems related to the recently
> merged bdrv_reopen() overhaul code.
>
> 1. bs->children is not updated correctly
> ----------------------------------------
> The problem is described in this e-mail:
>
>    https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06813.html
>
> In short, changing an image's backing hd does not replace the pointer
> in the bs->children list.
>
> The children list is a feature added recently in 6e93e7c41fdfdee30.
> In addition to bs->backing_hd and bs->file it also includes other
> driver-specific children for cases like Quorum.

I was involved in discussions leading up to the children list, but I
couldn't find the time to review actual patches, so I only have
high-level remarks at this time.

Obvious: the BDS graph is linked together with pointers.

Two pointers are special: BDS members @backing_hd and @file.  By
convention:

1. COW drivers use @file for the delta and @backing_hd for the base.

   Example: "qcow2" uses them that way.

2. Non-COW drivers don't use @backing_hd (it remains null), and they may
   use @file.

   Example: "raw" uses @file to point to its underlying BDS.
   Example: "file" doesn't use @file (it remains null).

3. If a driver needs children that don't fit the above, it has suitable
   members in its private state.

   Example "blkverify" has a member @test_file.

Due to 3., generic code cannot find all children.  This is unfortunate.

Possible solutions:

A. Driver callbacks to help iterate over children.

B. Change the data structure so that generic code can iterate over
   children.

C. Augment the data structure so that generic code can iterate over
   children.

Solution A seems relatively straightforward to do, but clumsy.  B is a
lot of code churn, and the result might be less clear, say bs->child[0]
and bs->child[1] instead of bs->file and bs->backing_hd.  C begs
consistency questions.

Our children list is C.  How do we ensure consistency?

One way to make ensuring it easier is another level of indirection: have
the children list nodes point to the child pointer instead of the child.
Then we need to mess with the children list only initially, and when
child pointers get born or die.

Without that, we need to mess with it when child pointers change.  We
can require all changes to go through a helper function that updates the
children list.  Perhaps something like

    void bdrv_set_child_internal(BlockDriverState *bs, size_t offs,
                                 BlockDriverState *new_child)
    {
        BlockDriverState **child_ptr = (BlockDriverState **)((char *)bs + offs);

        if (*child_ptr) {
            remove *child_ptr from bs->children
        }
        *child_ptr = new_child
        if (new_child) {
            add *child_ptr to bs->children
        }
    }

    #define bdrv_set_child(bs, member, new_child) \
        bdrv_set_child_internal(bs, offsetof(bs, member), new_child)
    // build time assertion bs->member is of type BlockDriverState * omitted

Caution: when the same child occurs multiple times, this may destroy any
association of "actual child pointer" with "position in child list".

Aside: why is it a children list and not an array?  Are members deleted
or inserted that often?

> However there's no way to remove items from that list. It seems that
> this was discussed when the patch was first published, but no one saw
> a case where this could break:
>
>    https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg02284.html
>
> The problem is that it can break: the block-stream command removes a
> BDS's backing image (optionally replacing it with a new one), so the
> pointer in bs->children becomes invalid.

Anything changing child pointers, be it @file, @backing_hd or in private
state, must update the children list consistently.

> I wrote a patch that updates bs->children when bdrv_set_backing_hd()
> is called.

Sounds like a correct approach for updating @backing_hd.  What about the
other child pointers?

>            It also makes sure that the same children is not added
> twice to the same parent (that can happen due to bdrv_set_backing_hd()
> being called in bdrv_open_backing_file()).

As Kevin pointed out, the same child can legitimately occur multiple
times in the children list.

> I think this is enough to solve this problem, but I haven't checked
> all other cases of chidren added using bdrv_attach_child(). Anyway the
> assumption that once a BDS is added to that list it will always be
> valid seems very broad to me.
>
>
> 2. bdrv_reopen_queue() includes the complete backing chain
> ----------------------------------------------------------
> Calling bdrv_reopen() on a particular BlockDriverState now adds its
> whole backing chain to the queue (formerly I think it was just
> bs->file).

I'm not sure I understand.  Can you give an example with before and
after behavior?

> I don't know why this behavior is necessary, but there are surely
> things that I'm overlooking.
>
> However this breaks one of the features of my intermediate block
> streaming patchset: the ability to start several block-stream
> operations in parallel as long as the affected chains don't overlap.
>
> This breaks iotest 030, as described here:
>
>    https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06273.html
>
> Now, this feature was just a nice side effect of the ability to stream
> to intermediate images, and is of secondary importance to me; so if I
> can no longer assume that bdrv_reopen() is not going to touch the
> whole backing chain, I can just remove it very easily and still leave
> the main functionality intact.

Does your patch address this in any way?

> Comments are welcome.
>
> Thanks,
>
> Berto
>
> Alberto Garcia (1):
>   block: update BlockDriverState's children in bdrv_set_backing_hd()
>
>  block.c | 40 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 38 insertions(+), 2 deletions(-)

Re: [Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list

[Kevin Wolf]

Am 23.07.2015 um 08:47 hat Markus Armbruster geschrieben:
> Alberto Garcia <berto@igalia.com> writes:
> 
> > I've been debugging a couple of problems related to the recently
> > merged bdrv_reopen() overhaul code.
> >
> > 1. bs->children is not updated correctly
> > ----------------------------------------
> > The problem is described in this e-mail:
> >
> >    https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06813.html
> >
> > In short, changing an image's backing hd does not replace the pointer
> > in the bs->children list.
> >
> > The children list is a feature added recently in 6e93e7c41fdfdee30.
> > In addition to bs->backing_hd and bs->file it also includes other
> > driver-specific children for cases like Quorum.
> 
> I was involved in discussions leading up to the children list, but I
> couldn't find the time to review actual patches, so I only have
> high-level remarks at this time.
> 
> Obvious: the BDS graph is linked together with pointers.
> 
> Two pointers are special: BDS members @backing_hd and @file.  By
> convention:
> 
> 1. COW drivers use @file for the delta and @backing_hd for the base.
> 
>    Example: "qcow2" uses them that way.
> 
> 2. Non-COW drivers don't use @backing_hd (it remains null), and they may
>    use @file.
> 
>    Example: "raw" uses @file to point to its underlying BDS.
>    Example: "file" doesn't use @file (it remains null).
> 
> 3. If a driver needs children that don't fit the above, it has suitable
>    members in its private state.
> 
>    Example "blkverify" has a member @test_file.
> 
> Due to 3., generic code cannot find all children.  This is unfortunate.
> 
> Possible solutions:
> 
> A. Driver callbacks to help iterate over children.
> 
> B. Change the data structure so that generic code can iterate over
>    children.
> 
> C. Augment the data structure so that generic code can iterate over
>    children.
> 
> Solution A seems relatively straightforward to do, but clumsy.  B is a
> lot of code churn, and the result might be less clear, say bs->child[0]
> and bs->child[1] instead of bs->file and bs->backing_hd.  C begs
> consistency questions.
> 
> Our children list is C.  How do we ensure consistency?

By doing B.

> One way to make ensuring it easier is another level of indirection: have
> the children list nodes point to the child pointer instead of the child.
> Then we need to mess with the children list only initially, and when
> child pointers get born or die.

My series does it the other way round: bs->backing_file becomes a
pointer to BdrvChild.

> Without that, we need to mess with it when child pointers change.  We
> can require all changes to go through a helper function that updates the
> children list.  Perhaps something like
> 
>     void bdrv_set_child_internal(BlockDriverState *bs, size_t offs,
>                                  BlockDriverState *new_child)
>     {
>         BlockDriverState **child_ptr = (BlockDriverState **)((char *)bs + offs);
> 
>         if (*child_ptr) {
>             remove *child_ptr from bs->children
>         }
>         *child_ptr = new_child
>         if (new_child) {
>             add *child_ptr to bs->children
>         }
>     }
> 
>     #define bdrv_set_child(bs, member, new_child) \
>         bdrv_set_child_internal(bs, offsetof(bs, member), new_child)
>     // build time assertion bs->member is of type BlockDriverState * omitted
> 
> Caution: when the same child occurs multiple times, this may destroy any
> association of "actual child pointer" with "position in child list".

This looks much too error-prone to me. We have to avoid duplication.

> Aside: why is it a children list and not an array?  Are members deleted
> or inserted that often?

Not often, but not all at the same time either. Implementation detail,
feel free to convert it to reallocating an array each time.

> > 2. bdrv_reopen_queue() includes the complete backing chain
> > ----------------------------------------------------------
> > Calling bdrv_reopen() on a particular BlockDriverState now adds its
> > whole backing chain to the queue (formerly I think it was just
> > bs->file).
> 
> I'm not sure I understand.  Can you give an example with before and
> after behavior?

Backing chain: base <- snap

Start with cache=none set for snap. base inherits the option. Reopen
with cache=writeback. Before the change, base remained at cache=none,
afterwards it inherits the new setting cache=writeback.

As we defined that reopen should adjust the inherited options of child
nodes accordingly, this was a bug fix.

Kevin