[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: |
Fri, 3 Nov 2023 07:38:59 -0500 |
User-agent: |
NeoMutt/20231023 |
On Fri, Nov 03, 2023 at 11:33:47AM +0100, Kevin Wolf wrote:
> Am 30.10.2023 um 14:57 hat Eric Blake geschrieben:
> > 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.
>
> It's because qcow2 already had multiple different indentations, but qed
> had everything aligned to the same column. I can update qcow2.
It's your call how you want it to look (as I stated, we are already
inconsistent on style; my preference is a version with less churn, but
I also understand preserving the existing style as a way to avoid
churn). Whatever the end result, it doesn't affect my R-b.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org