| Message ID | 20220208153655.1251658-4-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: bug fixes in preparation of AioContext removal | expand |
On Tue, Feb 08, 2022 at 10:36:52AM -0500, Emanuele Giuseppe Esposito wrote: > 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 | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben: > 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 | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index ec346a7e2e..08a6e3a4ef 100644 > --- a/block.c > +++ b/block.c > @@ -2872,8 +2872,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 > @@ -2890,6 +2888,10 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, > if (child->klass->attach) { > child->klass->attach(child); > } > + > + assert_bdrv_graph_writable(new_bs); > + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); > + > } Extra empty line. Looks good otherwise. Does this also mean that the order in bdrv_child_cb_attach/detach() is wrong? Or maybe adding a new node to bs->children is okay even when the child node isn't drained. Kevin
On 11/02/2022 13:34, Kevin Wolf wrote: > Am 08.02.2022 um 16:36 hat Emanuele Giuseppe Esposito geschrieben: >> 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 | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/block.c b/block.c >> index ec346a7e2e..08a6e3a4ef 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2872,8 +2872,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 >> @@ -2890,6 +2888,10 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, >> if (child->klass->attach) { >> child->klass->attach(child); >> } >> + >> + assert_bdrv_graph_writable(new_bs); >> + QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); >> + >> } > > Extra empty line. Looks good otherwise. > > Does this also mean that the order in bdrv_child_cb_attach/detach() is > wrong? Or maybe adding a new node to bs->children is okay even when the > child node isn't drained. No I don't think it's wrong. In fact, if we are just replacing a node (so old_bs and new_bs are both != NULL), the child will be just removed and then re-added to the same children's list of the same parent (child->opaque). Whether adding a new node to bs->children requires a drain or not is still under debate in the other serie with Vladimir. We'll see about that, but in the meanwhile this is just a safe fix that makes sure that *if* drains are added, everything will always stay under proper drain. Emanuele > > Kevin >
diff --git a/block.c b/block.c index ec346a7e2e..08a6e3a4ef 100644 --- a/block.c +++ b/block.c @@ -2872,8 +2872,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 @@ -2890,6 +2888,10 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, if (child->klass->attach) { child->klass->attach(child); } + + 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 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)