Re: [PATCH 19/24] block: Introduce bdrv_co_change_backing_file()

[Kevin Wolf]

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.

Kevin