| Message ID | 20220314131854.2202651-6-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: bug fixes in preparation of AioContext removal | expand |
Unfortunately this patch is not safe: theoretically ->attach can call bdrv_apply_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_INSERT_HEAD(&bs->children, child, next); and QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); 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 adding the child node to a non-drained node, > as apply_subtree_drain is only done in ->attach() and thus make > assert_bdrv_graph_writable fail. > > This can happen for example during a transaction rollback (test 245, > test_io_with_graph_changes): > 1. a node is removed from the graph, thus it is undrained > 2. then something happens, and we need to roll back the transactions > through tran_abort() > 3. at this point, the current code would first attach the undrained node > to the graph via QLIST_INSERT_HEAD, and then call ->attach() that > will take care of restoring the drain with apply_subtree_drain(), > leaving the node undrained between the two operations. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/block.c b/block.c > index d870ba5393..c6a550f9c6 100644 > --- a/block.c > +++ b/block.c > @@ -1434,6 +1434,11 @@ static void bdrv_inherited_options(BdrvChildRole role, bool parent_is_format, > *child_flags = flags; > } > > +/* > + * Add the child node to child->opaque->children list, > + * and then apply the drain to the whole child subtree, > + * so that the drain count matches with the parent. > + */ > static void bdrv_child_cb_attach(BdrvChild *child) > { > BlockDriverState *bs = child->opaque; > @@ -2889,8 +2894,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, > } > > if (new_bs) { > - assert_bdrv_graph_writable(new_bs); > - QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); > > /* > * Detaching the old node may have led to the new node's > @@ -2901,12 +2904,19 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, > assert(new_bs->quiesce_counter <= new_bs_quiesce_counter); > drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter; > > - /* Attach only after starting new drained sections, so that recursive > - * drain sections coming from @child don't get an extra .drained_begin > - * callback. */ > + /* > + * First call ->attach() cb. > + * In child_of_bds case, add child to the parent > + * (child->opaque) ->children list and if > + * necessary add missing drains in the child subtree. > + */ > if (child->klass->attach) { > child->klass->attach(child); > } > + > + /* Then add child to new_bs->parents list */ > + assert_bdrv_graph_writable(new_bs); > + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); > } > > /* >
diff --git a/block.c b/block.c index d870ba5393..c6a550f9c6 100644 --- a/block.c +++ b/block.c @@ -1434,6 +1434,11 @@ static void bdrv_inherited_options(BdrvChildRole role, bool parent_is_format, *child_flags = flags; } +/* + * Add the child node to child->opaque->children list, + * and then apply the drain to the whole child subtree, + * so that the drain count matches with the parent. + */ static void bdrv_child_cb_attach(BdrvChild *child) { BlockDriverState *bs = child->opaque; @@ -2889,8 +2894,6 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, } if (new_bs) { - assert_bdrv_graph_writable(new_bs); - QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); /* * Detaching the old node may have led to the new node's @@ -2901,12 +2904,19 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, assert(new_bs->quiesce_counter <= new_bs_quiesce_counter); drain_saldo += new_bs->quiesce_counter - new_bs_quiesce_counter; - /* Attach only after starting new drained sections, so that recursive - * drain sections coming from @child don't get an extra .drained_begin - * callback. */ + /* + * First call ->attach() cb. + * In child_of_bds case, add child to the parent + * (child->opaque) ->children list and if + * necessary add missing drains in the child subtree. + */ if (child->klass->attach) { child->klass->attach(child); } + + /* Then add child to new_bs->parents list */ + assert_bdrv_graph_writable(new_bs); + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); } /*
Doing the opposite can make adding the child node to a non-drained node, as apply_subtree_drain is only done in ->attach() and thus make assert_bdrv_graph_writable fail. This can happen for example during a transaction rollback (test 245, test_io_with_graph_changes): 1. a node is removed from the graph, thus it is undrained 2. then something happens, and we need to roll back the transactions through tran_abort() 3. at this point, the current code would first attach the undrained node to the graph via QLIST_INSERT_HEAD, and then call ->attach() that will take care of restoring the drain with apply_subtree_drain(), leaving the node undrained between the two operations. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)