A couple of problems with BlockDriverState's children list

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 <[email protected]>

Cc Kevin Wolf <[email protected]>

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

block.c +8 -0

index 7e130cc..eaf3ad0 100644
@@ -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'",

block.c +1 -0

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

block.c +17 -1

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

Quoted 4 lines

block.c +0 -0

@@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
          bs->backing_blocker = NULL;
          goto out;
      }

block.c void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) · line 1134

Quoted 4 lines

block.c +4 -0

+
+    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.

Quoted 9 lines

block.c +4 -1

@@ -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) {

block.c static void bdrv_attach_child(BlockDriverState *parent_bs, · line 1355

Quoted 4 lines

block.c +4 -0

+        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?

Quoted 4 lines · Max Reitz

Quoted 4 lines

block.c +0 -0

@@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
           bs->backing_blocker = NULL;
           goto out;
       }

Quoted 8 lines

block.c void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) · line 1134

Quoted 4 lines

block.c +4 -0

+
+    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.

Quoted 9 lines

Quoted 9 lines

block.c +4 -1

@@ -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) {

Quoted 8 lines

block.c static void bdrv_attach_child(BlockDriverState *parent_bs, · line 1355

Quoted 4 lines

block.c +4 -0

+        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.

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 <[email protected]>

Cc Kevin Wolf <[email protected]>

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

block.c +8 -0

index 7e130cc..eaf3ad0 100644
@@ -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'",

block.c +0 -0

     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)

block.c +5 -0

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

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

Quoted 7 lines · 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:

Quoted 8 lines

   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?

Quoted 5 lines

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?