From af295ac08ffca4efd6f10a2d1a38eaa8d09d8e6f Mon Sep 17 00:00:00 2001 From: Fiona Ebner Date: Fri, 30 May 2025 17:10:51 +0200 Subject: [PATCH 26/33] block: move drain outside of quorum_add_child() RH-Author: Kevin Wolf RH-MergeRequest: 393: block: do not drain while holding the graph lock RH-Jira: RHEL-88561 RH-Acked-by: Stefan Hajnoczi RH-Acked-by: Hanna Czenczek RH-Commit: [14/21] 1079c7996e39d09d021d210d974522fba2e5515d (kmwolf/centos-qemu-kvm) This is part of resolving the deadlock mentioned in commit "block: move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK". The quorum_add_child() callback runs under the graph lock, so it is not allowed to drain. It is only called as the .bdrv_add_child() callback, which is only called in the bdrv_add_child() function, which also runs under the graph lock. The bdrv_add_child() function is called by qmp_x_blockdev_change(), where a drained section is introduced. Signed-off-by: Fiona Ebner Message-ID: <20250530151125.955508-15-f.ebner@proxmox.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf (cherry picked from commit 0414930d3adfa89299eaea5ce92accab15d9fba5) Signed-off-by: Kevin Wolf --- block.c | 10 ++++++++-- block/quorum.c | 2 -- blockdev.c | 2 ++ include/block/block_int-common.h | 7 +++++++ 4 files changed, 17 insertions(+), 4 deletions(-) diff --git a/block.c b/block.c index 3857f42877..d7425ff971 100644 --- a/block.c +++ b/block.c @@ -8220,8 +8220,10 @@ char *bdrv_dirname(BlockDriverState *bs, Error **errp) } /* - * Hot add/remove a BDS's child. So the user can take a child offline when - * it is broken and take a new child online + * Hot add a BDS's child. Used in combination with bdrv_del_child, so the user + * can take a child offline when it is broken and take a new child online. + * + * All block nodes must be drained. */ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, Error **errp) @@ -8261,6 +8263,10 @@ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp); } +/* + * Hot remove a BDS's child. Used in combination with bdrv_add_child, so the + * user can take a child offline when it is broken and take a new child online. + */ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp) { BdrvChild *tmp; diff --git a/block/quorum.c b/block/quorum.c index ea17b0ec13..ed8ce801ee 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -1096,10 +1096,8 @@ quorum_add_child(BlockDriverState *bs, BlockDriverState *child_bs, Error **errp) /* We can safely add the child now */ bdrv_ref(child_bs); - bdrv_drain_all_begin(); child = bdrv_attach_child(bs, child_bs, indexstr, &child_of_bds, BDRV_CHILD_DATA, errp); - bdrv_drain_all_end(); if (child == NULL) { s->next_child_index--; return; diff --git a/blockdev.c b/blockdev.c index 41b6481c9a..8edd3e7bba 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3525,6 +3525,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child, BlockDriverState *parent_bs, *new_bs = NULL; BdrvChild *p_child; + bdrv_drain_all_begin(); bdrv_graph_wrlock(); parent_bs = bdrv_lookup_bs(parent, parent, errp); @@ -3562,6 +3563,7 @@ void qmp_x_blockdev_change(const char *parent, const char *child, out: bdrv_graph_wrunlock(); + bdrv_drain_all_end(); } BlockJobInfoList *qmp_query_block_jobs(Error **errp) diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 5d34e6a510..8d76b37c03 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -396,6 +396,13 @@ struct BlockDriver { int GRAPH_RDLOCK_PTR (*bdrv_probe_geometry)( BlockDriverState *bs, HDGeometry *geo); + /** + * Hot add a BDS's child. Used in combination with bdrv_del_child, so the + * user can take a child offline when it is broken and take a new child + * online. + * + * All block nodes must be drained. + */ void GRAPH_WRLOCK_PTR (*bdrv_add_child)( BlockDriverState *parent, BlockDriverState *child, Error **errp); -- 2.39.3