[Qemu-devel] [PATCH 0/4] block: BdrvChildRole.drained_begin/end fixes

[Qemu-devel] [PATCH 0/4] block: BdrvChildRole.drained_begin/end fixes

[Kevin Wolf]

While working on the series that converts block jobs to using a separate
BlockBackend, I noticed that I need to do some drain fixes first. So here they
are.

Patches 1 and 2 are just preparation, patches 3 and 4 are the actual fixes.

Kevin Wolf (4):
  block: Introduce bdrv_replace_child()
  block: Make bdrv_drain() use bdrv_drained_begin/end()
  block: Fix reconfiguring graph with drained nodes
  block: Propagate .drained_begin/end callbacks

 block.c                   | 58 +++++++++++++++++++++++++++++++++------
 block/block-backend.c     |  9 ++++---
 block/io.c                | 69 +++++++++++++++++++++++------------------------
 include/block/block_int.h |  3 ++-
 4 files changed, 90 insertions(+), 49 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] block: Introduce bdrv_replace_child()

[Kevin Wolf]

This adds a common function that is called when attaching a new child to
a parent, removing a child from a parent and when reconfiguring the
graph so that an existing child points to a different node now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 38df365..351344e 100644
--- a/block.c
+++ b/block.c
@@ -1150,18 +1150,32 @@ static int bdrv_fill_options(QDict **options, const char *filename,
     return 0;
 }
 
+static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
+{
+    BlockDriverState *old_bs = child->bs;
+
+    if (old_bs) {
+        QLIST_REMOVE(child, next_parent);
+    }
+    if (new_bs) {
+        QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
+    }
+
+    child->bs = new_bs;
+}
+
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
                                   const BdrvChildRole *child_role)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
-        .bs     = child_bs,
+        .bs     = NULL,
         .name   = g_strdup(child_name),
         .role   = child_role,
     };
 
-    QLIST_INSERT_HEAD(&child_bs->parents, child, next_parent);
+    bdrv_replace_child(child, child_bs);
 
     return child;
 }
@@ -1182,7 +1196,9 @@ static void bdrv_detach_child(BdrvChild *child)
         QLIST_REMOVE(child, next);
         child->next.le_prev = NULL;
     }
-    QLIST_REMOVE(child, next_parent);
+
+    bdrv_replace_child(child, NULL);
+
     g_free(child->name);
     g_free(child);
 }
@@ -2203,10 +2219,8 @@ static void change_parent_backing_link(BlockDriverState *from,
 
     QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
         assert(c->role != &child_backing);
-        c->bs = to;
-        QLIST_REMOVE(c, next_parent);
-        QLIST_INSERT_HEAD(&to->parents, c, next_parent);
         bdrv_ref(to);
+        bdrv_replace_child(c, to);
         bdrv_unref(from);
     }
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] block: Make bdrv_drain() use bdrv_drained_begin/end()

[Kevin Wolf]

Until now, bdrv_drained_begin() used bdrv_drain() internally to drain
the queue. This is kind of backwards and caused quiescing code to be
duplicated because bdrv_drained_begin() had to ensure that no new
requests come in even after bdrv_drain() returns, whereas bdrv_drain()
had to have them because it could be called from other places.

Instead move the bdrv_drain() code to bdrv_drained_begin() and make
bdrv_drain() a simple wrapper around bdrv_drained_begin/end().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 69 ++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 33 insertions(+), 36 deletions(-)

diff --git a/block/io.c b/block/io.c
index 2a28d63..9bc1d45 100644
--- a/block/io.c
+++ b/block/io.c
@@ -225,6 +225,34 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
     assert(data.done);
 }
 
