[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 19/24] block: Introduce bdrv_co_change_backing_file()
From: |
Eric Blake |
Subject: |
Re: [PATCH 19/24] block: Introduce bdrv_co_change_backing_file() |
Date: |
Mon, 30 Oct 2023 08:57:08 -0500 |
User-agent: |
NeoMutt/20231023 |
On Fri, Oct 27, 2023 at 05:53:28PM +0200, Kevin Wolf wrote:
> bdrv_change_backing_file() is called both inside and outside coroutine
> context. This makes it difficult for it to take the graph lock
> internally. It also means that driver implementations need to be able to
> run outside of coroutines, too. Switch it to the usual model with a
> coroutine based implementation and a co_wrapper instead. The new
> function is marked GRAPH_RDLOCK.
>
> As the co_wrapper now runs the function in the AioContext of the node
> (as it should always have done), this is not GLOBAL_STATE_CODE() any
> more.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> include/block/block-global-state.h | 3 +-
> include/block/block-io.h | 8 ++++
> include/block/block_int-common.h | 5 ++-
> block.c | 11 ++---
> block/qcow2.c | 18 +++++----
> block/qed.c | 64 +++++++++++++++---------------
> tests/unit/test-bdrv-drain.c | 8 ++--
> 7 files changed, 65 insertions(+), 52 deletions(-)
>
> +++ b/block/qcow2.c
> @@ -6155,9 +6159,9 @@ BlockDriver bdrv_qcow2 = {
> .bdrv_co_save_vmstate = qcow2_co_save_vmstate,
> .bdrv_co_load_vmstate = qcow2_co_load_vmstate,
>
> - .is_format = true,
> - .supports_backing = true,
> - .bdrv_change_backing_file = qcow2_change_backing_file,
> + .is_format = true,
> + .supports_backing = true,
> + .bdrv_co_change_backing_file = qcow2_co_change_backing_file,
>
> .bdrv_refresh_limits = qcow2_refresh_limits,
> .bdrv_co_invalidate_cache = qcow2_co_invalidate_cache,
Here, you only realigned = on a portion of the initializer...
> diff --git a/block/qed.c b/block/qed.c
> index 686ad711f7..996aa384fe 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> static BlockDriver bdrv_qed = {
> - .format_name = "qed",
> - .instance_size = sizeof(BDRVQEDState),
> - .create_opts = &qed_create_opts,
> - .is_format = true,
> - .supports_backing = true,
> -
> - .bdrv_probe = bdrv_qed_probe,
...while here, you are doing it on the entire block. This shows why I
personally dislike aligning =, but I tolerate it when it is already
prevailing style. Still, it feels weird to be inconsistent within the
same patch.
At any rate, that's cosmetic, and not a correctness issue. Up to you
if you want to ignore my commentary in this case.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
- Re: [PATCH 07/24] block: Mark bdrv_skip_implicit_filters() and callers GRAPH_RDLOCK, (continued)
- [PATCH 17/24] block: Protect bs->backing with graph_lock, Kevin Wolf, 2023/10/27
- [PATCH 16/24] block: Mark bdrv_replace_node() GRAPH_WRLOCK, Kevin Wolf, 2023/10/27
- [PATCH 18/24] blkverify: Add locking for request_fn, Kevin Wolf, 2023/10/27
- [PATCH 20/24] block: Add missing GRAPH_RDLOCK annotations, Kevin Wolf, 2023/10/27
- [PATCH 19/24] block: Introduce bdrv_co_change_backing_file(), Kevin Wolf, 2023/10/27
- Re: [PATCH 19/24] block: Introduce bdrv_co_change_backing_file(),
Eric Blake <=
- [PATCH 12/24] block: Mark bdrv_cow_child() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/10/27
- [PATCH 22/24] vhdx: Take locks for accessing bs->file, Kevin Wolf, 2023/10/27
- [PATCH 23/24] block: Take graph lock for most of .bdrv_open, Kevin Wolf, 2023/10/27
- [PATCH 24/24] block: Protect bs->file with graph_lock, Kevin Wolf, 2023/10/27