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);
+
     }
 
     /*
-- 
2.31.1

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
>