+void bdrv_drained_begin(BlockDriverState *bs)
+{
+    if (!bs->quiesce_counter++) {
+        aio_disable_external(bdrv_get_aio_context(bs));
+        bdrv_parent_drained_begin(bs);
+    }
+
+    bdrv_io_unplugged_begin(bs);
+    bdrv_drain_recurse(bs);
+    if (qemu_in_coroutine()) {
+        bdrv_co_yield_to_drain(bs);
+    } else {
+        bdrv_drain_poll(bs);
+    }
+    bdrv_io_unplugged_end(bs);
+}
+
+void bdrv_drained_end(BlockDriverState *bs)
+{
+    assert(bs->quiesce_counter > 0);
+    if (--bs->quiesce_counter > 0) {
+        return;
+    }
+
+    bdrv_parent_drained_end(bs);
+    aio_enable_external(bdrv_get_aio_context(bs));
+}
+
 /*
  * Wait for pending requests to complete on a single BlockDriverState subtree,
  * and suspend block driver's internal I/O until next request arrives.
@@ -238,26 +266,15 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
  */
 void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
 {
-    bdrv_parent_drained_begin(bs);
-    bdrv_io_unplugged_begin(bs);
-    bdrv_drain_recurse(bs);
-    bdrv_co_yield_to_drain(bs);
-    bdrv_io_unplugged_end(bs);
-    bdrv_parent_drained_end(bs);
+    assert(qemu_in_coroutine());
+    bdrv_drained_begin(bs);
+    bdrv_drained_end(bs);
 }
 
 void bdrv_drain(BlockDriverState *bs)
 {
-    bdrv_parent_drained_begin(bs);
-    bdrv_io_unplugged_begin(bs);
-    bdrv_drain_recurse(bs);
-    if (qemu_in_coroutine()) {
-        bdrv_co_yield_to_drain(bs);
-    } else {
-        bdrv_drain_poll(bs);
-    }
-    bdrv_io_unplugged_end(bs);
-    bdrv_parent_drained_end(bs);
+    bdrv_drained_begin(bs);
+    bdrv_drained_end(bs);
 }
 
 /*
@@ -2541,23 +2558,3 @@ void bdrv_io_unplugged_end(BlockDriverState *bs)
         }
     }
 }
-
-void bdrv_drained_begin(BlockDriverState *bs)
-{
-    if (!bs->quiesce_counter++) {
-        aio_disable_external(bdrv_get_aio_context(bs));
-    }
-    bdrv_parent_drained_begin(bs);
-    bdrv_drain(bs);
-}
-
-void bdrv_drained_end(BlockDriverState *bs)
-{
-    bdrv_parent_drained_end(bs);
-
-    assert(bs->quiesce_counter > 0);
-    if (--bs->quiesce_counter > 0) {
-        return;
-    }
-    aio_enable_external(bdrv_get_aio_context(bs));
-}
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] block: Fix reconfiguring graph with drained nodes

[Kevin Wolf]

When changing the BlockDriverState that a BdrvChild points to while the
node is currently drained, we must call the .drained_end() parent
callback. Conversely, when this means attaching a new node that is
already drained, we need to call .drained_begin().

bdrv_root_attach_child() takes now an opaque parameter, which is needed
because the callbacks must also be called if we're attaching a new child
to the BlockBackend when the root node is already drained, and they need
a way to identify the BlockBackend. Previously, child->opaque was set
too late and the callbacks would still see it as NULL.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 18 ++++++++++++++----
 block/block-backend.c     |  9 +++++----
 include/block/block_int.h |  3 ++-
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 351344e..598624f 100644
--- a/block.c
+++ b/block.c
@@ -1155,24 +1155,33 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
     BlockDriverState *old_bs = child->bs;
 
     if (old_bs) {
+        if (old_bs->quiesce_counter && child->role->drained_end) {
+            child->role->drained_end(child);
+        }
         QLIST_REMOVE(child, next_parent);
     }
+
+    child->bs = new_bs;
+
     if (new_bs) {
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
+        if (new_bs->quiesce_counter && child->role->drained_begin) {
+            child->role->drained_begin(child);
+        }
     }
-
-    child->bs = new_bs;
 }
 
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
-                                  const BdrvChildRole *child_role)
+                                  const BdrvChildRole *child_role,
+                                  void *opaque)
 {
     BdrvChild *child = g_new(BdrvChild, 1);
     *child = (BdrvChild) {
         .bs     = NULL,
         .name   = g_strdup(child_name),
         .role   = child_role,
+        .opaque = opaque,
     };
 
     bdrv_replace_child(child, child_bs);
@@ -1185,7 +1194,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              const char *child_name,
                              const BdrvChildRole *child_role)
 {
-    BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role);
+    BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role,
+                                              NULL);
     QLIST_INSERT_HEAD(&parent_bs->children, child, next);
     return child;
 }
diff --git a/block/block-backend.c b/block/block-backend.c
index 4e8298b..14e528e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -161,8 +161,7 @@ BlockBackend *blk_new_open(const char *filename, const char *reference,
     }
 
     blk_set_enable_write_cache(blk, true);
-    blk->root = bdrv_root_attach_child(bs, "root", &child_root);
-    blk->root->opaque = blk;
+    blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
 
     return blk;
 }
@@ -481,8 +480,7 @@ void blk_remove_bs(BlockBackend *blk)
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
 {
     bdrv_ref(bs);
-    blk->root = bdrv_root_attach_child(bs, "root", &child_root);
-    blk->root->opaque = blk;
+    blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
 
     notifier_list_notify(&blk->insert_bs_notifiers, blk);
     if (blk->public.throttle_state) {
@@ -1676,6 +1674,9 @@ static void blk_root_drained_begin(BdrvChild *child)
 {
     BlockBackend *blk = child->opaque;
 
+    /* Note that blk->root may not be accessible here yet if we are just
+     * attaching to a BlockDriverState that is drained. Use child instead. */
+
     if (blk->public.io_limits_disabled++ == 0) {
         throttle_group_restart_blk(blk);
     }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b6f4755..30a9717 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -719,7 +719,8 @@ void hmp_drive_add_node(Monitor *mon, const char *optstr);
 
 BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const char *child_name,
