Re: [PATCH v2 6/8] block: do not acquire AioContext in check_to_replace_node

[Paolo Bonzini]

On 19/04/21 10:55, Emanuele Giuseppe Esposito wrote:

+ *
+ * Called with AioContext lock held.



... for @to_replace_bs.


  aio_context_acquire(replace_aio_context);
+if (!check_to_replace_node(bs, to_replace_bs, replaces, errp)) {


A release is missing here.

Paolo


+return;
+}
  replace_size = bdrv_getlength(to_replace_bs);

[PATCH v2 6/8] block: do not acquire AioContext in check_to_replace_node

[Emanuele Giuseppe Esposito]

Make the (only) caller do it.

Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Paolo Bonzini 
Signed-off-by: Emanuele Giuseppe Esposito 
---
 block.c   | 22 +-
 blockdev.c|  7 ++-
 include/block/block.h |  1 +
 3 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index f40fb63c75..fd12f88062 100644
--- a/block.c
+++ b/block.c
@@ -6776,24 +6776,15 @@ bool bdrv_recurse_can_replace(BlockDriverState *bs,
  *
  * The result (whether the node can be replaced or not) is only valid
  * for as long as no graph or permission changes occur.
+ *
+ * Called with AioContext lock held.
  */
 BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+BlockDriverState *to_replace_bs,
 const char *node_name, Error **errp)
 {
-BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
-AioContext *aio_context;
-
-if (!to_replace_bs) {
-error_setg(errp, "Failed to find node with node-name='%s'", node_name);
-return NULL;
-}
-
-aio_context = bdrv_get_aio_context(to_replace_bs);
-aio_context_acquire(aio_context);
-
 if (bdrv_op_is_blocked(to_replace_bs, BLOCK_OP_TYPE_REPLACE, errp)) {
-to_replace_bs = NULL;
-goto out;
+return NULL;
 }
 
 /* We don't want arbitrary node of the BDS chain to be replaced only the 
top
@@ -6806,12 +6797,9 @@ BlockDriverState *check_to_replace_node(BlockDriverState 
*parent_bs,
"because it cannot be guaranteed that doing so would not "
"lead to an abrupt change of visible data",
node_name, parent_bs->node_name);
-to_replace_bs = NULL;
-goto out;
+return NULL;
 }
 
-out:
-aio_context_release(aio_context);
 return to_replace_bs;
 }
 
diff --git a/blockdev.c b/blockdev.c
index a57590aae4..e901107344 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3056,13 +3056,18 @@ static void blockdev_mirror_common(const char *job_id, 
BlockDriverState *bs,
 return;
 }
 
-to_replace_bs = check_to_replace_node(bs, replaces, errp);
+to_replace_bs = bdrv_find_node(replaces);
 if (!to_replace_bs) {
+error_setg(errp, "Failed to find node with node-name='%s'",
+   replaces);
 return;
 }
 
 replace_aio_context = bdrv_get_aio_context(to_replace_bs);
 aio_context_acquire(replace_aio_context);
+if (!check_to_replace_node(bs, to_replace_bs, replaces, errp)) {
+return;
+}
 replace_size = bdrv_getlength(to_replace_bs);
 aio_context_release(replace_aio_context);
 
diff --git a/include/block/block.h b/include/block/block.h
index b3f6e509d4..f57c34a19a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -474,6 +474,7 @@ int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts 
*opts,
 
 /* check if a named node can be replaced when doing drive-mirror */
 BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+BlockDriverState *to_replace_bs,
 const char *node_name, Error **errp);
 
 /* async block I/O */
-- 
2.30.2