Re: [PATCH 10/11] block/graph-lock: add drain flag to bdrv_graph_wr{,un}lock

[Kevin Wolf <kwolf@redhat.com>]

Am 08.05.2025 um 16:09 hat Fiona Ebner geschrieben:
> For convenience, allow indicating whether the write-locked section
> should also be a drained section directly when taking the write
> lock.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Good idea, the pattern seems to be common enough.

> In bdrv_graph_wrlock() there is a comment that it uses
> bdrv_drain_all_begin_nopoll() to make sure that constantly arriving
> new I/O doesn't cause starvation. The changes from this series are at
> odds with that, but there doesn't seem to be any (new) test failures.

I don't see why they are at odds with it? Draining an already drained
node isn't a problem, it just increases the counter without doing
anything else.

> The following script was used:
> 
> > find . -name '*.c' -exec sed -i -z 's/bdrv_drain_all_begin();\n\s*bdrv_graph_wrlock();/bdrv_graph_wrlock(true);/g' {} ';'
> > find . -name '*.c' -exec sed -i -z 's/bdrv_graph_wrunlock();\n\s*bdrv_drain_all_end();/bdrv_graph_wrunlock(true);/g' {} ';'
> > find . -name '*.c' -exec sed -i -z 's/bdrv_graph_wrlock();/bdrv_graph_wrlock(false);/g' {} ';'
> > find . -name '*.c' -exec sed -i -z 's/bdrv_graph_wrunlock();/bdrv_graph_wrunlock(false);/g' {} ';'
> 
>  block.c                            | 62 ++++++++++++------------------
>  block/backup.c                     |  6 +--
>  block/blklogwrites.c               | 12 ++----
>  block/blkverify.c                  |  6 +--
>  block/block-backend.c              | 12 ++----
>  block/commit.c                     | 20 ++++------
>  block/graph-lock.c                 | 22 ++++++++---
>  block/mirror.c                     | 27 ++++++-------
>  block/qcow2.c                      |  6 +--
>  block/quorum.c                     | 12 ++----
>  block/replication.c                | 21 ++++------
>  block/snapshot.c                   |  6 +--
>  block/stream.c                     | 16 +++-----
>  block/vmdk.c                       | 30 +++++----------
>  blockdev.c                         | 10 ++---
>  blockjob.c                         | 18 +++------
>  include/block/graph-lock.h         |  8 +++-
>  scripts/block-coroutine-wrapper.py |  4 +-
>  tests/unit/test-bdrv-drain.c       | 58 ++++++++++------------------
>  tests/unit/test-bdrv-graph-mod.c   | 30 +++++----------
>  20 files changed, 152 insertions(+), 234 deletions(-)

> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
> index 2c26c72108..f291ccbc97 100644
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -108,17 +108,21 @@ void unregister_aiocontext(AioContext *ctx);
>   *
>   * The wrlock can only be taken from the main loop, with BQL held, as only the
>   * main loop is allowed to modify the graph.
> + *
> + * @drain whether bdrv_drain_all_begin() should be called before taking the lock
>   */
>  void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA
> -bdrv_graph_wrlock(void);
> +bdrv_graph_wrlock(bool drain);

I would prefer having two separate functions instead of a bool
parameter.

bdrv_graph_wrlock() could stay as it is, and bdrv_graph_wrlock_drained()
could be the convenience wrapper that drains first.

>  /*
>   * bdrv_graph_wrunlock:
>   * Write finished, reset global has_writer to 0 and restart
>   * all readers that are waiting.
> + *
> + * @drain whether bdrv_drain_all_end() should be called after releasing the lock
>   */
>  void no_coroutine_fn TSA_RELEASE(graph_lock) TSA_NO_TSA
> -bdrv_graph_wrunlock(void);
> +bdrv_graph_wrunlock(bool drain);

Here I would prefer to only keep the old bdrv_graph_wrunlock() without
a parameter. Can we just remember @drain from bdrv_graph_wrlock() in a
global variable? This would prevent callers from mismatching lock and
unlock variants (which TSA wouldn't be able to catch).

Kevin