diff mbox series

[v5,10/31] block.c: modify .attach and .detach callbacks of child_of_bds

Message ID 20211124064418.3120601-11-eesposit@redhat.com
State New
Headers show
Series block layer: split block APIs in global state and I/O | expand

Commit Message

Emanuele Giuseppe Esposito Nov. 24, 2021, 6:43 a.m. UTC
According to the assertions put in the previous patch, we should
first drain and then modify the ->children list. In this way
we prevent other iothreads to read the list while it is being
updated.

In this case, moving the drain won't cause any harm, because
child is a parameter of the drain function so it will still be
included in the operation, despite not being in the list.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Hanna Czenczek Dec. 16, 2021, 2:57 p.m. UTC | #1
On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
> According to the assertions put in the previous patch, we should
> first drain and then modify the ->children list. In this way
> we prevent other iothreads to read the list while it is being
> updated.
>
> In this case, moving the drain won't cause any harm, because
> child is a parameter of the drain function so it will still be
> included in the operation, despite not being in the list.

Sounds good.

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 522a273140..5516c84ec4 100644
> --- a/block.c
> +++ b/block.c
> @@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>   {
>       BlockDriverState *bs = child->opaque;
>   
> +    bdrv_apply_subtree_drain(child, bs);
>       assert_bdrv_graph_writable(bs);
>       QLIST_INSERT_HEAD(&bs->children, child, next);
>   
> @@ -1423,7 +1424,6 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>           bdrv_backing_attach(child);
>       }
>   
> -    bdrv_apply_subtree_drain(child, bs);

I think we should also remove the empty line above.

Hanna

>   }
>   
>   static void bdrv_child_cb_detach(BdrvChild *child)
> @@ -1434,10 +1434,9 @@ static void bdrv_child_cb_detach(BdrvChild *child)
>           bdrv_backing_detach(child);
>       }
>   
> -    bdrv_unapply_subtree_drain(child, bs);
> -
>       assert_bdrv_graph_writable(bs);
>       QLIST_REMOVE(child, next);
> +    bdrv_unapply_subtree_drain(child, bs);
>   }
>   
>   static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,
Emanuele Giuseppe Esposito Dec. 16, 2021, 4:05 p.m. UTC | #2
On 16/12/2021 15:57, Hanna Reitz wrote:
> On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
>> According to the assertions put in the previous patch, we should
>> first drain and then modify the ->children list. In this way
>> we prevent other iothreads to read the list while it is being
>> updated.
>>
>> In this case, moving the drain won't cause any harm, because
>> child is a parameter of the drain function so it will still be
>> included in the operation, despite not being in the list.
> 
> Sounds good.

Uhm.. I don't think this is useful at all. I was thinking to drop this 
patch in v6.

My plans on subtree drains, ->children and ->parent lists are explained 
in "Removal of Aiocontext lock and usage of subtree drains in aborted 
transactions"

https://patchew.org/QEMU/20211213104014.69858-1-eesposit@redhat.com/

And as I say there, commit d736f119da makes sure that it is safe to 
modify the graph even side a bdrv_subtree_drained_begin/end() section.
So this should be unnecessary.

Thank you,
Emanuele
> 
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>   block.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 522a273140..5516c84ec4 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>>   {
>>       BlockDriverState *bs = child->opaque;
>> +    bdrv_apply_subtree_drain(child, bs);
>>       assert_bdrv_graph_writable(bs);
>>       QLIST_INSERT_HEAD(&bs->children, child, next);
>> @@ -1423,7 +1424,6 @@ static void bdrv_child_cb_attach(BdrvChild *child)
>>           bdrv_backing_attach(child);
>>       }
>> -    bdrv_apply_subtree_drain(child, bs);
> 
> I think we should also remove the empty line above.
> 
> Hanna
> 
>>   }
>>   static void bdrv_child_cb_detach(BdrvChild *child)
>> @@ -1434,10 +1434,9 @@ static void bdrv_child_cb_detach(BdrvChild *child)
>>           bdrv_backing_detach(child);
>>       }
>> -    bdrv_unapply_subtree_drain(child, bs);
>> -
>>       assert_bdrv_graph_writable(bs);
>>       QLIST_REMOVE(child, next);
>> +    bdrv_unapply_subtree_drain(child, bs);
>>   }
>>   static int bdrv_child_cb_update_filename(BdrvChild *c, 
>> BlockDriverState *base,
>
Hanna Czenczek Dec. 16, 2021, 4:12 p.m. UTC | #3
On 16.12.21 17:05, Emanuele Giuseppe Esposito wrote:
>
>
> On 16/12/2021 15:57, Hanna Reitz wrote:
>> On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote:
>>> According to the assertions put in the previous patch, we should
>>> first drain and then modify the ->children list. In this way
>>> we prevent other iothreads to read the list while it is being
>>> updated.
>>>
>>> In this case, moving the drain won't cause any harm, because
>>> child is a parameter of the drain function so it will still be
>>> included in the operation, despite not being in the list.
>>
>> Sounds good.
>
> Uhm.. I don't think this is useful at all. I was thinking to drop this 
> patch in v6.
>
> My plans on subtree drains, ->children and ->parent lists are 
> explained in "Removal of Aiocontext lock and usage of subtree drains 
> in aborted transactions"
>
> https://patchew.org/QEMU/20211213104014.69858-1-eesposit@redhat.com/
>
> And as I say there, commit d736f119da makes sure that it is safe to 
> modify the graph even side a bdrv_subtree_drained_begin/end() section.
> So this should be unnecessary.

Sounds good, too. :)

Hanna
diff mbox series

Patch

diff --git a/block.c b/block.c
index 522a273140..5516c84ec4 100644
--- a/block.c
+++ b/block.c
@@ -1416,6 +1416,7 @@  static void bdrv_child_cb_attach(BdrvChild *child)
 {
     BlockDriverState *bs = child->opaque;
 
+    bdrv_apply_subtree_drain(child, bs);
     assert_bdrv_graph_writable(bs);
     QLIST_INSERT_HEAD(&bs->children, child, next);
 
@@ -1423,7 +1424,6 @@  static void bdrv_child_cb_attach(BdrvChild *child)
         bdrv_backing_attach(child);
     }
 
-    bdrv_apply_subtree_drain(child, bs);
 }
 
 static void bdrv_child_cb_detach(BdrvChild *child)
@@ -1434,10 +1434,9 @@  static void bdrv_child_cb_detach(BdrvChild *child)
         bdrv_backing_detach(child);
     }
 
-    bdrv_unapply_subtree_drain(child, bs);
-
     assert_bdrv_graph_writable(bs);
     QLIST_REMOVE(child, next);
+    bdrv_unapply_subtree_drain(child, bs);
 }
 
 static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base,