| Message ID | 20220314131854.2202651-5-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: bug fixes in preparation of AioContext removal | expand |
Unfortunately this patch is not safe: theoretically ->detach can call bdrv_unapply_subtree_drain, and if it polls, will can call a bh that for example reads the graph, finding it in an inconsistent state, since it is between the two writes QLIST_REMOVE(child, next_parent); and QLIST_REMOVE(child, next); Please ignore it. This patch could eventually go in the subtree_drain serie, if we decide to go in that direction. Emanuele Am 14/03/2022 um 14:18 schrieb Emanuele Giuseppe Esposito: > Doing the opposite can make ->detach() (more precisely > bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain > just performed to protect the removal of the child from the graph, > thus making the fully-enabled assert_bdrv_graph_writable fail. > > Note that assert_bdrv_graph_writable is not yet fully enabled. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index 372f16f4a0..d870ba5393 100644 > --- a/block.c > +++ b/block.c > @@ -1448,6 +1448,11 @@ static void bdrv_child_cb_attach(BdrvChild *child) > bdrv_apply_subtree_drain(child, bs); > } > > +/* > + * Unapply the drain in the whole child subtree, as > + * it has been already detached, and then remove from > + * child->opaque->children. > + */ > static void bdrv_child_cb_detach(BdrvChild *child) > { > BlockDriverState *bs = child->opaque; > @@ -2864,14 +2869,18 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, > } > > if (old_bs) { > - /* Detach first so that the recursive drain sections coming from @child > - * are already gone and we only end the drain sections that came from > - * elsewhere. */ > + /* First remove child from child->bs->parents list */ > + assert_bdrv_graph_writable(old_bs); > + QLIST_REMOVE(child, next_parent); > + /* > + * Then call ->detach() cb. > + * In child_of_bds case, update the child parent > + * (child->opaque) ->children list and > + * remove any drain left in the child subtree. > + */ > if (child->klass->detach) { > child->klass->detach(child); > } > - assert_bdrv_graph_writable(old_bs); > - QLIST_REMOVE(child, next_parent); > } > > child->bs = new_bs; >
diff --git a/block.c b/block.c index 372f16f4a0..d870ba5393 100644 --- a/block.c +++ b/block.c @@ -1448,6 +1448,11 @@ static void bdrv_child_cb_attach(BdrvChild *child) bdrv_apply_subtree_drain(child, bs); } +/* + * Unapply the drain in the whole child subtree, as + * it has been already detached, and then remove from + * child->opaque->children. + */ static void bdrv_child_cb_detach(BdrvChild *child) { BlockDriverState *bs = child->opaque; @@ -2864,14 +2869,18 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, } if (old_bs) { - /* Detach first so that the recursive drain sections coming from @child - * are already gone and we only end the drain sections that came from - * elsewhere. */ + /* First remove child from child->bs->parents list */ + assert_bdrv_graph_writable(old_bs); + QLIST_REMOVE(child, next_parent); + /* + * Then call ->detach() cb. + * In child_of_bds case, update the child parent + * (child->opaque) ->children list and + * remove any drain left in the child subtree. + */ if (child->klass->detach) { child->klass->detach(child); } - assert_bdrv_graph_writable(old_bs); - QLIST_REMOVE(child, next_parent); } child->bs = new_bs;
Doing the opposite can make ->detach() (more precisely bdrv_unapply_subtree_drain() in bdrv_child_cb_detach) undo the subtree_drain just performed to protect the removal of the child from the graph, thus making the fully-enabled assert_bdrv_graph_writable fail. Note that assert_bdrv_graph_writable is not yet fully enabled. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)