Re: [PATCH 17/24] block: Protect bs->backing with graph_lock

[Eric Blake]

On Fri, Oct 27, 2023 at 05:53:26PM +0200, Kevin Wolf wrote:
> Almost all functions that access bs->backing already take the graph
> lock now. Add locking to the remaining users and finally annotate the
> struct field itself as protected by the graph lock.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int-common.h |  2 +-
>  block.c                          | 27 +++++++++++++++++----------
>  block/commit.c                   |  5 ++++-
>  block/mirror.c                   | 17 ++++++++++++-----
>  block/qed.c                      |  2 +-
>  block/replication.c              |  7 ++++++-
>  block/vmdk.c                     |  7 ++++---
>  tests/unit/test-bdrv-drain.c     | 22 ++++++++++++++++++----
>  tests/unit/test-bdrv-graph-mod.c |  5 ++++-
>  9 files changed, 67 insertions(+), 27 deletions(-)
> 
> +++ b/block.c
...
> @@ -5204,10 +5208,11 @@ static void bdrv_close(BlockDriverState *bs)
>      QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
>          bdrv_unref_child(bs, child);
>      }
> -    bdrv_graph_wrunlock();
>  
>      assert(!bs->backing);
>      assert(!bs->file);
> +    bdrv_graph_wrunlock();
> +
>      g_free(bs->opaque);

At first I wondered why you needed code motion to pull the asserts
inside the lock.  But now I get it - direct access to bs->backing
itself now requires a slightly larger lock scope.

>  static int coroutine_fn mirror_run(Job *job, Error **errp)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
> -    BlockDriverState *bs = s->mirror_top_bs->backing->bs;
> +    BlockDriverState *bs;
>      MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque;
>      BlockDriverState *target_bs = blk_bs(s->target);
>      bool need_drain = true;
> @@ -935,6 +939,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
>      }
>  
>      bdrv_graph_co_rdlock();
> +    bs = bdrv_filter_bs(s->mirror_top_bs);

Interesting change to prefer bdrv_filter_bs() instead of direct access
to ->backing->bs; but I think it's okay.

> +++ b/tests/unit/test-bdrv-drain.c
> @@ -218,8 +218,14 @@ static void do_drain_end_unlocked(enum drain_type 
> drain_type, BlockDriverState *
>      }
>  }
>  
> -static void test_drv_cb_common(BlockBackend *blk, enum drain_type drain_type,
> -                               bool recursive)
> +/*
> + * Locking the block graph would be a bit cumbersome here because this 
> function
> + * is called both in coroutine and non-coroutine context. We know this is a 
> test
> + * and nothing else is running, so don't bother with TSA.
> + */
> +static void coroutine_mixed_fn TSA_NO_TSA
> +test_drv_cb_common(BlockBackend *blk, enum drain_type drain_type,
> +                   bool recursive)

Fair enough.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org