block: Graph locking part 6 (bs->file/backing)

[PATCH 03/24] block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK

Posted by Kevin Wolf 1 year, 1 month ago

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_filter_bs() need to hold a reader lock for the graph because
it calls bdrv_filter_child(), which accesses bs->file/backing.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-io.h       | 2 +-
 include/block/block_int-io.h   | 3 ++-
 block.c                        | 9 +++++++--
 block/stream.c                 | 2 ++
 migration/block-dirty-bitmap.c | 4 ++++
 5 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/include/block/block-io.h b/include/block/block-io.h
index ad270b6ad2..58c4cf50a0 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -183,7 +183,7 @@ bdrv_co_eject(BlockDriverState *bs, bool eject_flag);
 
 const char *bdrv_get_format_name(BlockDriverState *bs);
 
-bool bdrv_supports_compressed_writes(BlockDriverState *bs);
+bool GRAPH_RDLOCK bdrv_supports_compressed_writes(BlockDriverState *bs);
 const char *bdrv_get_node_name(const BlockDriverState *bs);
 
 const char * GRAPH_RDLOCK
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 34eac72d7a..26bff94e4e 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -143,7 +143,8 @@ static inline BlockDriverState *bdrv_cow_bs(BlockDriverState *bs)
     return child_bs(bdrv_cow_child(bs));
 }
 
-static inline BlockDriverState *bdrv_filter_bs(BlockDriverState *bs)
+static inline BlockDriverState * GRAPH_RDLOCK
+bdrv_filter_bs(BlockDriverState *bs)
 {
     IO_CODE();
     return child_bs(bdrv_filter_child(bs));
diff --git a/block.c b/block.c
index 1171f47570..d85738b7dc 100644
--- a/block.c
+++ b/block.c
@@ -820,12 +820,17 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
 int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
 {
     BlockDriver *drv = bs->drv;
-    BlockDriverState *filtered = bdrv_filter_bs(bs);
+    BlockDriverState *filtered;
+
     GLOBAL_STATE_CODE();
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
 
     if (drv && drv->bdrv_probe_geometry) {
         return drv->bdrv_probe_geometry(bs, geo);
-    } else if (filtered) {
+    }
+
+    filtered = bdrv_filter_bs(bs);
+    if (filtered) {
         return bdrv_probe_geometry(filtered, geo);
     }
 
diff --git a/block/stream.c b/block/stream.c
index ddaab7dbbd..b22d9c236b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -268,6 +268,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
         assert(!bottom->drv->is_filter);
         base_overlay = above_base = bottom;
     } else {
+        GRAPH_RDLOCK_GUARD_MAINLOOP();
+
         base_overlay = bdrv_find_overlay(bs, base);
         if (!base_overlay) {
             error_setg(errp, "'%s' is not in the backing chain of '%s'",
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 03cb2e72ee..24347ab0f7 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -607,6 +607,10 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
     BlockBackend *blk;
     GHashTable *alias_map = NULL;
 
+    /* Runs in the migration thread, but holds the iothread lock */
+    GLOBAL_STATE_CODE();
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     if (migrate_has_block_bitmap_mapping()) {
         alias_map = construct_alias_map(migrate_block_bitmap_mapping(), true,
                                         &error_abort);
-- 
2.41.0

Re: [PATCH 03/24] block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK

Posted by Eric Blake 1 year, 1 month ago

On Fri, Oct 27, 2023 at 05:53:12PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_filter_bs() need to hold a reader lock for the graph because
> it calls bdrv_filter_child(), which accesses bs->file/backing.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-io.h       | 2 +-
>  include/block/block_int-io.h   | 3 ++-
>  block.c                        | 9 +++++++--
>  block/stream.c                 | 2 ++
>  migration/block-dirty-bitmap.c | 4 ++++
>  5 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/include/block/block-io.h b/include/block/block-io.h
> index ad270b6ad2..58c4cf50a0 100644
> --- a/include/block/block-io.h
> +++ b/include/block/block-io.h
> @@ -183,7 +183,7 @@ bdrv_co_eject(BlockDriverState *bs, bool eject_flag);
>  
>  const char *bdrv_get_format_name(BlockDriverState *bs);
>  
> -bool bdrv_supports_compressed_writes(BlockDriverState *bs);
> +bool GRAPH_RDLOCK bdrv_supports_compressed_writes(BlockDriverState *bs);

Unrelated change that belongs elsewhere in the series?

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

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

Re: [PATCH 03/24] block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK

Posted by Eric Blake 1 year, 1 month ago

On Fri, Oct 27, 2023 at 03:02:38PM -0500, Eric Blake wrote:
> On Fri, Oct 27, 2023 at 05:53:12PM +0200, Kevin Wolf wrote:
> > This adds GRAPH_RDLOCK annotations to declare that callers of
> > bdrv_filter_bs() need to hold a reader lock for the graph because
> > it calls bdrv_filter_child(), which accesses bs->file/backing.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/block/block-io.h       | 2 +-
> >  include/block/block_int-io.h   | 3 ++-
> >  block.c                        | 9 +++++++--
> >  block/stream.c                 | 2 ++
> >  migration/block-dirty-bitmap.c | 4 ++++
> >  5 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/block/block-io.h b/include/block/block-io.h
> > index ad270b6ad2..58c4cf50a0 100644
> > --- a/include/block/block-io.h
> > +++ b/include/block/block-io.h
> > @@ -183,7 +183,7 @@ bdrv_co_eject(BlockDriverState *bs, bool eject_flag);
> >  
> >  const char *bdrv_get_format_name(BlockDriverState *bs);
> >  
> > -bool bdrv_supports_compressed_writes(BlockDriverState *bs);
> > +bool GRAPH_RDLOCK bdrv_supports_compressed_writes(BlockDriverState *bs);
> 
> Unrelated change that belongs elsewhere in the series?

Answering myself:

bdrv_supports_compressed_writes() calls bdrv_filter_bs(), and is
itself called only by backup_job_create() which already uses
GRAPH_RDLOCK_GUARD_MAINLOOP().  It wasn't obvious in the diff, but
looking at the bigger context it makes sense.  Maybe calling it out in
the commit message would help future readers, though.

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

R-b stands.

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