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.eb...@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
