Re: [PATCH v3 22/36] block: add bdrv_remove_filter_or_cow transaction action

[Vladimir Sementsov-Ogievskiy]

26.04.2021 19:26, Kevin Wolf wrote:

Am 17.03.2021 um 15:35 hat Vladimir Sementsov-Ogievskiy geschrieben:

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
  block.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
  1 file changed, 76 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 11f7ad0818..2fca1f2ad5 100644
--- a/block.c
+++ b/block.c
@@ -2929,12 +2929,19 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs)
      }
  }

+static void bdrv_child_free(void *opaque)

+{
+    BdrvChild *c = opaque;
+
+    g_free(c->name);
+    g_free(c);
+}
+
  static void bdrv_remove_empty_child(BdrvChild *child)
  {
      assert(!child->bs);
      QLIST_SAFE_REMOVE(child, next);
-    g_free(child->name);
-    g_free(child);
+    bdrv_child_free(child);
  }

typedef struct BdrvAttachChildCommonState {

@@ -4956,6 +4963,73 @@ static bool should_update_child(BdrvChild *c, 
BlockDriverState *to)
      return ret;
  }

+typedef struct BdrvRemoveFilterOrCowChild {

+    BdrvChild *child;
+    bool is_backing;
+} BdrvRemoveFilterOrCowChild;
+
+/* this doesn't restore original child bs, only the child itself */

Hm, this comment tells me that it's intentional, but why is it correct?

that's because bdrv_remove_filter_or_cow_child_abort() aborts only part of  
bdrv_remove_filter_or_cow_child().

Look: bdrv_remove_filter_or_cow_child() firstly do 
bdrv_replace_child_safe(child, NULL, tran);, so bs would be restored by 
.abort() of bdrv_replace_child_safe() action.


So, improved comment may look like:

This doesn't restore original child bs, only the child itself. The bs would be 
restored by .abort() bdrv_replace_child_safe() subation of 
bdrv_remove_filter_or_cow_child() action.

Probably it would be more correct to rename

BdrvRemoveFilterOrCowChild -> BdrvRemoveFilterOrCowChildNoBs
bdrv_remove_filter_or_cow_child_abort -> 
bdrv_remove_filter_or_cow_child_no_bs_abort
bdrv_remove_filter_or_cow_child_commit -> 
bdrv_remove_filter_or_cow_child_no_bs_commit

and assert on .abort() and .commit() that s->child->bs is NULL.

+static void bdrv_remove_filter_or_cow_child_abort(void *opaque)
+{
+    BdrvRemoveFilterOrCowChild *s = opaque;
+    BlockDriverState *parent_bs = s->child->opaque;
+
+    QLIST_INSERT_HEAD(&parent_bs->children, s->child, next);
+    if (s->is_backing) {
+        parent_bs->backing = s->child;
+    } else {
+        parent_bs->file = s->child;
+    }
+}

Kevin

--
Best regards,
Vladimir