-                                  const BdrvChildRole *child_role);
+                                  const BdrvChildRole *child_role,
+                                  void *opaque);
 void bdrv_root_unref_child(BdrvChild *child);
 
 const char *bdrv_get_parent_name(const BlockDriverState *bs);
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] block: Propagate .drained_begin/end callbacks

[Kevin Wolf]

When draining intermediate nodes (i.e. nodes that aren't the root node
for at least one of their parents; with node references, the user can
always configure the graph to create this situation), we need to
propagate the .drained_begin/end callbacks all the way up to the root
for the drain to be effective.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 598624f..45cddd6 100644
--- a/block.c
+++ b/block.c
@@ -659,6 +659,18 @@ int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough)
     return 0;
 }
 
+static void bdrv_child_cb_drained_begin(BdrvChild *child)
+{
+    BlockDriverState *bs = child->opaque;
+    bdrv_drained_begin(bs);
+}
+
+static void bdrv_child_cb_drained_end(BdrvChild *child)
+{
+    BlockDriverState *bs = child->opaque;
+    bdrv_drained_end(bs);
+}
+
 /*
  * Returns the options and flags that a temporary snapshot should get, based on
  * the originally requested flags (the originally requested image will have
@@ -705,6 +717,8 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
 
 const BdrvChildRole child_file = {
     .inherit_options = bdrv_inherited_options,
+    .drained_begin   = bdrv_child_cb_drained_begin,
+    .drained_end     = bdrv_child_cb_drained_end,
 };
 
 /*
@@ -723,6 +737,8 @@ static void bdrv_inherited_fmt_options(int *child_flags, QDict *child_options,
 
 const BdrvChildRole child_format = {
     .inherit_options = bdrv_inherited_fmt_options,
+    .drained_begin   = bdrv_child_cb_drained_begin,
+    .drained_end     = bdrv_child_cb_drained_end,
 };
 
 /*
@@ -750,6 +766,8 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
 
 static const BdrvChildRole child_backing = {
     .inherit_options = bdrv_backing_options,
+    .drained_begin   = bdrv_child_cb_drained_begin,
+    .drained_end     = bdrv_child_cb_drained_end,
 };
 
 static int bdrv_open_flags(BlockDriverState *bs, int flags)
@@ -1195,7 +1213,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              const BdrvChildRole *child_role)
 {
     BdrvChild *child = bdrv_root_attach_child(child_bs, child_name, child_role,
-                                              NULL);
+                                              parent_bs);
     QLIST_INSERT_HEAD(&parent_bs->children, child, next);
     return child;
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/4] block: Introduce bdrv_replace_child()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On 05/23/2016 10:55 AM, Kevin Wolf wrote:
> This adds a common function that is called when attaching a new child to
> a parent, removing a child from a parent and when reconfiguring the
> graph so that an existing child points to a different node now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/4] block: Make bdrv_drain() use bdrv_drained_begin/end()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On 05/23/2016 10:55 AM, Kevin Wolf wrote:
> Until now, bdrv_drained_begin() used bdrv_drain() internally to drain
> the queue. This is kind of backwards and caused quiescing code to be
> duplicated because bdrv_drained_begin() had to ensure that no new
> requests come in even after bdrv_drain() returns, whereas bdrv_drain()
> had to have them because it could be called from other places.
> 
> Instead move the bdrv_drain() code to bdrv_drained_begin() and make
> bdrv_drain() a simple wrapper around bdrv_drained_begin/end().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 69 ++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 33 insertions(+), 36 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/4] block: Fix reconfiguring graph with drained nodes

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 05/23/2016 10:55 AM, Kevin Wolf wrote:
> When changing the BlockDriverState that a BdrvChild points to while the
> node is currently drained, we must call the .drained_end() parent
> callback. Conversely, when this means attaching a new node that is
> already drained, we need to call .drained_begin().
> 
> bdrv_root_attach_child() takes now an opaque parameter, which is needed
> because the callbacks must also be called if we're attaching a new child
> to the BlockBackend when the root node is already drained, and they need
> a way to identify the BlockBackend. Previously, child->opaque was set
> too late and the callbacks would still see it as NULL.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 18 ++++++++++++++----
>  block/block-backend.c     |  9 +++++----
>  include/block/block_int.h |  3 ++-
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] block: Propagate .drained_begin/end callbacks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On 05/23/2016 10:55 AM, Kevin Wolf wrote:
> When draining intermediate nodes (i.e. nodes that aren't the root node
> for at least one of their parents; with node references, the user can
> always configure the graph to create this situation), we need to
> propagate the .drained_begin/end callbacks all the way up to the root
> for the drain to be effective.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/4] block: BdrvChildRole.drained_begin/end fixes

[Fam Zheng]

On Mon, 05/23 18:55, Kevin Wolf wrote:
> While working on the series that converts block jobs to using a separate
> BlockBackend, I noticed that I need to do some drain fixes first. So here they
> are.
> 
> Patches 1 and 2 are just preparation, patches 3 and 4 are the actual fixes.
> 
> Kevin Wolf (4):
>   block: Introduce bdrv_replace_child()
>   block: Make bdrv_drain() use bdrv_drained_begin/end()
>   block: Fix reconfiguring graph with drained nodes
>   block: Propagate .drained_begin/end callbacks

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
>  block.c                   | 58 +++++++++++++++++++++++++++++++++------
>  block/block-backend.c     |  9 ++++---
>  block/io.c                | 69 +++++++++++++++++++++++------------------------
>  include/block/block_int.h |  3 ++-
>  4 files changed, 90 insertions(+), 49 deletions(-)
> 
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 0/4] block: BdrvChildRole.drained_begin/end fixes

[Kevin Wolf]

Am 23.05.2016 um 18:55 hat Kevin Wolf geschrieben:
> While working on the series that converts block jobs to using a separate
> BlockBackend, I noticed that I need to do some drain fixes first. So here they
> are.
> 
> Patches 1 and 2 are just preparation, patches 3 and 4 are the actual fixes.

Thanks for the quick reviews! Applied to the block branch.

Kevin