This is part of resolving the deadlock mentioned in commit "block:
move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".

The function bdrv_set_backing_hd_drained() holds the graph lock, so it
is not allowed to drain. It is called by:
1. bdrv_set_backing_hd(), where a drained section is introduced,
   replacing the previously present bs-specific drains.
2. stream_prepare(), where a drained section is introduced replacing
   the previously present bs-specific drains.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v3.

 block.c        | 6 ++----
 block/stream.c | 6 ++----
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 64db71e38d..75322789b5 100644
--- a/block.c
+++ b/block.c
@@ -3569,7 +3569,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
         assert(bs->backing->bs->quiesce_counter > 0);
     }
 
-    bdrv_drain_all_begin();
     ret = bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp);
     if (ret < 0) {
         goto out;
@@ -3578,7 +3577,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
     ret = bdrv_refresh_perms(bs, tran, errp);
 out:
     tran_finalize(tran, ret);
-    bdrv_drain_all_end();
     return ret;
 }
 
@@ -3594,11 +3592,11 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
     bdrv_graph_rdunlock_main_loop();
 
     bdrv_ref(drain_bs);
-    bdrv_drained_begin(drain_bs);
+    bdrv_drain_all_begin();
     bdrv_graph_wrlock();
     ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
     bdrv_graph_wrunlock();
-    bdrv_drained_end(drain_bs);
+    bdrv_drain_all_end();
     bdrv_unref(drain_bs);
 
     return ret;
diff --git a/block/stream.c b/block/stream.c
index 999d9e56d4..6ba49cffd3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -80,11 +80,10 @@ static int stream_prepare(Job *job)
      * may end up working with the wrong base node (or it might even have gone
      * away by the time we want to use it).
      */
-    bdrv_drained_begin(unfiltered_bs);
     if (unfiltered_bs_cow) {
         bdrv_ref(unfiltered_bs_cow);
-        bdrv_drained_begin(unfiltered_bs_cow);
     }
+    bdrv_drain_all_begin();
 
     bdrv_graph_rdlock_main_loop();
     base = bdrv_filter_or_cow_bs(s->above_base);
@@ -123,11 +122,10 @@ static int stream_prepare(Job *job)
     }
 
 out:
+    bdrv_drain_all_end();
     if (unfiltered_bs_cow) {
-        bdrv_drained_end(unfiltered_bs_cow);
         bdrv_unref(unfiltered_bs_cow);
     }
-    bdrv_drained_end(unfiltered_bs);
     return ret;
 }
 
-- 
2.39.5

Am 26.05.2025 um 15:21 hat Fiona Ebner geschrieben:
> This is part of resolving the deadlock mentioned in commit "block:
> move draining out of bdrv_change_aio_context() and mark GRAPH_RDLOCK".
> 
> The function bdrv_set_backing_hd_drained() holds the graph lock, so it
> is not allowed to drain. It is called by:
> 1. bdrv_set_backing_hd(), where a drained section is introduced,
>    replacing the previously present bs-specific drains.
> 2. stream_prepare(), where a drained section is introduced replacing
>    the previously present bs-specific drains.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> No changes in v3.
> 
>  block.c        | 6 ++----
>  block/stream.c | 6 ++----
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 64db71e38d..75322789b5 100644
> --- a/block.c
> +++ b/block.c
> @@ -3569,7 +3569,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
>          assert(bs->backing->bs->quiesce_counter > 0);
>      }
>  
> -    bdrv_drain_all_begin();
>      ret = bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp);
>      if (ret < 0) {
>          goto out;
> @@ -3578,7 +3577,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
>      ret = bdrv_refresh_perms(bs, tran, errp);
>  out:
>      tran_finalize(tran, ret);
> -    bdrv_drain_all_end();
>      return ret;
>  }

Do we need to update the comment for bdrv_set_backing_hd_drained()?

 * If a backing child is already present (i.e. we're detaching a node), that
 * child node must be drained.

Same as in the previous patch, this is now probably all nodes.

> @@ -3594,11 +3592,11 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>      bdrv_graph_rdunlock_main_loop();
>  
>      bdrv_ref(drain_bs);
> -    bdrv_drained_begin(drain_bs);
> +    bdrv_drain_all_begin();
>      bdrv_graph_wrlock();
>      ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
>      bdrv_graph_wrunlock();
> -    bdrv_drained_end(drain_bs);
> +    bdrv_drain_all_end();
>      bdrv_unref(drain_bs);

The only thing we do with drain_bs now is finding it, bdrv_ref() and
immediately bdrv_unref(). I don't think it should exist any more after
the change to drain_all.

Kevin

Am 27.05.25 um 17:29 schrieb Kevin Wolf:
> Am 26.05.2025 um 15:21 hat Fiona Ebner geschrieben:
>> @@ -3578,7 +3577,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
>>      ret = bdrv_refresh_perms(bs, tran, errp);
>>  out:
>>      tran_finalize(tran, ret);
>> -    bdrv_drain_all_end();
>>      return ret;
>>  }
> 
> Do we need to update the comment for bdrv_set_backing_hd_drained()?
> 
>  * If a backing child is already present (i.e. we're detaching a node), that
>  * child node must be drained.
> 
> Same as in the previous patch, this is now probably all nodes.

Yes, and even in case no backing child is already present.

>> @@ -3594,11 +3592,11 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>>      bdrv_graph_rdunlock_main_loop();
>>  
>>      bdrv_ref(drain_bs);
>> -    bdrv_drained_begin(drain_bs);
>> +    bdrv_drain_all_begin();
>>      bdrv_graph_wrlock();
>>      ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
>>      bdrv_graph_wrunlock();
>> -    bdrv_drained_end(drain_bs);
>> +    bdrv_drain_all_end();
>>      bdrv_unref(drain_bs);
> 
> The only thing we do with drain_bs now is finding it, bdrv_ref() and
> immediately bdrv_unref(). I don't think it should exist any more after
> the change to drain_all.

I'll drop it in v4.

I now noticed that bdrv_set_backing_hd() is required to be
GRAPH_UNLOCKED, because it calls bdrv_drain_all_begin(), but is not
marked as such yet. Adding that annotation requires adapting some
callers of bdrv_set_backing_hd() first. I'll try to add some more
patches at the end of the series for this.

At least the caller in block/mirror.c seems to be better of using
bdrv_set_backing_hd_drained() and bdrv_graph_wrlock_drained() itself, so
the section can cover more related calls like
"unfiltered_target = bdrv_skip_filters(target_bs);"

Best Regards,
Fiona