[PATCH 00/20] Graph locking, part 3 (more block drivers)

[PATCH 00/20] Graph locking, part 3 (more block drivers)

[Kevin Wolf]

The first few patches in this series fix coroutine correctness problems
that have existed for a while, but only actually start to make things
fail in practice with stricter checks that we're going to introduce with
the graph locking work.

The rest of the series makes sure that the graph lock is held in more
block driver functions that access the children or parents list of a
block node. This is incremental work and more patches are yet to come.

Emanuele Giuseppe Esposito (5):
  nbd: Mark nbd_co_do_establish_connection() and callers GRAPH_RDLOCK
  block: Mark bdrv_co_get_allocated_file_size() and callers GRAPH_RDLOCK
  block: Mark bdrv_co_get_info() and callers GRAPH_RDLOCK
  block: Mark bdrv_co_debug_event() GRAPH_RDLOCK
  block: Mark BlockDriver callbacks for amend job GRAPH_RDLOCK

Kevin Wolf (15):
  qcow2: Don't call bdrv_getlength() in coroutine_fns
  block: Consistently call bdrv_activate() outside coroutine
  block: bdrv/blk_co_unref() for calls in coroutine context
  block: Don't call no_coroutine_fns in qmp_block_resize()
  test-bdrv-drain: Don't modify the graph in coroutines
  graph-lock: Add GRAPH_UNLOCKED(_PTR)
  graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock
  block: .bdrv_open is non-coroutine and unlocked
  nbd: Remove nbd_co_flush() wrapper function
  vhdx: Take graph lock for accessing a node's parent list
  mirror: Take graph lock for accessing a node's parent list
  block: Mark bdrv_query_bds_stats() and callers GRAPH_RDLOCK
  block: Mark bdrv_query_block_graph_info() and callers GRAPH_RDLOCK
  block: Mark bdrv_recurse_can_replace() and callers GRAPH_RDLOCK
  block: Mark bdrv_refresh_limits() and callers GRAPH_RDLOCK

 block/coroutines.h                          |   5 +-
 block/qcow2.h                               |   4 +-
 include/block/block-global-state.h          |  19 +++-
 include/block/block-io.h                    |  23 ++--
 include/block/block_int-common.h            |  37 +++----
 include/block/block_int-global-state.h      |   4 +-
 include/block/graph-lock.h                  |   6 +-
 include/block/qapi.h                        |   7 +-
 include/sysemu/block-backend-global-state.h |   5 +-
 block.c                                     |  25 ++++-
 block/amend.c                               |   8 +-
 block/blkverify.c                           |   5 +-
 block/block-backend.c                       |  10 +-
 block/crypto.c                              |   8 +-
 block/io.c                                  |  12 +--
 block/mirror.c                              |  14 ++-
 block/nbd.c                                 |  50 +++++----
 block/parallels.c                           |   6 +-
 block/qapi.c                                |   6 +-
 block/qcow.c                                |   6 +-
 block/qcow2-refcount.c                      |   2 +-
 block/qcow2.c                               |  48 ++++-----
 block/qed.c                                 |  24 ++---
 block/quorum.c                              |   4 +-
 block/raw-format.c                          |   2 +-
 block/vdi.c                                 |   6 +-
 block/vhdx.c                                |  15 +--
 block/vmdk.c                                |  20 ++--
 block/vpc.c                                 |   6 +-
 blockdev.c                                  |   7 +-
 qemu-img.c                                  |   2 +
 tests/unit/test-bdrv-drain.c                | 112 +++++++++++++-------
 32 files changed, 304 insertions(+), 204 deletions(-)

-- 
2.40.0

[PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns

[Kevin Wolf]

There is a bdrv_co_getlength() now, which should be used in coroutine
context.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.h          |  4 +++-
 block/qcow2-refcount.c |  2 +-
 block/qcow2.c          | 19 +++++++++----------
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index c75decc38a..4f67eb912a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
                                 void *cb_opaque, Error **errp);
 int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs);
 int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
-int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs);
+
+int coroutine_fn GRAPH_RDLOCK
+qcow2_detect_metadata_preallocation(BlockDriverState *bs);
 
 /* qcow2-cluster.c functions */
 int qcow2_grow_l1_table(BlockDriverState *bs, uint64_t min_size,
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b2a81ff707..4cf91bd955 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -3715,7 +3715,7 @@ int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs)
 
     qemu_co_mutex_assert_locked(&s->lock);
 
-    file_length = bdrv_getlength(bs->file->bs);
+    file_length = bdrv_co_getlength(bs->file->bs);
     if (file_length < 0) {
         return file_length;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index fe5def438e..94cf59af8b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options)
     }
 }
 
-static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
-                                              bool want_zero,
-                                              int64_t offset, int64_t count,
-                                              int64_t *pnum, int64_t *map,
-                                              BlockDriverState **file)
+static int coroutine_fn GRAPH_RDLOCK
+qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
+                      int64_t count, int64_t *pnum, int64_t *map,
+                      BlockDriverState **file)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t host_offset;
@@ -3235,7 +3234,7 @@ preallocate_co(BlockDriverState *bs, uint64_t offset, uint64_t new_length,
      * all of the allocated clusters (otherwise we get failing reads after
      * EOF). Extend the image to the last allocated sector.
      */
-    file_length = bdrv_getlength(s->data_file->bs);
+    file_length = bdrv_co_getlength(s->data_file->bs);
     if (file_length < 0) {
         error_setg_errno(errp, -file_length, "Could not get file size");
         ret = file_length;
@@ -4098,7 +4097,7 @@ qcow2_co_copy_range_from(BlockDriverState *bs,
         case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
         case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
             if (bs->backing && bs->backing->bs) {
-                int64_t backing_length = bdrv_getlength(bs->backing->bs);
+                int64_t backing_length = bdrv_co_getlength(bs->backing->bs);
                 if (src_offset >= backing_length) {
                     cur_write_flags |= BDRV_REQ_ZERO_WRITE;
                 } else {
@@ -4293,7 +4292,7 @@ qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
             goto fail;
         }
 
-        old_file_size = bdrv_getlength(bs->file->bs);
+        old_file_size = bdrv_co_getlength(bs->file->bs);
         if (old_file_size < 0) {
             error_setg_errno(errp, -old_file_size,
                              "Failed to inquire current file length");
@@ -4386,7 +4385,7 @@ qcow2_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
             break;
         }
 
-        old_file_size = bdrv_getlength(bs->file->bs);
+        old_file_size = bdrv_co_getlength(bs->file->bs);
         if (old_file_size < 0) {
             error_setg_errno(errp, -old_file_size,
                              "Failed to inquire current file length");
@@ -4694,7 +4693,7 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
          * align end of file to a sector boundary to ease reading with
          * sector based I/Os
          */
-        int64_t len = bdrv_getlength(bs->file->bs);
+        int64_t len = bdrv_co_getlength(bs->file->bs);
         if (len < 0) {
             return len;
         }
-- 
2.40.0

Re: [PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote:
> There is a bdrv_co_getlength() now, which should be used in coroutine
> context.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.h          |  4 +++-
>  block/qcow2-refcount.c |  2 +-
>  block/qcow2.c          | 19 +++++++++----------
>  3 files changed, 13 insertions(+), 12 deletions(-)

> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index c75decc38a..4f67eb912a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
>                                  void *cb_opaque, Error **errp);
>  int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs);
>  int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
> -int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs);
> +
> +int coroutine_fn GRAPH_RDLOCK
> +qcow2_detect_metadata_preallocation(BlockDriverState *bs);

> +++ b/block/qcow2.c
> @@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options)
>      }
>  }
>  
> -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
> -                                              bool want_zero,
> -                                              int64_t offset, int64_t count,
> -                                              int64_t *pnum, int64_t *map,
> -                                              BlockDriverState **file)
> +static int coroutine_fn GRAPH_RDLOCK
> +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
> +                      int64_t count, int64_t *pnum, int64_t *map,
> +                      BlockDriverState **file)
>  {

Should the commit message also call out that you are using this
opportunity to add GRAPH_RDLOCK annotations on affected functions?

Overall, all changes in this patch make sense, but I'm reluctant to
add R-b unless you confirm whether this was a rebase mistake (where
the annotations were intended to be added in a later patch).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns

[Kevin Wolf]

Am 25.04.2023 um 20:37 hat Eric Blake geschrieben:
> On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote:
> > There is a bdrv_co_getlength() now, which should be used in coroutine
> > context.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/qcow2.h          |  4 +++-
> >  block/qcow2-refcount.c |  2 +-
> >  block/qcow2.c          | 19 +++++++++----------
> >  3 files changed, 13 insertions(+), 12 deletions(-)
> 
> > 
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index c75decc38a..4f67eb912a 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -895,7 +895,9 @@ int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
> >                                  void *cb_opaque, Error **errp);
> >  int coroutine_fn GRAPH_RDLOCK qcow2_shrink_reftable(BlockDriverState *bs);
> >  int64_t coroutine_fn qcow2_get_last_cluster(BlockDriverState *bs, int64_t size);
> > -int coroutine_fn qcow2_detect_metadata_preallocation(BlockDriverState *bs);
> > +
> > +int coroutine_fn GRAPH_RDLOCK
> > +qcow2_detect_metadata_preallocation(BlockDriverState *bs);
> 
> > +++ b/block/qcow2.c
> > @@ -2089,11 +2089,10 @@ static void qcow2_join_options(QDict *options, QDict *old_options)
> >      }
> >  }
> >  
> > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
> > -                                              bool want_zero,
> > -                                              int64_t offset, int64_t count,
> > -                                              int64_t *pnum, int64_t *map,
> > -                                              BlockDriverState **file)
> > +static int coroutine_fn GRAPH_RDLOCK
> > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
> > +                      int64_t count, int64_t *pnum, int64_t *map,
> > +                      BlockDriverState **file)
> >  {
> 
> Should the commit message also call out that you are using this
> opportunity to add GRAPH_RDLOCK annotations on affected functions?

This is not just some additional change I did on the side, but the patch
doesn't compile (on clang with TSA enabled) without it because
bdrv_co_getlength() is GRAPH_RDLOCK, so its callers already must hold
the lock.

I can be more explicit about it in this patch, though I expect that the
same situation might happen more often in the future, and I'm not sure
if we want to mention that in the commit message any more than why we're
passing through an Error pointer.

> Overall, all changes in this patch make sense, but I'm reluctant to
> add R-b unless you confirm whether this was a rebase mistake (where
> the annotations were intended to be added in a later patch).

No, it's intentional.

Kevin

Re: [PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns

[Eric Blake]

On Thu, Apr 27, 2023 at 01:12:43PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 20:37 hat Eric Blake geschrieben:
> > On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote:
> > > There is a bdrv_co_getlength() now, which should be used in coroutine
> > > context.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---

> > >  
> > > -static int coroutine_fn qcow2_co_block_status(BlockDriverState *bs,
> > > -                                              bool want_zero,
> > > -                                              int64_t offset, int64_t count,
> > > -                                              int64_t *pnum, int64_t *map,
> > > -                                              BlockDriverState **file)
> > > +static int coroutine_fn GRAPH_RDLOCK
> > > +qcow2_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
> > > +                      int64_t count, int64_t *pnum, int64_t *map,
> > > +                      BlockDriverState **file)
> > >  {
> > 
> > Should the commit message also call out that you are using this
> > opportunity to add GRAPH_RDLOCK annotations on affected functions?
> 
> This is not just some additional change I did on the side, but the patch
> doesn't compile (on clang with TSA enabled) without it because
> bdrv_co_getlength() is GRAPH_RDLOCK, so its callers already must hold
> the lock.

Okay, that makes sense.

> 
> I can be more explicit about it in this patch, though I expect that the
> same situation might happen more often in the future, and I'm not sure
> if we want to mention that in the commit message any more than why we're
> passing through an Error pointer.

Still, a commit message something like:

There is a bdrv_co_getlength() now, which should be used in coroutine
context, which in turn requires more accurate function annotations.

would have helped me review faster.

> 
> > Overall, all changes in this patch make sense, but I'm reluctant to
> > add R-b unless you confirm whether this was a rebase mistake (where
> > the annotations were intended to be added in a later patch).
> 
> No, it's intentional.

All right, you've answered my question.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 01/20] qcow2: Don't call bdrv_getlength() in coroutine_fns

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Tue, Apr 25, 2023 at 07:31:39PM +0200, Kevin Wolf wrote:
> There is a bdrv_co_getlength() now, which should be used in coroutine
> context.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.h          |  4 +++-
>  block/qcow2-refcount.c |  2 +-
>  block/qcow2.c          | 19 +++++++++----------
>  3 files changed, 13 insertions(+), 12 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 02/20] block: Consistently call bdrv_activate() outside coroutine

[Kevin Wolf]

Migration code can call bdrv_activate() in coroutine context, whereas
other callers call it outside of coroutines. As it calls other code that
is not supposed to run in coroutines, standardise on running outside of
coroutines.

This adds a no_co_wrapper to switch to the main loop before calling
bdrv_activate().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-global-state.h |  6 +++++-
 block/block-backend.c              | 10 +++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 399200a9a3..2c312cc774 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -166,7 +166,11 @@ int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
 BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
                                         const char *node_name, Error **errp);
 
-int bdrv_activate(BlockDriverState *bs, Error **errp);
+int no_coroutine_fn bdrv_activate(BlockDriverState *bs, Error **errp);
+
+int coroutine_fn no_co_wrapper
+bdrv_co_activate(BlockDriverState *bs, Error **errp);
+
 void bdrv_activate_all(Error **errp);
 int bdrv_inactivate_all(void);
 
diff --git a/block/block-backend.c b/block/block-backend.c
index fc530ded6a..e37d55d3e9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2024,7 +2024,15 @@ void blk_activate(BlockBackend *blk, Error **errp)
         return;
     }
 
-    bdrv_activate(bs, errp);
+    /*
+     * Migration code can call this function in coroutine context, so leave
+     * coroutine context if necessary.
+     */
+    if (qemu_in_coroutine()) {
+        bdrv_co_activate(bs, errp);
+    } else {
+        bdrv_activate(bs, errp);
+    }
 }
 
 bool coroutine_fn blk_co_is_inserted(BlockBackend *blk)
-- 
2.40.0

Re: [PATCH 02/20] block: Consistently call bdrv_activate() outside coroutine

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:40PM +0200, Kevin Wolf wrote:
> Migration code can call bdrv_activate() in coroutine context, whereas
> other callers call it outside of coroutines. As it calls other code that
> is not supposed to run in coroutines, standardise on running outside of
> coroutines.
> 
> This adds a no_co_wrapper to switch to the main loop before calling
> bdrv_activate().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h |  6 +++++-
>  block/block-backend.c              | 10 +++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 02/20] block: Consistently call bdrv_activate() outside coroutine

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Tue, Apr 25, 2023 at 07:31:40PM +0200, Kevin Wolf wrote:
> Migration code can call bdrv_activate() in coroutine context, whereas
> other callers call it outside of coroutines. As it calls other code that
> is not supposed to run in coroutines, standardise on running outside of
> coroutines.
> 
> This adds a no_co_wrapper to switch to the main loop before calling
> bdrv_activate().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h |  6 +++++-
>  block/block-backend.c              | 10 +++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Kevin Wolf]

These functions must not be called in coroutine context, because they
need write access to the graph.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-global-state.h          |  3 ++-
 include/sysemu/block-backend-global-state.h |  5 ++++-
 block.c                                     |  2 +-
 block/crypto.c                              |  6 +++---
 block/parallels.c                           |  6 +++---
 block/qcow.c                                |  6 +++---
 block/qcow2.c                               | 14 +++++++-------
 block/qed.c                                 |  6 +++---
 block/vdi.c                                 |  6 +++---
 block/vhdx.c                                |  6 +++---
 block/vmdk.c                                | 18 +++++++++---------
 block/vpc.c                                 |  6 +++---
 12 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 2c312cc774..ec3ddb17a8 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -218,7 +218,8 @@ void bdrv_img_create(const char *filename, const char *fmt,
                      bool quiet, Error **errp);
 
 void bdrv_ref(BlockDriverState *bs);
-void bdrv_unref(BlockDriverState *bs);
+void no_coroutine_fn bdrv_unref(BlockDriverState *bs);
+void coroutine_fn no_co_wrapper bdrv_co_unref(BlockDriverState *bs);
 void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
 BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
                              BlockDriverState *child_bs,
diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h
index 2b6d27db7c..fa83f9389c 100644
--- a/include/sysemu/block-backend-global-state.h
+++ b/include/sysemu/block-backend-global-state.h
@@ -42,7 +42,10 @@ blk_co_new_open(const char *filename, const char *reference, QDict *options,
 
 int blk_get_refcnt(BlockBackend *blk);
 void blk_ref(BlockBackend *blk);
-void blk_unref(BlockBackend *blk);
+
+void no_coroutine_fn blk_unref(BlockBackend *blk);
+void coroutine_fn no_co_wrapper blk_co_unref(BlockBackend *blk);
+
 void blk_remove_all_bs(void);
 BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
diff --git a/block.c b/block.c
index 5ec1a3897e..20d5ee0959 100644
--- a/block.c
+++ b/block.c
@@ -680,7 +680,7 @@ int coroutine_fn bdrv_co_create_opts_simple(BlockDriver *drv,
 
     ret = 0;
 out:
-    blk_unref(blk);
+    blk_co_unref(blk);
     return ret;
 }
 
diff --git a/block/crypto.c b/block/crypto.c
index ca67289187..8fd3ad0054 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -355,7 +355,7 @@ block_crypto_co_create_generic(BlockDriverState *bs, int64_t size,
     ret = 0;
  cleanup:
     qcrypto_block_free(crypto);
-    blk_unref(blk);
+    blk_co_unref(blk);
     return ret;
 }
 
@@ -661,7 +661,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions *create_options, Error **errp)
 
     ret = 0;
 fail:
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     return ret;
 }
 
@@ -730,7 +730,7 @@ fail:
         bdrv_co_delete_file_noerr(bs);
     }
 
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
     qobject_unref(cryptoopts);
     return ret;
diff --git a/block/parallels.c b/block/parallels.c
index 013684801a..b49c35929e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -613,8 +613,8 @@ static int coroutine_fn parallels_co_create(BlockdevCreateOptions* opts,
 
     ret = 0;
 out:
-    blk_unref(blk);
-    bdrv_unref(bs);
+    blk_co_unref(blk);
+    bdrv_co_unref(bs);
     return ret;
 
 exit:
@@ -691,7 +691,7 @@ parallels_co_create_opts(BlockDriver *drv, const char *filename,
 
 done:
     qobject_unref(qdict);
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
diff --git a/block/qcow.c b/block/qcow.c
index 490e4f819e..a0c701f578 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -915,8 +915,8 @@ static int coroutine_fn qcow_co_create(BlockdevCreateOptions *opts,
     g_free(tmp);
     ret = 0;
 exit:
-    blk_unref(qcow_blk);
-    bdrv_unref(bs);
+    blk_co_unref(qcow_blk);
+    bdrv_co_unref(bs);
     qcrypto_block_free(crypto);
     return ret;
 }
@@ -1015,7 +1015,7 @@ qcow_co_create_opts(BlockDriver *drv, const char *filename,
 fail:
     g_free(backing_fmt);
     qobject_unref(qdict);
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
diff --git a/block/qcow2.c b/block/qcow2.c
index 94cf59af8b..01742b3ebe 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3705,7 +3705,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
         goto out;
     }
 
-    blk_unref(blk);
+    blk_co_unref(blk);
     blk = NULL;
 
     /*
@@ -3785,7 +3785,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
         }
     }
 
-    blk_unref(blk);
+    blk_co_unref(blk);
     blk = NULL;
 
     /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning.
@@ -3810,9 +3810,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 
     ret = 0;
 out:
-    blk_unref(blk);
-    bdrv_unref(bs);
-    bdrv_unref(data_bs);
+    blk_co_unref(blk);
+    bdrv_co_unref(bs);
+    bdrv_co_unref(data_bs);
     return ret;
 }
 
@@ -3943,8 +3943,8 @@ finish:
     }
 
     qobject_unref(qdict);
-    bdrv_unref(bs);
-    bdrv_unref(data_bs);
+    bdrv_co_unref(bs);
+    bdrv_co_unref(data_bs);
     qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
diff --git a/block/qed.c b/block/qed.c
index 0705a7b4e2..aff2a2076e 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -748,8 +748,8 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
     ret = 0; /* success */
 out:
     g_free(l1_table);
-    blk_unref(blk);
-    bdrv_unref(bs);
+    blk_co_unref(blk);
+    bdrv_co_unref(bs);
     return ret;
 }
 
@@ -819,7 +819,7 @@ bdrv_qed_co_create_opts(BlockDriver *drv, const char *filename,
 
 fail:
     qobject_unref(qdict);
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
diff --git a/block/vdi.c b/block/vdi.c
index f2434d6153..08331d2dd7 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -886,8 +886,8 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
 
     ret = 0;
 exit:
-    blk_unref(blk);
-    bdrv_unref(bs_file);
+    blk_co_unref(blk);
+    bdrv_co_unref(bs_file);
     g_free(bmap);
     return ret;
 }
@@ -975,7 +975,7 @@ vdi_co_create_opts(BlockDriver *drv, const char *filename,
 done:
     qobject_unref(qdict);
     qapi_free_BlockdevCreateOptions(create_options);
-    bdrv_unref(bs_file);
+    bdrv_co_unref(bs_file);
     return ret;
 }
 
diff --git a/block/vhdx.c b/block/vhdx.c
index 81420722a1..00777da91a 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2053,8 +2053,8 @@ static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
 
     ret = 0;
 delete_and_exit:
-    blk_unref(blk);
-    bdrv_unref(bs);
+    blk_co_unref(blk);
+    bdrv_co_unref(bs);
     g_free(creator);
     return ret;
 }
@@ -2144,7 +2144,7 @@ vhdx_co_create_opts(BlockDriver *drv, const char *filename,
 
 fail:
     qobject_unref(qdict);
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
diff --git a/block/vmdk.c b/block/vmdk.c
index 3f8c731e32..11b553ef25 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2306,7 +2306,7 @@ exit:
         if (pbb) {
             *pbb = blk;
         } else {
-            blk_unref(blk);
+            blk_co_unref(blk);
             blk = NULL;
         }
     }
@@ -2516,12 +2516,12 @@ vmdk_co_do_create(int64_t size,
         if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
             error_setg(errp, "Invalid backing file format: %s. Must be vmdk",
                        blk_bs(backing)->drv->format_name);
-            blk_unref(backing);
+            blk_co_unref(backing);
             ret = -EINVAL;
             goto exit;
         }
         ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
-        blk_unref(backing);
+        blk_co_unref(backing);
         if (ret) {
             error_setg(errp, "Failed to read parent CID");
             goto exit;
@@ -2542,14 +2542,14 @@ vmdk_co_do_create(int64_t size,
                              blk_bs(extent_blk)->filename);
         created_size += cur_size;
         extent_idx++;
-        blk_unref(extent_blk);
+        blk_co_unref(extent_blk);
     }
 
     /* Check whether we got excess extents */
     extent_blk = extent_fn(-1, extent_idx, flat, split, compress, zeroed_grain,
                            opaque, NULL);
     if (extent_blk) {
-        blk_unref(extent_blk);
+        blk_co_unref(extent_blk);
         error_setg(errp, "List of extents contains unused extents");
         ret = -EINVAL;
         goto exit;
@@ -2590,7 +2590,7 @@ vmdk_co_do_create(int64_t size,
     ret = 0;
 exit:
     if (blk) {
-        blk_unref(blk);
+        blk_co_unref(blk);
     }
     g_free(desc);
     g_free(parent_desc_line);
@@ -2641,7 +2641,7 @@ vmdk_co_create_opts_cb(int64_t size, int idx, bool flat, bool split,
                            errp)) {
         goto exit;
     }
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
 exit:
     g_free(ext_filename);
     return blk;
@@ -2797,12 +2797,12 @@ static BlockBackend * coroutine_fn vmdk_co_create_cb(int64_t size, int idx,
         return NULL;
     }
     blk_set_allow_write_beyond_eof(blk, true);
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
 
     if (size != -1) {
         ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain, errp);
         if (ret) {
-            blk_unref(blk);
+            blk_co_unref(blk);
             blk = NULL;
         }
     }
diff --git a/block/vpc.c b/block/vpc.c
index b89b0ff8e2..07ddda5b99 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1082,8 +1082,8 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     }
 
 out:
-    blk_unref(blk);
-    bdrv_unref(bs);
+    blk_co_unref(blk);
+    bdrv_co_unref(bs);
     return ret;
 }
 
@@ -1162,7 +1162,7 @@ vpc_co_create_opts(BlockDriver *drv, const char *filename,
 
 fail:
     qobject_unref(qdict);
-    bdrv_unref(bs);
+    bdrv_co_unref(bs);
     qapi_free_BlockdevCreateOptions(create_options);
     return ret;
 }
-- 
2.40.0

Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:41PM +0200, Kevin Wolf wrote:
> These functions must not be called in coroutine context, because they
> need write access to the graph.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h          |  3 ++-
>  include/sysemu/block-backend-global-state.h |  5 ++++-
>  block.c                                     |  2 +-
>  block/crypto.c                              |  6 +++---
>  block/parallels.c                           |  6 +++---
>  block/qcow.c                                |  6 +++---
>  block/qcow2.c                               | 14 +++++++-------
>  block/qed.c                                 |  6 +++---
>  block/vdi.c                                 |  6 +++---
>  block/vhdx.c                                |  6 +++---
>  block/vmdk.c                                | 18 +++++++++---------
>  block/vpc.c                                 |  6 +++---
>  12 files changed, 44 insertions(+), 40 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 11520 bytes --]

Il mar 25 apr 2023, 19:32 Kevin Wolf <kwolf@redhat.com> ha scritto:

> These functions must not be called in coroutine context, because they
> need write access to the graph.
>

With these patches applied vrc is still complaining about calls to
bdrv_unref_child from qcow2_do_open and qcow2_do_close.

Otherwise, the situation looks pretty good.

Paolo

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h          |  3 ++-
>  include/sysemu/block-backend-global-state.h |  5 ++++-
>  block.c                                     |  2 +-
>  block/crypto.c                              |  6 +++---
>  block/parallels.c                           |  6 +++---
>  block/qcow.c                                |  6 +++---
>  block/qcow2.c                               | 14 +++++++-------
>  block/qed.c                                 |  6 +++---
>  block/vdi.c                                 |  6 +++---
>  block/vhdx.c                                |  6 +++---
>  block/vmdk.c                                | 18 +++++++++---------
>  block/vpc.c                                 |  6 +++---
>  12 files changed, 44 insertions(+), 40 deletions(-)
>
> diff --git a/include/block/block-global-state.h
> b/include/block/block-global-state.h
> index 2c312cc774..ec3ddb17a8 100644
> --- a/include/block/block-global-state.h
> +++ b/include/block/block-global-state.h
> @@ -218,7 +218,8 @@ void bdrv_img_create(const char *filename, const char
> *fmt,
>                       bool quiet, Error **errp);
>
>  void bdrv_ref(BlockDriverState *bs);
> -void bdrv_unref(BlockDriverState *bs);
> +void no_coroutine_fn bdrv_unref(BlockDriverState *bs);
> +void coroutine_fn no_co_wrapper bdrv_co_unref(BlockDriverState *bs);
>  void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);
>  BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
>                               BlockDriverState *child_bs,
> diff --git a/include/sysemu/block-backend-global-state.h
> b/include/sysemu/block-backend-global-state.h
> index 2b6d27db7c..fa83f9389c 100644
> --- a/include/sysemu/block-backend-global-state.h
> +++ b/include/sysemu/block-backend-global-state.h
> @@ -42,7 +42,10 @@ blk_co_new_open(const char *filename, const char
> *reference, QDict *options,
>
>  int blk_get_refcnt(BlockBackend *blk);
>  void blk_ref(BlockBackend *blk);
> -void blk_unref(BlockBackend *blk);
> +
> +void no_coroutine_fn blk_unref(BlockBackend *blk);
> +void coroutine_fn no_co_wrapper blk_co_unref(BlockBackend *blk);
> +
>  void blk_remove_all_bs(void);
>  BlockBackend *blk_by_name(const char *name);
>  BlockBackend *blk_next(BlockBackend *blk);
> diff --git a/block.c b/block.c
> index 5ec1a3897e..20d5ee0959 100644
> --- a/block.c
> +++ b/block.c
> @@ -680,7 +680,7 @@ int coroutine_fn
> bdrv_co_create_opts_simple(BlockDriver *drv,
>
>      ret = 0;
>  out:
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      return ret;
>  }
>
> diff --git a/block/crypto.c b/block/crypto.c
> index ca67289187..8fd3ad0054 100644
> --- a/block/crypto.c
> +++ b/block/crypto.c
> @@ -355,7 +355,7 @@ block_crypto_co_create_generic(BlockDriverState *bs,
> int64_t size,
>      ret = 0;
>   cleanup:
>      qcrypto_block_free(crypto);
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      return ret;
>  }
>
> @@ -661,7 +661,7 @@ block_crypto_co_create_luks(BlockdevCreateOptions
> *create_options, Error **errp)
>
>      ret = 0;
>  fail:
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      return ret;
>  }
>
> @@ -730,7 +730,7 @@ fail:
>          bdrv_co_delete_file_noerr(bs);
>      }
>
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_QCryptoBlockCreateOptions(create_opts);
>      qobject_unref(cryptoopts);
>      return ret;
> diff --git a/block/parallels.c b/block/parallels.c
> index 013684801a..b49c35929e 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -613,8 +613,8 @@ static int coroutine_fn
> parallels_co_create(BlockdevCreateOptions* opts,
>
>      ret = 0;
>  out:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      return ret;
>
>  exit:
> @@ -691,7 +691,7 @@ parallels_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  done:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/qcow.c b/block/qcow.c
> index 490e4f819e..a0c701f578 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -915,8 +915,8 @@ static int coroutine_fn
> qcow_co_create(BlockdevCreateOptions *opts,
>      g_free(tmp);
>      ret = 0;
>  exit:
> -    blk_unref(qcow_blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(qcow_blk);
> +    bdrv_co_unref(bs);
>      qcrypto_block_free(crypto);
>      return ret;
>  }
> @@ -1015,7 +1015,7 @@ qcow_co_create_opts(BlockDriver *drv, const char
> *filename,
>  fail:
>      g_free(backing_fmt);
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 94cf59af8b..01742b3ebe 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3705,7 +3705,7 @@ qcow2_co_create(BlockdevCreateOptions
> *create_options, Error **errp)
>          goto out;
>      }
>
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      blk = NULL;
>
>      /*
> @@ -3785,7 +3785,7 @@ qcow2_co_create(BlockdevCreateOptions
> *create_options, Error **errp)
>          }
>      }
>
> -    blk_unref(blk);
> +    blk_co_unref(blk);
>      blk = NULL;
>
>      /* Reopen the image without BDRV_O_NO_FLUSH to flush it before
> returning.
> @@ -3810,9 +3810,9 @@ qcow2_co_create(BlockdevCreateOptions
> *create_options, Error **errp)
>
>      ret = 0;
>  out:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> -    bdrv_unref(data_bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
> +    bdrv_co_unref(data_bs);
>      return ret;
>  }
>
> @@ -3943,8 +3943,8 @@ finish:
>      }
>
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> -    bdrv_unref(data_bs);
> +    bdrv_co_unref(bs);
> +    bdrv_co_unref(data_bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/qed.c b/block/qed.c
> index 0705a7b4e2..aff2a2076e 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -748,8 +748,8 @@ static int coroutine_fn
> bdrv_qed_co_create(BlockdevCreateOptions *opts,
>      ret = 0; /* success */
>  out:
>      g_free(l1_table);
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      return ret;
>  }
>
> @@ -819,7 +819,7 @@ bdrv_qed_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  fail:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/vdi.c b/block/vdi.c
> index f2434d6153..08331d2dd7 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -886,8 +886,8 @@ static int coroutine_fn
> vdi_co_do_create(BlockdevCreateOptions *create_options,
>
>      ret = 0;
>  exit:
> -    blk_unref(blk);
> -    bdrv_unref(bs_file);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs_file);
>      g_free(bmap);
>      return ret;
>  }
> @@ -975,7 +975,7 @@ vdi_co_create_opts(BlockDriver *drv, const char
> *filename,
>  done:
>      qobject_unref(qdict);
>      qapi_free_BlockdevCreateOptions(create_options);
> -    bdrv_unref(bs_file);
> +    bdrv_co_unref(bs_file);
>      return ret;
>  }
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 81420722a1..00777da91a 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -2053,8 +2053,8 @@ static int coroutine_fn
> vhdx_co_create(BlockdevCreateOptions *opts,
>
>      ret = 0;
>  delete_and_exit:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      g_free(creator);
>      return ret;
>  }
> @@ -2144,7 +2144,7 @@ vhdx_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  fail:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3f8c731e32..11b553ef25 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -2306,7 +2306,7 @@ exit:
>          if (pbb) {
>              *pbb = blk;
>          } else {
> -            blk_unref(blk);
> +            blk_co_unref(blk);
>              blk = NULL;
>          }
>      }
> @@ -2516,12 +2516,12 @@ vmdk_co_do_create(int64_t size,
>          if (strcmp(blk_bs(backing)->drv->format_name, "vmdk")) {
>              error_setg(errp, "Invalid backing file format: %s. Must be
> vmdk",
>                         blk_bs(backing)->drv->format_name);
> -            blk_unref(backing);
> +            blk_co_unref(backing);
>              ret = -EINVAL;
>              goto exit;
>          }
>          ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
> -        blk_unref(backing);
> +        blk_co_unref(backing);
>          if (ret) {
>              error_setg(errp, "Failed to read parent CID");
>              goto exit;
> @@ -2542,14 +2542,14 @@ vmdk_co_do_create(int64_t size,
>                               blk_bs(extent_blk)->filename);
>          created_size += cur_size;
>          extent_idx++;
> -        blk_unref(extent_blk);
> +        blk_co_unref(extent_blk);
>      }
>
>      /* Check whether we got excess extents */
>      extent_blk = extent_fn(-1, extent_idx, flat, split, compress,
> zeroed_grain,
>                             opaque, NULL);
>      if (extent_blk) {
> -        blk_unref(extent_blk);
> +        blk_co_unref(extent_blk);
>          error_setg(errp, "List of extents contains unused extents");
>          ret = -EINVAL;
>          goto exit;
> @@ -2590,7 +2590,7 @@ vmdk_co_do_create(int64_t size,
>      ret = 0;
>  exit:
>      if (blk) {
> -        blk_unref(blk);
> +        blk_co_unref(blk);
>      }
>      g_free(desc);
>      g_free(parent_desc_line);
> @@ -2641,7 +2641,7 @@ vmdk_co_create_opts_cb(int64_t size, int idx, bool
> flat, bool split,
>                             errp)) {
>          goto exit;
>      }
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>  exit:
>      g_free(ext_filename);
>      return blk;
> @@ -2797,12 +2797,12 @@ static BlockBackend * coroutine_fn
> vmdk_co_create_cb(int64_t size, int idx,
>          return NULL;
>      }
>      blk_set_allow_write_beyond_eof(blk, true);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>
>      if (size != -1) {
>          ret = vmdk_init_extent(blk, size, flat, compress, zeroed_grain,
> errp);
>          if (ret) {
> -            blk_unref(blk);
> +            blk_co_unref(blk);
>              blk = NULL;
>          }
>      }
> diff --git a/block/vpc.c b/block/vpc.c
> index b89b0ff8e2..07ddda5b99 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -1082,8 +1082,8 @@ static int coroutine_fn
> vpc_co_create(BlockdevCreateOptions *opts,
>      }
>
>  out:
> -    blk_unref(blk);
> -    bdrv_unref(bs);
> +    blk_co_unref(blk);
> +    bdrv_co_unref(bs);
>      return ret;
>  }
>
> @@ -1162,7 +1162,7 @@ vpc_co_create_opts(BlockDriver *drv, const char
> *filename,
>
>  fail:
>      qobject_unref(qdict);
> -    bdrv_unref(bs);
> +    bdrv_co_unref(bs);
>      qapi_free_BlockdevCreateOptions(create_options);
>      return ret;
>  }
> --
> 2.40.0
>
>

[-- Attachment #2: Type: text/html, Size: 14192 bytes --]

Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Kevin Wolf]

Am 27.04.2023 um 16:30 hat Paolo Bonzini geschrieben:
> Il mar 25 apr 2023, 19:32 Kevin Wolf <kwolf@redhat.com> ha scritto:
> 
> > These functions must not be called in coroutine context, because they
> > need write access to the graph.
> >
> 
> With these patches applied vrc is still complaining about calls to
> bdrv_unref_child from qcow2_do_open and qcow2_do_close.

bdrv_unref_child() is addressed in one of the patches that I'll probably
send in the next batch, so it should be covered without additional work.

> Otherwise, the situation looks pretty good.

Thanks for checking!

By the way, and slightly unrelated, can vrc somehow help with finding
places that call coroutine wrappers without holding the AioContext lock?
(This results in an abort() when AIO_WAIT_WHILE() tries to unlock the
AioContext.) This is one of the classes of bugs we're seeing in 8.0.

Kevin

Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

Il gio 27 apr 2023, 19:00 Kevin Wolf <kwolf@redhat.com> ha scritto:

> By the way, and slightly unrelated, can vrc somehow help with finding
> places that call coroutine wrappers without holding the AioContext lock?
> (This results in an abort() when AIO_WAIT_WHILE() tries to unlock the
> AioContext.) This is one of the classes of bugs we're seeing in 8.0.
>

Seems more like a task for TSA.

Even though C TSA doesn't let you check that the *right* AioContext lock is
taken, it can check statically that *one* such lock is taken, and in
general I would guess it's rare for the wrong AioContext to be locked.

Paolo


> Kevin
>
>

[-- Attachment #2: Type: text/html, Size: 1270 bytes --]

Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Kevin Wolf]

Am 27.04.2023 um 22:49 hat Paolo Bonzini geschrieben:
> Il gio 27 apr 2023, 19:00 Kevin Wolf <kwolf@redhat.com> ha scritto:
> 
> > By the way, and slightly unrelated, can vrc somehow help with finding
> > places that call coroutine wrappers without holding the AioContext lock?
> > (This results in an abort() when AIO_WAIT_WHILE() tries to unlock the
> > AioContext.) This is one of the classes of bugs we're seeing in 8.0.
> >
> 
> Seems more like a task for TSA.
> 
> Even though C TSA doesn't let you check that the *right* AioContext lock is
> taken, it can check statically that *one* such lock is taken, and in
> general I would guess it's rare for the wrong AioContext to be locked.

The problem with TSA is that you need to add annotations everywhere, it
doesn't look at the full call graph. It would be almost the same effort
as for adding the graph lock annotation. I don't think we want to do
this for a lock that we want to remove relatively soon anyway.

I'd really prefer a tool where I don't need to modify the source code,
at least not more than saying "aio_context_acquire() takes the lock",
"aio_context_release() releases the lock" and "you must have the lock in
AIO_WAIT_WHILE()".

I guess the other option is manual auditing.

Kevin

Re: [PATCH 03/20] block: bdrv/blk_co_unref() for calls in coroutine context

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On Tue, Apr 25, 2023 at 07:31:41PM +0200, Kevin Wolf wrote:
> These functions must not be called in coroutine context, because they
> need write access to the graph.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h          |  3 ++-
>  include/sysemu/block-backend-global-state.h |  5 ++++-
>  block.c                                     |  2 +-
>  block/crypto.c                              |  6 +++---
>  block/parallels.c                           |  6 +++---
>  block/qcow.c                                |  6 +++---
>  block/qcow2.c                               | 14 +++++++-------
>  block/qed.c                                 |  6 +++---
>  block/vdi.c                                 |  6 +++---
>  block/vhdx.c                                |  6 +++---
>  block/vmdk.c                                | 18 +++++++++---------
>  block/vpc.c                                 |  6 +++---
>  12 files changed, 44 insertions(+), 40 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 04/20] block: Don't call no_coroutine_fns in qmp_block_resize()

[Kevin Wolf]

This QMP handler runs in a coroutine, so it must use the corresponding
no_co_wrappers instead.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d7b5c18f0a..eb509cf964 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2430,7 +2430,7 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,
         return;
     }
 
-    blk = blk_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
+    blk = blk_co_new_with_bs(bs, BLK_PERM_RESIZE, BLK_PERM_ALL, errp);
     if (!blk) {
         return;
     }
@@ -2445,7 +2445,7 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,
 
     bdrv_co_lock(bs);
     bdrv_drained_end(bs);
-    blk_unref(blk);
+    blk_co_unref(blk);
     bdrv_co_unlock(bs);
 }
 
-- 
2.40.0

Re: [PATCH 04/20] block: Don't call no_coroutine_fns in qmp_block_resize()

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:42PM +0200, Kevin Wolf wrote:
> This QMP handler runs in a coroutine, so it must use the corresponding
> no_co_wrappers instead.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

> 
> diff --git a/blockdev.c b/blockdev.c
> index d7b5c18f0a..eb509cf964 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2430,7 +2430,7 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,

Would it help matters to add another patch that changes the name of
the function to qmp_co_block_resize?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 04/20] block: Don't call no_coroutine_fns in qmp_block_resize()

[Kevin Wolf]

Am 25.04.2023 um 22:08 hat Eric Blake geschrieben:
> On Tue, Apr 25, 2023 at 07:31:42PM +0200, Kevin Wolf wrote:
> > This QMP handler runs in a coroutine, so it must use the corresponding
> > no_co_wrappers instead.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  blockdev.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index d7b5c18f0a..eb509cf964 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2430,7 +2430,7 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,
> 
> Would it help matters to add another patch that changes the name of
> the function to qmp_co_block_resize?

I'm not opposed to it, but you would have to change the QAPI generator
for that. Currently, it always calls qmp_$COMMAND_NAME() no matter
whether it is coroutine command handler or not.

Kevin

Re: [PATCH 04/20] block: Don't call no_coroutine_fns in qmp_block_resize()

[Eric Blake]

On Thu, Apr 27, 2023 at 01:15:08PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 22:08 hat Eric Blake geschrieben:
> > On Tue, Apr 25, 2023 at 07:31:42PM +0200, Kevin Wolf wrote:
> > > This QMP handler runs in a coroutine, so it must use the corresponding
> > > no_co_wrappers instead.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  blockdev.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> > > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index d7b5c18f0a..eb509cf964 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -2430,7 +2430,7 @@ void coroutine_fn qmp_block_resize(const char *device, const char *node_name,
> > 
> > Would it help matters to add another patch that changes the name of
> > the function to qmp_co_block_resize?
> 
> I'm not opposed to it, but you would have to change the QAPI generator
> for that. Currently, it always calls qmp_$COMMAND_NAME() no matter
> whether it is coroutine command handler or not.

Oh, that indeed is a bigger task, not worth holding up this series
for.

But the QAPI generater DOES have the 'coroutine':true information
available, if we wanted to automate that.  Having a naming convention
that reflects usage patterns is helpful during code reviews.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 04/20] block: Don't call no_coroutine_fns in qmp_block_resize()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

On Tue, Apr 25, 2023 at 07:31:42PM +0200, Kevin Wolf wrote:
> This QMP handler runs in a coroutine, so it must use the corresponding
> no_co_wrappers instead.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 05/20] test-bdrv-drain: Don't modify the graph in coroutines

[Kevin Wolf]

test-bdrv-drain contains a few test cases that are run both in coroutine
and non-coroutine context. Running the entire code including the setup
and shutdown in coroutines is incorrect because graph modifications can
generally not happen in coroutines.

Change the test so that creating and destroying the test nodes and
BlockBackends always happens outside of coroutine context.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/unit/test-bdrv-drain.c | 112 +++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 37 deletions(-)

diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index d9d3807062..765ae382da 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -188,6 +188,25 @@ static void do_drain_begin_unlocked(enum drain_type drain_type, BlockDriverState
     }
 }
 
+static BlockBackend * no_coroutine_fn test_setup(void)
+{
+    BlockBackend *blk;
+    BlockDriverState *bs, *backing;
+
+    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
+    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
+                              &error_abort);
+    blk_insert_bs(blk, bs, &error_abort);
+
+    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
+    bdrv_set_backing_hd(bs, backing, &error_abort);
+
+    bdrv_unref(backing);
+    bdrv_unref(bs);
+
+    return blk;
+}
+
 static void do_drain_end_unlocked(enum drain_type drain_type, BlockDriverState *bs)
 {
     if (drain_type != BDRV_DRAIN_ALL) {
@@ -199,25 +218,19 @@ static void do_drain_end_unlocked(enum drain_type drain_type, BlockDriverState *
     }
 }
 
-static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
+static void test_drv_cb_common(BlockBackend *blk, enum drain_type drain_type,
+                               bool recursive)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs, *backing;
+    BlockDriverState *bs = blk_bs(blk);
+    BlockDriverState *backing = bs->backing->bs;
     BDRVTestState *s, *backing_s;
     BlockAIOCB *acb;
     int aio_ret;
 
     QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
 
-    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
-    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
-                              &error_abort);
     s = bs->opaque;
-    blk_insert_bs(blk, bs, &error_abort);
-
-    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
     backing_s = backing->opaque;
-    bdrv_set_backing_hd(bs, backing, &error_abort);
 
     /* Simple bdrv_drain_all_begin/end pair, check that CBs are called */
     g_assert_cmpint(s->drain_count, ==, 0);
@@ -252,44 +265,53 @@ static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
 
     g_assert_cmpint(s->drain_count, ==, 0);
     g_assert_cmpint(backing_s->drain_count, ==, 0);
-
-    bdrv_unref(backing);
-    bdrv_unref(bs);
-    blk_unref(blk);
 }
 
 static void test_drv_cb_drain_all(void)
 {
-    test_drv_cb_common(BDRV_DRAIN_ALL, true);
+    BlockBackend *blk = test_setup();
+    test_drv_cb_common(blk, BDRV_DRAIN_ALL, true);
+    blk_unref(blk);
 }
 
 static void test_drv_cb_drain(void)
 {
-    test_drv_cb_common(BDRV_DRAIN, false);
+    BlockBackend *blk = test_setup();
+    test_drv_cb_common(blk, BDRV_DRAIN, false);
+    blk_unref(blk);
+}
+
+static void test_drv_cb_co_drain_all_entry(void)
+{
+    BlockBackend *blk = blk_all_next(NULL);
+    test_drv_cb_common(blk, BDRV_DRAIN_ALL, true);
 }
 
 static void test_drv_cb_co_drain_all(void)
 {
-    call_in_coroutine(test_drv_cb_drain_all);
+    BlockBackend *blk = test_setup();
+    call_in_coroutine(test_drv_cb_co_drain_all_entry);
+    blk_unref(blk);
 }
 
-static void test_drv_cb_co_drain(void)
+static void test_drv_cb_co_drain_entry(void)
 {
-    call_in_coroutine(test_drv_cb_drain);
+    BlockBackend *blk = blk_all_next(NULL);
+    test_drv_cb_common(blk, BDRV_DRAIN, false);
 }
 
-static void test_quiesce_common(enum drain_type drain_type, bool recursive)
+static void test_drv_cb_co_drain(void)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs, *backing;
-
-    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
-    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
-                              &error_abort);
-    blk_insert_bs(blk, bs, &error_abort);
+    BlockBackend *blk = test_setup();
+    call_in_coroutine(test_drv_cb_co_drain_entry);
+    blk_unref(blk);
+}
 
-    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
-    bdrv_set_backing_hd(bs, backing, &error_abort);
+static void test_quiesce_common(BlockBackend *blk, enum drain_type drain_type,
+                                bool recursive)
+{
+    BlockDriverState *bs = blk_bs(blk);
+    BlockDriverState *backing = bs->backing->bs;
 
     g_assert_cmpint(bs->quiesce_counter, ==, 0);
     g_assert_cmpint(backing->quiesce_counter, ==, 0);
@@ -307,30 +329,46 @@ static void test_quiesce_common(enum drain_type drain_type, bool recursive)
 
     g_assert_cmpint(bs->quiesce_counter, ==, 0);
     g_assert_cmpint(backing->quiesce_counter, ==, 0);
-
-    bdrv_unref(backing);
-    bdrv_unref(bs);
-    blk_unref(blk);
 }
 
 static void test_quiesce_drain_all(void)
 {
-    test_quiesce_common(BDRV_DRAIN_ALL, true);
+    BlockBackend *blk = test_setup();
+    test_quiesce_common(blk, BDRV_DRAIN_ALL, true);
+    blk_unref(blk);
 }
 
 static void test_quiesce_drain(void)
 {
-    test_quiesce_common(BDRV_DRAIN, false);
+    BlockBackend *blk = test_setup();
+    test_quiesce_common(blk, BDRV_DRAIN, false);
+    blk_unref(blk);
+}
+
+static void test_quiesce_co_drain_all_entry(void)
+{
+    BlockBackend *blk = blk_all_next(NULL);
+    test_quiesce_common(blk, BDRV_DRAIN_ALL, true);
 }
 
 static void test_quiesce_co_drain_all(void)
 {
-    call_in_coroutine(test_quiesce_drain_all);
+    BlockBackend *blk = test_setup();
+    call_in_coroutine(test_quiesce_co_drain_all_entry);
+    blk_unref(blk);
+}
+
+static void test_quiesce_co_drain_entry(void)
+{
+    BlockBackend *blk = blk_all_next(NULL);
+    test_quiesce_common(blk, BDRV_DRAIN, false);
 }
 
 static void test_quiesce_co_drain(void)
 {
-    call_in_coroutine(test_quiesce_drain);
+    BlockBackend *blk = test_setup();
+    call_in_coroutine(test_quiesce_co_drain_entry);
+    blk_unref(blk);
 }
 
 static void test_nested(void)
-- 
2.40.0

Re: [PATCH 05/20] test-bdrv-drain: Don't modify the graph in coroutines

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:43PM +0200, Kevin Wolf wrote:
> test-bdrv-drain contains a few test cases that are run both in coroutine
> and non-coroutine context. Running the entire code including the setup
> and shutdown in coroutines is incorrect because graph modifications can
> generally not happen in coroutines.
> 
> Change the test so that creating and destroying the test nodes and
> BlockBackends always happens outside of coroutine context.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/unit/test-bdrv-drain.c | 112 +++++++++++++++++++++++------------
>  1 file changed, 75 insertions(+), 37 deletions(-)
>

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 05/20] test-bdrv-drain: Don't modify the graph in coroutines

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 7200 bytes --]

On Tue, Apr 25, 2023 at 07:31:43PM +0200, Kevin Wolf wrote:
> test-bdrv-drain contains a few test cases that are run both in coroutine
> and non-coroutine context. Running the entire code including the setup
> and shutdown in coroutines is incorrect because graph modifications can
> generally not happen in coroutines.
> 
> Change the test so that creating and destroying the test nodes and
> BlockBackends always happens outside of coroutine context.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/unit/test-bdrv-drain.c | 112 +++++++++++++++++++++++------------
>  1 file changed, 75 insertions(+), 37 deletions(-)
> 
> diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
> index d9d3807062..765ae382da 100644
> --- a/tests/unit/test-bdrv-drain.c
> +++ b/tests/unit/test-bdrv-drain.c
> @@ -188,6 +188,25 @@ static void do_drain_begin_unlocked(enum drain_type drain_type, BlockDriverState
>      }
>  }
>  
> +static BlockBackend * no_coroutine_fn test_setup(void)
> +{
> +    BlockBackend *blk;
> +    BlockDriverState *bs, *backing;
> +
> +    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> +    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
> +                              &error_abort);
> +    blk_insert_bs(blk, bs, &error_abort);
> +
> +    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
> +    bdrv_set_backing_hd(bs, backing, &error_abort);
> +
> +    bdrv_unref(backing);
> +    bdrv_unref(bs);
> +
> +    return blk;
> +}
> +
>  static void do_drain_end_unlocked(enum drain_type drain_type, BlockDriverState *bs)
>  {
>      if (drain_type != BDRV_DRAIN_ALL) {
> @@ -199,25 +218,19 @@ static void do_drain_end_unlocked(enum drain_type drain_type, BlockDriverState *
>      }
>  }
>  
> -static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
> +static void test_drv_cb_common(BlockBackend *blk, enum drain_type drain_type,
> +                               bool recursive)
>  {
> -    BlockBackend *blk;
> -    BlockDriverState *bs, *backing;
> +    BlockDriverState *bs = blk_bs(blk);
> +    BlockDriverState *backing = bs->backing->bs;
>      BDRVTestState *s, *backing_s;
>      BlockAIOCB *acb;
>      int aio_ret;
>  
>      QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, NULL, 0);
>  
> -    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> -    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
> -                              &error_abort);
>      s = bs->opaque;
> -    blk_insert_bs(blk, bs, &error_abort);
> -
> -    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
>      backing_s = backing->opaque;
> -    bdrv_set_backing_hd(bs, backing, &error_abort);
>  
>      /* Simple bdrv_drain_all_begin/end pair, check that CBs are called */
>      g_assert_cmpint(s->drain_count, ==, 0);
> @@ -252,44 +265,53 @@ static void test_drv_cb_common(enum drain_type drain_type, bool recursive)
>  
>      g_assert_cmpint(s->drain_count, ==, 0);
>      g_assert_cmpint(backing_s->drain_count, ==, 0);
> -
> -    bdrv_unref(backing);
> -    bdrv_unref(bs);
> -    blk_unref(blk);
>  }
>  
>  static void test_drv_cb_drain_all(void)
>  {
> -    test_drv_cb_common(BDRV_DRAIN_ALL, true);
> +    BlockBackend *blk = test_setup();
> +    test_drv_cb_common(blk, BDRV_DRAIN_ALL, true);
> +    blk_unref(blk);
>  }
>  
>  static void test_drv_cb_drain(void)
>  {
> -    test_drv_cb_common(BDRV_DRAIN, false);
> +    BlockBackend *blk = test_setup();
> +    test_drv_cb_common(blk, BDRV_DRAIN, false);
> +    blk_unref(blk);
> +}
> +
> +static void test_drv_cb_co_drain_all_entry(void)

Missing coroutine_fn.

> +{
> +    BlockBackend *blk = blk_all_next(NULL);
> +    test_drv_cb_common(blk, BDRV_DRAIN_ALL, true);
>  }
>  
>  static void test_drv_cb_co_drain_all(void)
>  {
> -    call_in_coroutine(test_drv_cb_drain_all);
> +    BlockBackend *blk = test_setup();
> +    call_in_coroutine(test_drv_cb_co_drain_all_entry);
> +    blk_unref(blk);
>  }
>  
> -static void test_drv_cb_co_drain(void)
> +static void test_drv_cb_co_drain_entry(void)

Missing coroutine_fn.

>  {
> -    call_in_coroutine(test_drv_cb_drain);
> +    BlockBackend *blk = blk_all_next(NULL);
> +    test_drv_cb_common(blk, BDRV_DRAIN, false);
>  }
>  
> -static void test_quiesce_common(enum drain_type drain_type, bool recursive)
> +static void test_drv_cb_co_drain(void)
>  {
> -    BlockBackend *blk;
> -    BlockDriverState *bs, *backing;
> -
> -    blk = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
> -    bs = bdrv_new_open_driver(&bdrv_test, "test-node", BDRV_O_RDWR,
> -                              &error_abort);
> -    blk_insert_bs(blk, bs, &error_abort);
> +    BlockBackend *blk = test_setup();
> +    call_in_coroutine(test_drv_cb_co_drain_entry);
> +    blk_unref(blk);
> +}
>  
> -    backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
> -    bdrv_set_backing_hd(bs, backing, &error_abort);
> +static void test_quiesce_common(BlockBackend *blk, enum drain_type drain_type,
> +                                bool recursive)
> +{
> +    BlockDriverState *bs = blk_bs(blk);
> +    BlockDriverState *backing = bs->backing->bs;
>  
>      g_assert_cmpint(bs->quiesce_counter, ==, 0);
>      g_assert_cmpint(backing->quiesce_counter, ==, 0);
> @@ -307,30 +329,46 @@ static void test_quiesce_common(enum drain_type drain_type, bool recursive)
>  
>      g_assert_cmpint(bs->quiesce_counter, ==, 0);
>      g_assert_cmpint(backing->quiesce_counter, ==, 0);
> -
> -    bdrv_unref(backing);
> -    bdrv_unref(bs);
> -    blk_unref(blk);
>  }
>  
>  static void test_quiesce_drain_all(void)
>  {
> -    test_quiesce_common(BDRV_DRAIN_ALL, true);
> +    BlockBackend *blk = test_setup();
> +    test_quiesce_common(blk, BDRV_DRAIN_ALL, true);
> +    blk_unref(blk);
>  }
>  
>  static void test_quiesce_drain(void)
>  {
> -    test_quiesce_common(BDRV_DRAIN, false);
> +    BlockBackend *blk = test_setup();
> +    test_quiesce_common(blk, BDRV_DRAIN, false);
> +    blk_unref(blk);
> +}
> +
> +static void test_quiesce_co_drain_all_entry(void)

Missing coroutine_fn.

> +{
> +    BlockBackend *blk = blk_all_next(NULL);
> +    test_quiesce_common(blk, BDRV_DRAIN_ALL, true);
>  }
>  
>  static void test_quiesce_co_drain_all(void)
>  {
> -    call_in_coroutine(test_quiesce_drain_all);
> +    BlockBackend *blk = test_setup();
> +    call_in_coroutine(test_quiesce_co_drain_all_entry);
> +    blk_unref(blk);
> +}
> +
> +static void test_quiesce_co_drain_entry(void)

Missing coroutine_fn.

> +{
> +    BlockBackend *blk = blk_all_next(NULL);
> +    test_quiesce_common(blk, BDRV_DRAIN, false);
>  }
>  
>  static void test_quiesce_co_drain(void)
>  {
> -    call_in_coroutine(test_quiesce_drain);
> +    BlockBackend *blk = test_setup();
> +    call_in_coroutine(test_quiesce_co_drain_entry);
> +    blk_unref(blk);
>  }
>  
>  static void test_nested(void)
> -- 
> 2.40.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 06/20] graph-lock: Add GRAPH_UNLOCKED(_PTR)

[Kevin Wolf]

For some function, parts of their interface is that are called without
holding the graph lock. Add a new macro to document this.

The macro expands to TSA_EXCLUDES(), which is a relatively weak check
because it passes in cases where the compiler just doesn't know if the
lock is held. Function pointers can't be checked at all. Therefore, its
primary purpose is documentation.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/graph-lock.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index 18cc14de22..7ef391fab3 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -73,6 +73,7 @@ extern BdrvGraphLock graph_lock;
  */
 #define GRAPH_WRLOCK TSA_REQUIRES(graph_lock)
 #define GRAPH_RDLOCK TSA_REQUIRES_SHARED(graph_lock)
+#define GRAPH_UNLOCKED TSA_EXCLUDES(graph_lock)
 
 /*
  * TSA annotations are not part of function types, so checks are defeated when
@@ -83,6 +84,7 @@ extern BdrvGraphLock graph_lock;
  */
 #define GRAPH_RDLOCK_PTR TSA_GUARDED_BY(graph_lock)
 #define GRAPH_WRLOCK_PTR TSA_GUARDED_BY(graph_lock)
+#define GRAPH_UNLOCKED_PTR
 
 /*
  * register_aiocontext:
-- 
2.40.0

Re: [PATCH 06/20] graph-lock: Add GRAPH_UNLOCKED(_PTR)

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:44PM +0200, Kevin Wolf wrote:
> For some function, parts of their interface is that are called without
> holding the graph lock. Add a new macro to document this.

Grammar is off; maybe:

For some functions, it is part of their interface to be called without
holding the graph lock.

> 
> The macro expands to TSA_EXCLUDES(), which is a relatively weak check
> because it passes in cases where the compiler just doesn't know if the
> lock is held. Function pointers can't be checked at all. Therefore, its
> primary purpose is documentation.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/graph-lock.h | 2 ++
>  1 file changed, 2 insertions(+)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 06/20] graph-lock: Add GRAPH_UNLOCKED(_PTR)

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Tue, Apr 25, 2023 at 07:31:44PM +0200, Kevin Wolf wrote:
> For some function, parts of their interface is that are called without
> holding the graph lock. Add a new macro to document this.
> 
> The macro expands to TSA_EXCLUDES(), which is a relatively weak check
> because it passes in cases where the compiler just doesn't know if the
> lock is held. Function pointers can't be checked at all. Therefore, its
> primary purpose is documentation.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/graph-lock.h | 2 ++
>  1 file changed, 2 insertions(+)

Modulo Eric's comment about the commit description:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock

[Kevin Wolf]

GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a
reader lock for the graph, so the correct annotation for them to use is
TSA_ASSERT_SHARED rather than TSA_ASSERT.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/graph-lock.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index 7ef391fab3..adaa3ed089 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable;
  * unlocked. TSA_ASSERT() makes sure that the following calls know that we
  * hold the lock while unlocking is left unchecked.
  */
-static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA
+static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA
 graph_lockable_auto_lock(GraphLockable *x)
 {
     bdrv_graph_co_rdlock();
@@ -254,7 +254,7 @@ typedef struct GraphLockableMainloop { } GraphLockableMainloop;
  * unlocked. TSA_ASSERT() makes sure that the following calls know that we
  * hold the lock while unlocking is left unchecked.
  */
-static inline GraphLockableMainloop * TSA_ASSERT(graph_lock) TSA_NO_TSA
+static inline GraphLockableMainloop * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA
 graph_lockable_auto_lock_mainloop(GraphLockableMainloop *x)
 {
     bdrv_graph_rdlock_main_loop();
-- 
2.40.0

Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:45PM +0200, Kevin Wolf wrote:
> GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a
> reader lock for the graph, so the correct annotation for them to use is
> TSA_ASSERT_SHARED rather than TSA_ASSERT.

The comments at the start of graph-lock.h state that there is only 1
writer (main loop, under BQL), and all others are readers (coroutines
in varous AioContext) - but that's regarding the main BdrvGraphRWlock.
I guess my confusion is over the act of writing the graph (only in the
main loop) and using TSA annotations to check for safety.  Am I
correct that the reason graph_lockable_auto_lock() only needs a
TSA_ASSERT_SHARED locking is that it is only reachable from the other
threads (and not the main loop thread itself) to check that we are
indeed in a point where we aren't contending with the main loop's
writable lock?

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/graph-lock.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
> index 7ef391fab3..adaa3ed089 100644
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable;
>   * unlocked. TSA_ASSERT() makes sure that the following calls know that we
>   * hold the lock while unlocking is left unchecked.
>   */
> -static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA
> +static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA
>  graph_lockable_auto_lock(GraphLockable *x)
>  {
>      bdrv_graph_co_rdlock();

But the act of grabbing a rdlock is definitely a SHARED not EXCLUSIVE
action, so I think I agree with your changing of the annotation, even
if the commit message could be slightly improved.

Assuming I've sorted out my own confusion on the various lock
terminoligies,

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock

[Kevin Wolf]

Am 25.04.2023 um 22:36 hat Eric Blake geschrieben:
> On Tue, Apr 25, 2023 at 07:31:45PM +0200, Kevin Wolf wrote:
> > GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a
> > reader lock for the graph, so the correct annotation for them to use is
> > TSA_ASSERT_SHARED rather than TSA_ASSERT.
> 
> The comments at the start of graph-lock.h state that there is only 1
> writer (main loop, under BQL), and all others are readers (coroutines
> in varous AioContext) - but that's regarding the main BdrvGraphRWlock.

I think much of that comment is actually unrelated to BdrvGraphRWlock
(which tracks lock/unlock operations of a single thread), but to graph
locking in general.

> I guess my confusion is over the act of writing the graph (only in the
> main loop) and using TSA annotations to check for safety.  Am I
> correct that the reason graph_lockable_auto_lock() only needs a
> TSA_ASSERT_SHARED locking is that it is only reachable from the other
> threads (and not the main loop thread itself) to check that we are
> indeed in a point where we aren't contending with the main loop's
> writable lock?

TSA_ASSERT_SHARED is not a precondition requirement, but a postcondition
assertion. That is, callers of the function can assume that they hold
the lock after this function returns.

This should really be TSA_ACQUIRE_SHARED for graph_lockable_auto_lock(),
but as the comment above it states, this is impossible because TSA
doesn't understand unlocking via the cleanup attribute.

"shared" and "exclusive" in TSA map to "reader" and "writer" lock of a
RWLock. So since we're only taking a reader lock in this function, we
can only assert that the caller now holds a shared lock (which allows
you to call GRAPH_RDLOCK functions), but not an exclusive one like the
code previously suggested (this would allow you to call GRAPH_WRLOCK
functions).

I'm not sure if this fully addresses your confusion yet. Feel free to
ask if there are more unclear parts.

Kevin

Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock

[Eric Blake]

On Thu, Apr 27, 2023 at 01:28:15PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 22:36 hat Eric Blake geschrieben:
> > On Tue, Apr 25, 2023 at 07:31:45PM +0200, Kevin Wolf wrote:
> > > GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a
> > > reader lock for the graph, so the correct annotation for them to use is
> > > TSA_ASSERT_SHARED rather than TSA_ASSERT.
> > 
> > The comments at the start of graph-lock.h state that there is only 1
> > writer (main loop, under BQL), and all others are readers (coroutines
> > in varous AioContext) - but that's regarding the main BdrvGraphRWlock.
> 
> I think much of that comment is actually unrelated to BdrvGraphRWlock
> (which tracks lock/unlock operations of a single thread), but to graph
> locking in general.

Yeah, I ended up at the same point - writing the graph vs. grabbing a
reader/writer lock as a writer are not the same thing, so the comments
at the start of the file are unrelated to the TSA annotations.

> 
> > I guess my confusion is over the act of writing the graph (only in the
> > main loop) and using TSA annotations to check for safety.  Am I
> > correct that the reason graph_lockable_auto_lock() only needs a
> > TSA_ASSERT_SHARED locking is that it is only reachable from the other
> > threads (and not the main loop thread itself) to check that we are
> > indeed in a point where we aren't contending with the main loop's
> > writable lock?
> 
> TSA_ASSERT_SHARED is not a precondition requirement, but a postcondition
> assertion. That is, callers of the function can assume that they hold
> the lock after this function returns.
> 
> This should really be TSA_ACQUIRE_SHARED for graph_lockable_auto_lock(),
> but as the comment above it states, this is impossible because TSA
> doesn't understand unlocking via the cleanup attribute.

clang has really dropped the ball on their cleanup attribute handling;
it's been a known issue for years now, and could make their static
checking so much more powerful if we didn't have to keep working
around it.

> 
> "shared" and "exclusive" in TSA map to "reader" and "writer" lock of a
> RWLock. So since we're only taking a reader lock in this function, we
> can only assert that the caller now holds a shared lock (which allows
> you to call GRAPH_RDLOCK functions), but not an exclusive one like the
> code previously suggested (this would allow you to call GRAPH_WRLOCK
> functions).
> 
> I'm not sure if this fully addresses your confusion yet. Feel free to
> ask if there are more unclear parts.

At this point, I don't have any wording suggestions, and appreciate
your responses confirming what I arrived at on my own, so my R-b still
stands.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 07/20] graph-lock: Fix GRAPH_RDLOCK_GUARD*() to be reader lock

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1272 bytes --]

On Tue, Apr 25, 2023 at 07:31:45PM +0200, Kevin Wolf wrote:
> GRAPH_RDLOCK_GUARD() and GRAPH_RDLOCK_GUARD_MAINLOOP() only take a
> reader lock for the graph, so the correct annotation for them to use is
> TSA_ASSERT_SHARED rather than TSA_ASSERT.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/graph-lock.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
> index 7ef391fab3..adaa3ed089 100644
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -210,7 +210,7 @@ typedef struct GraphLockable { } GraphLockable;
>   * unlocked. TSA_ASSERT() makes sure that the following calls know that we

Does this comment need to be updated to TSA_ASSERT_SHARED()?

>   * hold the lock while unlocking is left unchecked.
>   */
> -static inline GraphLockable * TSA_ASSERT(graph_lock) TSA_NO_TSA
> +static inline GraphLockable * TSA_ASSERT_SHARED(graph_lock) TSA_NO_TSA
>  graph_lockable_auto_lock(GraphLockable *x)
>  {
>      bdrv_graph_co_rdlock();
> @@ -254,7 +254,7 @@ typedef struct GraphLockableMainloop { } GraphLockableMainloop;
>   * unlocked. TSA_ASSERT() makes sure that the following calls know that we

Same.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 08/20] block: .bdrv_open is non-coroutine and unlocked

[Kevin Wolf]

Drivers were a bit confused about whether .bdrv_open can run in a
coroutine and whether or not it holds a graph lock.

It cannot keep a graph lock from the caller across the whole function
because it both changes the graph (requires a writer lock) and does I/O
(requires a reader lock). Therefore, it should take these locks
internally as needed.

The functions used to be called in coroutine context during image
creation. This was buggy for other reasons, and as of commit 32192301,
all block drivers go through no_co_wrappers. So it is not called in
coroutine context any more.

Fix qcow2 and qed to work with the correct assumptions: The graph lock
needs to be taken internally instead of just assuming it's already
there, and the coroutine path is dead code that can be removed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int-common.h |  8 ++++----
 block.c                          |  6 +++---
 block/qcow2.c                    | 15 ++++++---------
 block/qed.c                      | 18 ++++++++----------
 4 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 013d419444..6fb28cd8fa 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -236,12 +236,12 @@ struct BlockDriver {
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
     void (*bdrv_join_options)(QDict *options, QDict *old_options);
 
-    int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
-                     Error **errp);
+    int GRAPH_UNLOCKED_PTR (*bdrv_open)(
+        BlockDriverState *bs, QDict *options, int flags, Error **errp);
 
     /* Protocol drivers should implement this instead of bdrv_open */
-    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
-                          Error **errp);
+    int GRAPH_UNLOCKED_PTR (*bdrv_file_open)(
+        BlockDriverState *bs, QDict *options, int flags, Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
 
     int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_create)(
diff --git a/block.c b/block.c
index 20d5ee0959..abec940867 100644
--- a/block.c
+++ b/block.c
@@ -1610,9 +1610,9 @@ out:
  * bdrv_refresh_total_sectors() which polls when called from non-coroutine
  * context.
  */
-static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
-                            const char *node_name, QDict *options,
-                            int open_flags, Error **errp)
+static int no_coroutine_fn GRAPH_UNLOCKED
+bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name,
+                 QDict *options, int open_flags, Error **errp)
 {
     Error *local_err = NULL;
     int i, ret;
diff --git a/block/qcow2.c b/block/qcow2.c
index 01742b3ebe..5bde3b8401 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1891,7 +1891,7 @@ static void coroutine_fn qcow2_open_entry(void *opaque)
     QCow2OpenCo *qoc = opaque;
     BDRVQcow2State *s = qoc->bs->opaque;
 
-    assume_graph_lock(); /* FIXME */
+    GRAPH_RDLOCK_GUARD();
 
     qemu_co_mutex_lock(&s->lock);
     qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, true,
@@ -1920,14 +1920,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     /* Initialise locks */
     qemu_co_mutex_init(&s->lock);
 
-    if (qemu_in_coroutine()) {
-        /* From bdrv_co_create.  */
-        qcow2_open_entry(&qoc);
-    } else {
-        assert(qemu_get_current_aio_context() == qemu_get_aio_context());
-        qemu_coroutine_enter(qemu_coroutine_create(qcow2_open_entry, &qoc));
-        BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
-    }
+    assert(!qemu_in_coroutine());
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+    qemu_coroutine_enter(qemu_coroutine_create(qcow2_open_entry, &qoc));
+    BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
+
     return qoc.ret;
 }
 
diff --git a/block/qed.c b/block/qed.c
index aff2a2076e..be9ff0fb34 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -557,11 +557,13 @@ typedef struct QEDOpenCo {
     int ret;
 } QEDOpenCo;
 
-static void coroutine_fn GRAPH_RDLOCK bdrv_qed_open_entry(void *opaque)
+static void coroutine_fn bdrv_qed_open_entry(void *opaque)
 {
     QEDOpenCo *qoc = opaque;
     BDRVQEDState *s = qoc->bs->opaque;
 
+    GRAPH_RDLOCK_GUARD();
+
     qemu_co_mutex_lock(&s->table_lock);
     qoc->ret = bdrv_qed_do_open(qoc->bs, qoc->options, qoc->flags, qoc->errp);
     qemu_co_mutex_unlock(&s->table_lock);
@@ -579,21 +581,17 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     };
     int ret;
 
-    assume_graph_lock(); /* FIXME */
-
     ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
     if (ret < 0) {
         return ret;
     }
 
     bdrv_qed_init_state(bs);
-    if (qemu_in_coroutine()) {
-        bdrv_qed_open_entry(&qoc);
-    } else {
-        assert(qemu_get_current_aio_context() == qemu_get_aio_context());
-        qemu_coroutine_enter(qemu_coroutine_create(bdrv_qed_open_entry, &qoc));
-        BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
-    }
+    assert(!qemu_in_coroutine());
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+    qemu_coroutine_enter(qemu_coroutine_create(bdrv_qed_open_entry, &qoc));
+    BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
+
     return qoc.ret;
 }
 
-- 
2.40.0

Re: [PATCH 08/20] block: .bdrv_open is non-coroutine and unlocked

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:46PM +0200, Kevin Wolf wrote:
> Drivers were a bit confused about whether .bdrv_open can run in a
> coroutine and whether or not it holds a graph lock.
> 
> It cannot keep a graph lock from the caller across the whole function
> because it both changes the graph (requires a writer lock) and does I/O
> (requires a reader lock). Therefore, it should take these locks
> internally as needed.
> 
> The functions used to be called in coroutine context during image
> creation. This was buggy for other reasons, and as of commit 32192301,
> all block drivers go through no_co_wrappers. So it is not called in
> coroutine context any more.
> 
> Fix qcow2 and qed to work with the correct assumptions: The graph lock
> needs to be taken internally instead of just assuming it's already
> there, and the coroutine path is dead code that can be removed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int-common.h |  8 ++++----
>  block.c                          |  6 +++---
>  block/qcow2.c                    | 15 ++++++---------
>  block/qed.c                      | 18 ++++++++----------
>  4 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 013d419444..6fb28cd8fa 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -236,12 +236,12 @@ struct BlockDriver {
>      void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
>      void (*bdrv_join_options)(QDict *options, QDict *old_options);
>  
> -    int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
> -                     Error **errp);
> +    int GRAPH_UNLOCKED_PTR (*bdrv_open)(
> +        BlockDriverState *bs, QDict *options, int flags, Error **errp);
>  
>      /* Protocol drivers should implement this instead of bdrv_open */
> -    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> -                          Error **errp);
> +    int GRAPH_UNLOCKED_PTR (*bdrv_file_open)(
> +        BlockDriverState *bs, QDict *options, int flags, Error **errp);

May conflict with Paolo's work to eliminate bdrv_file_open as
redundant.  But that should be easy to figure out.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 08/20] block: .bdrv_open is non-coroutine and unlocked

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On Tue, Apr 25, 2023 at 07:31:46PM +0200, Kevin Wolf wrote:
> Drivers were a bit confused about whether .bdrv_open can run in a
> coroutine and whether or not it holds a graph lock.
> 
> It cannot keep a graph lock from the caller across the whole function
> because it both changes the graph (requires a writer lock) and does I/O
> (requires a reader lock). Therefore, it should take these locks
> internally as needed.
> 
> The functions used to be called in coroutine context during image
> creation. This was buggy for other reasons, and as of commit 32192301,
> all block drivers go through no_co_wrappers. So it is not called in
> coroutine context any more.
> 
> Fix qcow2 and qed to work with the correct assumptions: The graph lock
> needs to be taken internally instead of just assuming it's already
> there, and the coroutine path is dead code that can be removed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int-common.h |  8 ++++----
>  block.c                          |  6 +++---
>  block/qcow2.c                    | 15 ++++++---------
>  block/qed.c                      | 18 ++++++++----------
>  4 files changed, 21 insertions(+), 26 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 09/20] nbd: Remove nbd_co_flush() wrapper function

[Kevin Wolf]

The only thing nbd_co_flush() does is calling nbd_client_co_flush().
Just use that function directly in the BlockDriver definitions and
remove the wrapper.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/nbd.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index bf2894ad5c..d3ee256844 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1920,11 +1920,6 @@ fail:
     return ret;
 }
 
-static int coroutine_fn nbd_co_flush(BlockDriverState *bs)
-{
-    return nbd_client_co_flush(bs);
-}
-
 static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
@@ -2120,7 +2115,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
     .bdrv_co_pwrite_zeroes      = nbd_client_co_pwrite_zeroes,
     .bdrv_close                 = nbd_close,
-    .bdrv_co_flush_to_os        = nbd_co_flush,
+    .bdrv_co_flush_to_os        = nbd_client_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_co_truncate           = nbd_co_truncate,
@@ -2148,7 +2143,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
     .bdrv_co_pwrite_zeroes      = nbd_client_co_pwrite_zeroes,
     .bdrv_close                 = nbd_close,
-    .bdrv_co_flush_to_os        = nbd_co_flush,
+    .bdrv_co_flush_to_os        = nbd_client_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_co_truncate           = nbd_co_truncate,
@@ -2176,7 +2171,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_co_pwritev            = nbd_client_co_pwritev,
     .bdrv_co_pwrite_zeroes      = nbd_client_co_pwrite_zeroes,
     .bdrv_close                 = nbd_close,
-    .bdrv_co_flush_to_os        = nbd_co_flush,
+    .bdrv_co_flush_to_os        = nbd_client_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_co_truncate           = nbd_co_truncate,
-- 
2.40.0

Re: [PATCH 09/20] nbd: Remove nbd_co_flush() wrapper function

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:47PM +0200, Kevin Wolf wrote:
> The only thing nbd_co_flush() does is calling nbd_client_co_flush().

s/calling/call/

> Just use that function directly in the BlockDriver definitions and
> remove the wrapper.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/nbd.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 09/20] nbd: Remove nbd_co_flush() wrapper function

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

On Tue, Apr 25, 2023 at 07:31:47PM +0200, Kevin Wolf wrote:
> The only thing nbd_co_flush() does is calling nbd_client_co_flush().
> Just use that function directly in the BlockDriver definitions and
> remove the wrapper.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/nbd.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 10/20] nbd: Mark nbd_co_do_establish_connection() and callers GRAPH_RDLOCK

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

This adds GRAPH_RDLOCK annotations to declare that callers of
nbd_co_do_establish_connection() need to hold a reader lock for the
graph.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/coroutines.h |  5 +++--
 block/nbd.c        | 39 +++++++++++++++++++++------------------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/block/coroutines.h b/block/coroutines.h
index dd9f3d449b..f3226682d6 100644
--- a/block/coroutines.h
+++ b/block/coroutines.h
@@ -61,7 +61,7 @@ bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
 int coroutine_fn GRAPH_RDLOCK
 bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos);
 
-int coroutine_fn
+int coroutine_fn GRAPH_RDLOCK
 nbd_co_do_establish_connection(BlockDriverState *bs, bool blocking,
                                Error **errp);
 
@@ -85,7 +85,8 @@ bdrv_common_block_status_above(BlockDriverState *bs,
                                int64_t *map,
                                BlockDriverState **file,
                                int *depth);
-int co_wrapper_mixed
+
+int co_wrapper_mixed_bdrv_rdlock
 nbd_do_establish_connection(BlockDriverState *bs, bool blocking, Error **errp);
 
 #endif /* BLOCK_COROUTINES_H */
diff --git a/block/nbd.c b/block/nbd.c
index d3ee256844..a3f8f8a9d5 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -322,6 +322,7 @@ int coroutine_fn nbd_co_do_establish_connection(BlockDriverState *bs,
     int ret;
     IO_CODE();
 
+    assert_bdrv_graph_readable();
     assert(!s->ioc);
 
     s->ioc = nbd_co_establish_connection(s->conn, &s->info, blocking, errp);
@@ -369,7 +370,7 @@ static bool nbd_client_connecting(BDRVNBDState *s)
 }
 
 /* Called with s->requests_lock taken.  */
-static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
+static void coroutine_fn GRAPH_RDLOCK nbd_reconnect_attempt(BDRVNBDState *s)
 {
     int ret;
     bool blocking = s->state == NBD_CLIENT_CONNECTING_WAIT;
@@ -480,9 +481,9 @@ static coroutine_fn int nbd_receive_replies(BDRVNBDState *s, uint64_t handle)
     }
 }
 
-static int coroutine_fn nbd_co_send_request(BlockDriverState *bs,
-                                            NBDRequest *request,
-                                            QEMUIOVector *qiov)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_co_send_request(BlockDriverState *bs, NBDRequest *request,
+                    QEMUIOVector *qiov)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
     int rc, i = -1;
@@ -1171,8 +1172,9 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
     return iter.ret;
 }
 
-static int coroutine_fn nbd_co_request(BlockDriverState *bs, NBDRequest *request,
-                                       QEMUIOVector *write_qiov)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_co_request(BlockDriverState *bs, NBDRequest *request,
+               QEMUIOVector *write_qiov)
 {
     int ret, request_ret;
     Error *local_err = NULL;
@@ -1208,9 +1210,9 @@ static int coroutine_fn nbd_co_request(BlockDriverState *bs, NBDRequest *request
     return ret ? ret : request_ret;
 }
 
-static int coroutine_fn nbd_client_co_preadv(BlockDriverState *bs, int64_t offset,
-                                             int64_t bytes, QEMUIOVector *qiov,
-                                             BdrvRequestFlags flags)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_client_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                     QEMUIOVector *qiov, BdrvRequestFlags flags)
 {
     int ret, request_ret;
     Error *local_err = NULL;
@@ -1266,9 +1268,9 @@ static int coroutine_fn nbd_client_co_preadv(BlockDriverState *bs, int64_t offse
     return ret ? ret : request_ret;
 }
 
-static int coroutine_fn nbd_client_co_pwritev(BlockDriverState *bs, int64_t offset,
-                                              int64_t bytes, QEMUIOVector *qiov,
-                                              BdrvRequestFlags flags)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_client_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                      QEMUIOVector *qiov, BdrvRequestFlags flags)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
     NBDRequest request = {
@@ -1291,8 +1293,9 @@ static int coroutine_fn nbd_client_co_pwritev(BlockDriverState *bs, int64_t offs
     return nbd_co_request(bs, &request, qiov);
 }
 
-static int coroutine_fn nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset,
-                                                    int64_t bytes, BdrvRequestFlags flags)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                            BdrvRequestFlags flags)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
     NBDRequest request = {
@@ -1326,7 +1329,7 @@ static int coroutine_fn nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_
     return nbd_co_request(bs, &request, NULL);
 }
 
-static int coroutine_fn nbd_client_co_flush(BlockDriverState *bs)
+static int coroutine_fn GRAPH_RDLOCK nbd_client_co_flush(BlockDriverState *bs)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
     NBDRequest request = { .type = NBD_CMD_FLUSH };
@@ -1341,8 +1344,8 @@ static int coroutine_fn nbd_client_co_flush(BlockDriverState *bs)
     return nbd_co_request(bs, &request, NULL);
 }
 
-static int coroutine_fn nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset,
-                                               int64_t bytes)
+static int coroutine_fn GRAPH_RDLOCK
+nbd_client_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
 {
     BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
     NBDRequest request = {
@@ -1361,7 +1364,7 @@ static int coroutine_fn nbd_client_co_pdiscard(BlockDriverState *bs, int64_t off
     return nbd_co_request(bs, &request, NULL);
 }
 
-static int coroutine_fn nbd_client_co_block_status(
+static int coroutine_fn GRAPH_RDLOCK nbd_client_co_block_status(
         BlockDriverState *bs, bool want_zero, int64_t offset, int64_t bytes,
         int64_t *pnum, int64_t *map, BlockDriverState **file)
 {
-- 
2.40.0

Re: [PATCH 10/20] nbd: Mark nbd_co_do_establish_connection() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:48PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> nbd_co_do_establish_connection() need to hold a reader lock for the
> graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/coroutines.h |  5 +++--
>  block/nbd.c        | 39 +++++++++++++++++++++------------------
>  2 files changed, 24 insertions(+), 20 deletions(-)
>

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 10/20] nbd: Mark nbd_co_do_establish_connection() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Tue, Apr 25, 2023 at 07:31:48PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> nbd_co_do_establish_connection() need to hold a reader lock for the
> graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/coroutines.h |  5 +++--
>  block/nbd.c        | 39 +++++++++++++++++++++------------------
>  2 files changed, 24 insertions(+), 20 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 11/20] vhdx: Take graph lock for accessing a node's parent list

[Kevin Wolf]

This adds GRAPH_RDLOCK annotations to declare that functions accessing
the parent list of a node need to hold a reader lock for the graph. As
it happens, they already do.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vhdx.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/vhdx.c b/block/vhdx.c
index 00777da91a..b20b1edf11 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1506,8 +1506,9 @@ exit:
  * There are 2 headers, and the highest sequence number will represent
  * the active header
  */
-static int vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size,
-                                   uint32_t log_size)
+static int coroutine_fn GRAPH_RDLOCK
+vhdx_create_new_headers(BlockBackend *blk, uint64_t image_size,
+                        uint32_t log_size)
 {
     BlockDriverState *bs = blk_bs(blk);
     BdrvChild *child;
@@ -1897,8 +1898,8 @@ exit:
  *    .---- ~ ----------- ~ ------------ ~ ---------------- ~ -----------.
  *   1MB
  */
-static int coroutine_fn vhdx_co_create(BlockdevCreateOptions *opts,
-                                       Error **errp)
+static int coroutine_fn GRAPH_RDLOCK
+vhdx_co_create(BlockdevCreateOptions *opts, Error **errp)
 {
     BlockdevCreateOptionsVhdx *vhdx_opts;
     BlockBackend *blk = NULL;
-- 
2.40.0

Re: [PATCH 11/20] vhdx: Take graph lock for accessing a node's parent list

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:49PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that functions accessing
> the parent list of a node need to hold a reader lock for the graph. As
> it happens, they already do.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vhdx.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 11/20] vhdx: Take graph lock for accessing a node's parent list

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

On Tue, Apr 25, 2023 at 07:31:49PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that functions accessing
> the parent list of a node need to hold a reader lock for the graph. As
> it happens, they already do.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vhdx.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

The commit message is misleading. This commit does not take the graph
lock, it declares that the caller must already hold the graph lock.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 12/20] mirror: Take graph lock for accessing a node's parent list

[Kevin Wolf]

This adds GRAPH_RDLOCK annotations to declare that functions accessing
the parent list of a node need to hold a reader lock for the graph. As
it happens, they already do.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index af9bbd23d4..067a0630ba 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1416,7 +1416,7 @@ static MirrorOp *coroutine_fn active_write_prepare(MirrorBlockJob *s,
     return op;
 }
 
-static void coroutine_fn active_write_settle(MirrorOp *op)
+static void coroutine_fn GRAPH_RDLOCK active_write_settle(MirrorOp *op)
 {
     uint64_t start_chunk = op->offset / op->s->granularity;
     uint64_t end_chunk = DIV_ROUND_UP(op->offset + op->bytes,
-- 
2.40.0

Re: [PATCH 12/20] mirror: Take graph lock for accessing a node's parent list

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:50PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that functions accessing
> the parent list of a node need to hold a reader lock for the graph. As
> it happens, they already do.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 12/20] mirror: Take graph lock for accessing a node's parent list

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Tue, Apr 25, 2023 at 07:31:50PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that functions accessing
> the parent list of a node need to hold a reader lock for the graph. As
> it happens, they already do.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The commit message is misleading. This commit does not take the graph
lock, it declares that the caller must already hold the graph lock.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 13/20] block: Mark bdrv_co_get_allocated_file_size() and callers GRAPH_RDLOCK

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_co_get_allocated_file_size() need to hold a reader lock for the
graph.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-io.h         | 7 +++++--
 include/block/block_int-common.h | 2 +-
 block.c                          | 4 +++-
 block/vmdk.c                     | 2 +-
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/include/block/block-io.h b/include/block/block-io.h
index 5dab88521d..fb2adb31c7 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -84,8 +84,11 @@ int64_t coroutine_mixed_fn bdrv_nb_sectors(BlockDriverState *bs);
 int64_t coroutine_fn GRAPH_RDLOCK bdrv_co_getlength(BlockDriverState *bs);
 int64_t co_wrapper_mixed_bdrv_rdlock bdrv_getlength(BlockDriverState *bs);
 
-int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs);
-int64_t co_wrapper bdrv_get_allocated_file_size(BlockDriverState *bs);
+int64_t coroutine_fn GRAPH_RDLOCK
+bdrv_co_get_allocated_file_size(BlockDriverState *bs);
+
+int64_t co_wrapper_bdrv_rdlock
+bdrv_get_allocated_file_size(BlockDriverState *bs);
 
 BlockMeasureInfo *bdrv_measure(BlockDriver *drv, QemuOpts *opts,
                                BlockDriverState *in_bs, Error **errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 6fb28cd8fa..6e0365d8f2 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -685,7 +685,7 @@ struct BlockDriver {
     int64_t coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_getlength)(
         BlockDriverState *bs);
 
-    int64_t coroutine_fn (*bdrv_co_get_allocated_file_size)(
+    int64_t coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_get_allocated_file_size)(
         BlockDriverState *bs);
 
     BlockMeasureInfo *(*bdrv_measure)(QemuOpts *opts, BlockDriverState *in_bs,
diff --git a/block.c b/block.c
index abec940867..3ccb935950 100644
--- a/block.c
+++ b/block.c
@@ -5750,7 +5750,8 @@ exit:
  * sums the size of all data-bearing children.  (This excludes backing
  * children.)
  */
-static int64_t coroutine_fn bdrv_sum_allocated_file_size(BlockDriverState *bs)
+static int64_t coroutine_fn GRAPH_RDLOCK
+bdrv_sum_allocated_file_size(BlockDriverState *bs)
 {
     BdrvChild *child;
     int64_t child_size, sum = 0;
@@ -5778,6 +5779,7 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
     IO_CODE();
+    assert_bdrv_graph_readable();
 
     if (!drv) {
         return -ENOMEDIUM;
diff --git a/block/vmdk.c b/block/vmdk.c
index 11b553ef25..fddbd1c86c 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2845,7 +2845,7 @@ static void vmdk_close(BlockDriverState *bs)
     error_free(s->migration_blocker);
 }
 
-static int64_t coroutine_fn
+static int64_t coroutine_fn GRAPH_RDLOCK
 vmdk_co_get_allocated_file_size(BlockDriverState *bs)
 {
     int i;
-- 
2.40.0

Re: [PATCH 13/20] block: Mark bdrv_co_get_allocated_file_size() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_co_get_allocated_file_size() need to hold a reader lock for the
> graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-io.h         | 7 +++++--
>  include/block/block_int-common.h | 2 +-
>  block.c                          | 4 +++-
>  block/vmdk.c                     | 2 +-
>  4 files changed, 10 insertions(+), 5 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 13/20] block: Mark bdrv_co_get_allocated_file_size() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote:
> @@ -5778,6 +5779,7 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
>      IO_CODE();
> +    assert_bdrv_graph_readable();

Is there a need for runtime assertions in functions already checked by
TSA?

I guess not. Otherwise runtime assertions should have been added in many
of the other functions marked GRAPH_RDLOCK in this series.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 13/20] block: Mark bdrv_co_get_allocated_file_size() and callers GRAPH_RDLOCK

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

Am 01.05.2023 um 21:03 hat Stefan Hajnoczi geschrieben:
> On Tue, Apr 25, 2023 at 07:31:51PM +0200, Kevin Wolf wrote:
> > @@ -5778,6 +5779,7 @@ int64_t coroutine_fn bdrv_co_get_allocated_file_size(BlockDriverState *bs)
> >  {
> >      BlockDriver *drv = bs->drv;
> >      IO_CODE();
> > +    assert_bdrv_graph_readable();
> 
> Is there a need for runtime assertions in functions already checked by
> TSA?
> 
> I guess not. Otherwise runtime assertions should have been added in many
> of the other functions marked GRAPH_RDLOCK in this series.

I guess we're a bit inconsistent with this. Emanuele started adding the
assertions everywhere before I added the TSA annotations. Since then,
I've tended to leave the assertions from Emanuele's patches (such as
this one) around, but didn't add assertions in new patches.

The point in favour for still having assertions is that people using gcc
won't see TSA failures, but runtime assertions will still work for them.
I don't think we should have them in every GRAPH_RDLOCK function, but
having them in one central place in each call chain (i.e. the block.c
wrappers for BlockDriver callbacks) does make sense to me. So if we
stick to this standard, we'd keep this assertion.

But if you prefer, I can drop it. I assume that enough developers run
with clang that the additional assertion doesn't buy us that much. And
I compile with clang anyway when applying patches.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 14/20] block: Mark bdrv_co_get_info() and callers GRAPH_RDLOCK

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_co_get_info() need to hold a reader lock for the graph.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-io.h         |  7 +++++--
 include/block/block_int-common.h |  4 ++--
 block.c                          |  2 ++
 block/crypto.c                   |  2 +-
 block/io.c                       | 11 +++++------
 block/mirror.c                   |  8 ++++++--
 block/raw-format.c               |  2 +-
 7 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/include/block/block-io.h b/include/block/block-io.h
index fb2adb31c7..bba7f957e1 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -167,8 +167,11 @@ const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 
-int coroutine_fn bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
-int co_wrapper_mixed bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
+int coroutine_fn GRAPH_RDLOCK
+bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
+
+int co_wrapper_mixed_bdrv_rdlock
+bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
                                           Error **errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 6e0365d8f2..ee77903f72 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -699,8 +699,8 @@ struct BlockDriver {
         BlockDriverState *bs, int64_t offset, int64_t bytes,
         QEMUIOVector *qiov, size_t qiov_offset);
 
-    int coroutine_fn (*bdrv_co_get_info)(BlockDriverState *bs,
-                                         BlockDriverInfo *bdi);
+    int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_get_info)(
+        BlockDriverState *bs, BlockDriverInfo *bdi);
 
     ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs,
                                                  Error **errp);
diff --git a/block.c b/block.c
index 3ccb935950..a6deaf8ad1 100644
--- a/block.c
+++ b/block.c
@@ -6349,6 +6349,8 @@ int coroutine_fn bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     int ret;
     BlockDriver *drv = bs->drv;
     IO_CODE();
+    assert_bdrv_graph_readable();
+
     /* if bs->drv == NULL, bs is closed, so there's nothing to do here */
     if (!drv) {
         return -ENOMEDIUM;
diff --git a/block/crypto.c b/block/crypto.c
index 8fd3ad0054..30093cff9b 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -736,7 +736,7 @@ fail:
     return ret;
 }
 
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
 block_crypto_co_get_info_luks(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BlockDriverInfo subbdi;
diff --git a/block/io.c b/block/io.c
index 6fa1993374..3bf9ef9d87 100644
--- a/block/io.c
+++ b/block/io.c
@@ -727,10 +727,9 @@ BdrvTrackedRequest *coroutine_fn bdrv_co_get_self_request(BlockDriverState *bs)
 /**
  * Round a region to cluster boundaries
  */
-void coroutine_fn bdrv_round_to_clusters(BlockDriverState *bs,
-                            int64_t offset, int64_t bytes,
-                            int64_t *cluster_offset,
-                            int64_t *cluster_bytes)
+void coroutine_fn GRAPH_RDLOCK
+bdrv_round_to_clusters(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                       int64_t *cluster_offset, int64_t *cluster_bytes)
 {
     BlockDriverInfo bdi;
     IO_CODE();
@@ -744,7 +743,7 @@ void coroutine_fn bdrv_round_to_clusters(BlockDriverState *bs,
     }
 }
 
-static coroutine_fn int bdrv_get_cluster_size(BlockDriverState *bs)
+static int coroutine_fn GRAPH_RDLOCK bdrv_get_cluster_size(BlockDriverState *bs)
 {
     BlockDriverInfo bdi;
     int ret;
@@ -1800,7 +1799,7 @@ fail:
     return ret;
 }
 
-static inline int coroutine_fn
+static inline int coroutine_fn GRAPH_RDLOCK
 bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,
                           BdrvTrackedRequest *req, int flags)
 {
diff --git a/block/mirror.c b/block/mirror.c
index 067a0630ba..a2f46783cf 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -576,8 +576,10 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         } else if (ret >= 0 && !(ret & BDRV_BLOCK_DATA)) {
             int64_t target_offset;
             int64_t target_bytes;
-            bdrv_round_to_clusters(blk_bs(s->target), offset, io_bytes,
-                                   &target_offset, &target_bytes);
+            WITH_GRAPH_RDLOCK_GUARD() {
+                bdrv_round_to_clusters(blk_bs(s->target), offset, io_bytes,
+                                       &target_offset, &target_bytes);
+            }
             if (target_offset == offset &&
                 target_bytes == io_bytes) {
                 mirror_method = ret & BDRV_BLOCK_ZERO ?
@@ -966,11 +968,13 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
      */
     bdrv_get_backing_filename(target_bs, backing_filename,
                               sizeof(backing_filename));
+    bdrv_graph_co_rdlock();
     if (!bdrv_co_get_info(target_bs, &bdi) && bdi.cluster_size) {
         s->target_cluster_size = bdi.cluster_size;
     } else {
         s->target_cluster_size = BDRV_SECTOR_SIZE;
     }
+    bdrv_graph_co_rdunlock();
     if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
         s->granularity < s->target_cluster_size) {
         s->buf_size = MAX(s->buf_size, s->target_cluster_size);
diff --git a/block/raw-format.c b/block/raw-format.c
index 06b8030d9d..fd9e61f58e 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -369,7 +369,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, BlockDriverState *in_bs,
     return info;
 }
 
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
 raw_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     return bdrv_co_get_info(bs->file->bs, bdi);
-- 
2.40.0

Re: [PATCH 14/20] block: Mark bdrv_co_get_info() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:52PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_co_get_info() need to hold a reader lock for the graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-io.h         |  7 +++++--
>  include/block/block_int-common.h |  4 ++--
>  block.c                          |  2 ++
>  block/crypto.c                   |  2 +-
>  block/io.c                       | 11 +++++------
>  block/mirror.c                   |  8 ++++++--
>  block/raw-format.c               |  2 +-
>  7 files changed, 22 insertions(+), 14 deletions(-)
>

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 14/20] block: Mark bdrv_co_get_info() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Tue, Apr 25, 2023 at 07:31:52PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_co_get_info() need to hold a reader lock for the graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-io.h         |  7 +++++--
>  include/block/block_int-common.h |  4 ++--
>  block.c                          |  2 ++
>  block/crypto.c                   |  2 +-
>  block/io.c                       | 11 +++++------
>  block/mirror.c                   |  8 ++++++--
>  block/raw-format.c               |  2 +-
>  7 files changed, 22 insertions(+), 14 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 15/20] block: Mark bdrv_co_debug_event() GRAPH_RDLOCK

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_co_debug_event() need to hold a reader lock for the graph.

Unfortunately we cannot use a co_wrapper_bdrv_rdlock, because the
function is called by mixed functions that run both in coroutine and
non-coroutine context (for example blkdebug_open).

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-io.h         | 9 +++++----
 include/block/block_int-common.h | 4 ++--
 block.c                          | 2 ++
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/block/block-io.h b/include/block/block-io.h
index bba7f957e1..1f612ec5bd 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -205,10 +205,11 @@ void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
 void bdrv_enable_copy_on_read(BlockDriverState *bs);
 void bdrv_disable_copy_on_read(BlockDriverState *bs);
 
-void coroutine_fn bdrv_co_debug_event(BlockDriverState *bs,
-                                      BlkdebugEvent event);
-void co_wrapper_mixed bdrv_debug_event(BlockDriverState *bs,
-                                       BlkdebugEvent event);
+void coroutine_fn GRAPH_RDLOCK
+bdrv_co_debug_event(BlockDriverState *bs, BlkdebugEvent event);
+
+void co_wrapper_mixed_bdrv_rdlock
+bdrv_debug_event(BlockDriverState *bs, BlkdebugEvent event);
 
 #define BLKDBG_EVENT(child, evt) \
     do { \
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index ee77903f72..88ce7f9d9e 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -735,8 +735,8 @@ struct BlockDriver {
     int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_check)(
         BlockDriverState *bs, BdrvCheckResult *result, BdrvCheckMode fix);
 
-    void coroutine_fn (*bdrv_co_debug_event)(BlockDriverState *bs,
-                                             BlkdebugEvent event);
+    void coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_debug_event)(
+        BlockDriverState *bs, BlkdebugEvent event);
 
     /* io queue for linux-aio */
     void coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_io_plug)(BlockDriverState *bs);
diff --git a/block.c b/block.c
index a6deaf8ad1..1bc766c778 100644
--- a/block.c
+++ b/block.c
@@ -6399,6 +6399,8 @@ BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs)
 void coroutine_fn bdrv_co_debug_event(BlockDriverState *bs, BlkdebugEvent event)
 {
     IO_CODE();
+    assert_bdrv_graph_readable();
+
     if (!bs || !bs->drv || !bs->drv->bdrv_co_debug_event) {
         return;
     }
-- 
2.40.0

Re: [PATCH 15/20] block: Mark bdrv_co_debug_event() GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:53PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_co_debug_event() need to hold a reader lock for the graph.
> 
> Unfortunately we cannot use a co_wrapper_bdrv_rdlock, because the
> function is called by mixed functions that run both in coroutine and
> non-coroutine context (for example blkdebug_open).

Is this statement still true after 8/20?

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-io.h         | 9 +++++----
>  include/block/block_int-common.h | 4 ++--
>  block.c                          | 2 ++
>  3 files changed, 9 insertions(+), 6 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 15/20] block: Mark bdrv_co_debug_event() GRAPH_RDLOCK

[Kevin Wolf]

Am 25.04.2023 um 23:14 hat Eric Blake geschrieben:
> On Tue, Apr 25, 2023 at 07:31:53PM +0200, Kevin Wolf wrote:
> > From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > 
> > This adds GRAPH_RDLOCK annotations to declare that callers of
> > bdrv_co_debug_event() need to hold a reader lock for the graph.
> > 
> > Unfortunately we cannot use a co_wrapper_bdrv_rdlock, because the
> > function is called by mixed functions that run both in coroutine and
> > non-coroutine context (for example blkdebug_open).
> 
> Is this statement still true after 8/20?

You're right, 8/20 is newer than this commit message and actually made
the example wrong. The problem still exists, though. I'll point to the
qcow2 cluster/refcount functions instead, which are still mixed.

Kevin

Re: [PATCH 15/20] block: Mark bdrv_co_debug_event() GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Tue, Apr 25, 2023 at 07:31:53PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_co_debug_event() need to hold a reader lock for the graph.
> 
> Unfortunately we cannot use a co_wrapper_bdrv_rdlock, because the
> function is called by mixed functions that run both in coroutine and
> non-coroutine context (for example blkdebug_open).
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-io.h         | 9 +++++----
>  include/block/block_int-common.h | 4 ++--
>  block.c                          | 2 ++
>  3 files changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 16/20] block: Mark BlockDriver callbacks for amend job GRAPH_RDLOCK

[Kevin Wolf]

From: Emanuele Giuseppe Esposito <eesposit@redhat.com>

This adds GRAPH_RDLOCK annotations to declare that callers of amend
callbacks in BlockDriver need to hold a reader lock for the graph.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block_int-common.h | 12 ++++++------
 block/amend.c                    |  8 +++++++-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 88ce7f9d9e..37d094796e 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -204,12 +204,13 @@ struct BlockDriver {
      * to allow driver-specific initialization code that requires
      * the BQL, like setting up specific permission flags.
      */
-    int (*bdrv_amend_pre_run)(BlockDriverState *bs, Error **errp);
+    int GRAPH_RDLOCK_PTR (*bdrv_amend_pre_run)(
+        BlockDriverState *bs, Error **errp);
     /*
      * This function is invoked under BQL after .bdrv_co_amend()
      * to allow cleaning up what was done in .bdrv_amend_pre_run().
      */
-    void (*bdrv_amend_clean)(BlockDriverState *bs);
+    void GRAPH_RDLOCK_PTR (*bdrv_amend_clean)(BlockDriverState *bs);
 
     /*
      * Return true if @to_replace can be replaced by a BDS with the
@@ -463,10 +464,9 @@ struct BlockDriver {
 
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
 
-    int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs,
-                                      BlockdevAmendOptions *opts,
-                                      bool force,
-                                      Error **errp);
+    int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_amend)(
+        BlockDriverState *bs, BlockdevAmendOptions *opts, bool force,
+        Error **errp);
 
     /* aio */
     BlockAIOCB * GRAPH_RDLOCK_PTR (*bdrv_aio_preadv)(BlockDriverState *bs,
diff --git a/block/amend.c b/block/amend.c
index bc4bb7b416..53a410247c 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -46,6 +46,7 @@ static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
 {
     BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
     int ret;
+    GRAPH_RDLOCK_GUARD();
 
     job_progress_set_remaining(&s->common, 1);
     ret = s->bs->drv->bdrv_co_amend(s->bs, s->opts, s->force, errp);
@@ -54,7 +55,8 @@ static int coroutine_fn blockdev_amend_run(Job *job, Error **errp)
     return ret;
 }
 
-static int blockdev_amend_pre_run(BlockdevAmendJob *s, Error **errp)
+static int GRAPH_RDLOCK
+blockdev_amend_pre_run(BlockdevAmendJob *s, Error **errp)
 {
     if (s->bs->drv->bdrv_amend_pre_run) {
         return s->bs->drv->bdrv_amend_pre_run(s->bs, errp);
@@ -67,9 +69,11 @@ static void blockdev_amend_free(Job *job)
 {
     BlockdevAmendJob *s = container_of(job, BlockdevAmendJob, common);
 
+    bdrv_graph_rdlock_main_loop();
     if (s->bs->drv->bdrv_amend_clean) {
         s->bs->drv->bdrv_amend_clean(s->bs);
     }
+    bdrv_graph_rdunlock_main_loop();
 
     bdrv_unref(s->bs);
 }
@@ -93,6 +97,8 @@ void qmp_x_blockdev_amend(const char *job_id,
     BlockDriver *drv = bdrv_find_format(fmt);
     BlockDriverState *bs;
 
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     bs = bdrv_lookup_bs(NULL, node_name, errp);
     if (!bs) {
         return;
-- 
2.40.0

Re: [PATCH 16/20] block: Mark BlockDriver callbacks for amend job GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:54PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of amend
> callbacks in BlockDriver need to hold a reader lock for the graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int-common.h | 12 ++++++------
>  block/amend.c                    |  8 +++++++-
>  2 files changed, 13 insertions(+), 7 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 16/20] block: Mark BlockDriver callbacks for amend job GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

On Tue, Apr 25, 2023 at 07:31:54PM +0200, Kevin Wolf wrote:
> From: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> 
> This adds GRAPH_RDLOCK annotations to declare that callers of amend
> callbacks in BlockDriver need to hold a reader lock for the graph.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int-common.h | 12 ++++++------
>  block/amend.c                    |  8 +++++++-
>  2 files changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 17/20] block: Mark bdrv_query_bds_stats() and callers GRAPH_RDLOCK

[Kevin Wolf]

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_query_bds_stats() need to hold a reader lock for the graph because
it accesses the children list of a node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qapi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index c84147849d..71f2751257 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -604,8 +604,8 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
         = bdrv_latency_histogram_stats(&hgram[BLOCK_ACCT_FLUSH]);
 }
 
-static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
-                                        bool blk_level)
+static BlockStats * GRAPH_RDLOCK
+bdrv_query_bds_stats(BlockDriverState *bs, bool blk_level)
 {
     BdrvChild *parent_child;
     BlockDriverState *filter_or_cow_bs;
@@ -713,6 +713,8 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
     BlockBackend *blk;
     BlockDriverState *bs;
 
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     /* Just to be safe if query_nodes is not always initialized */
     if (has_query_nodes && query_nodes) {
         for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) {
-- 
2.40.0

Re: [PATCH 17/20] block: Mark bdrv_query_bds_stats() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:55PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_query_bds_stats() need to hold a reader lock for the graph because
> it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 17/20] block: Mark bdrv_query_bds_stats() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Tue, Apr 25, 2023 at 07:31:55PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_query_bds_stats() need to hold a reader lock for the graph because
> it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qapi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 18/20] block: Mark bdrv_query_block_graph_info() and callers GRAPH_RDLOCK

[Kevin Wolf]

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_query_block_graph_info() need to hold a reader lock for the graph
because it accesses the children list of a node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/qapi.h | 7 ++++---
 qemu-img.c           | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/block/qapi.h b/include/block/qapi.h
index 8773b9b191..18d48ddb70 100644
--- a/include/block/qapi.h
+++ b/include/block/qapi.h
@@ -25,6 +25,7 @@
 #ifndef BLOCK_QAPI_H
 #define BLOCK_QAPI_H
 
+#include "block/graph-lock.h"
 #include "block/snapshot.h"
 #include "qapi/qapi-types-block-core.h"
 
@@ -43,9 +44,9 @@ void bdrv_query_image_info(BlockDriverState *bs,
                            bool flat,
                            bool skip_implicit_filters,
                            Error **errp);
-void bdrv_query_block_graph_info(BlockDriverState *bs,
-                                 BlockGraphInfo **p_info,
-                                 Error **errp);
+void GRAPH_RDLOCK
+bdrv_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info,
+                            Error **errp);
 
 void bdrv_snapshot_dump(QEMUSnapshotInfo *sn);
 void bdrv_image_info_specific_dump(ImageInfoSpecific *info_spec,
diff --git a/qemu-img.c b/qemu-img.c
index 9aeac69fa6..9f9f0a7629 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2938,6 +2938,8 @@ static BlockGraphInfoList *collect_image_info_list(bool image_opts,
         }
         bs = blk_bs(blk);
 
+        GRAPH_RDLOCK_GUARD_MAINLOOP();
+
         /*
          * Note that the returned BlockGraphInfo object will not have
          * information about this image's backing node, because we have opened
-- 
2.40.0

Re: [PATCH 18/20] block: Mark bdrv_query_block_graph_info() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:56PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_query_block_graph_info() need to hold a reader lock for the graph
> because it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/qapi.h | 7 ++++---
>  qemu-img.c           | 2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 18/20] block: Mark bdrv_query_block_graph_info() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

On Tue, Apr 25, 2023 at 07:31:56PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_query_block_graph_info() need to hold a reader lock for the graph
> because it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/qapi.h | 7 ++++---
>  qemu-img.c           | 2 ++
>  2 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 19/20] block: Mark bdrv_recurse_can_replace() and callers GRAPH_RDLOCK

[Kevin Wolf]

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_recurse_can_replace() need to hold a reader lock for the graph
because it accesses the children list of a node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-global-state.h     | 5 +++--
 include/block/block_int-common.h       | 4 ++--
 include/block/block_int-global-state.h | 4 ++--
 block/blkverify.c                      | 5 +++--
 block/mirror.c                         | 4 ++++
 block/quorum.c                         | 4 ++--
 blockdev.c                             | 3 +++
 7 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index ec3ddb17a8..f234bca0b6 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -163,8 +163,9 @@ int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
                        Error **errp);
 
 /* check if a named node can be replaced when doing drive-mirror */
-BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
-                                        const char *node_name, Error **errp);
+BlockDriverState * GRAPH_RDLOCK
+check_to_replace_node(BlockDriverState *parent_bs, const char *node_name,
+                      Error **errp);
 
 int no_coroutine_fn bdrv_activate(BlockDriverState *bs, Error **errp);
 
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 37d094796e..024ded4fc2 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -217,8 +217,8 @@ struct BlockDriver {
      * same data as @bs without it affecting @bs's behavior (that is,
      * without it being visible to @bs's parents).
      */
-    bool (*bdrv_recurse_can_replace)(BlockDriverState *bs,
-                                     BlockDriverState *to_replace);
+    bool GRAPH_RDLOCK_PTR (*bdrv_recurse_can_replace)(
+        BlockDriverState *bs, BlockDriverState *to_replace);
 
     int (*bdrv_probe_device)(const char *filename);
 
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index 902406eb99..da5fb31089 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -225,8 +225,8 @@ int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
  */
 int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp);
 
-bool bdrv_recurse_can_replace(BlockDriverState *bs,
-                              BlockDriverState *to_replace);
+bool GRAPH_RDLOCK bdrv_recurse_can_replace(BlockDriverState *bs,
+                                           BlockDriverState *to_replace);
 
 /*
  * Default implementation for BlockDriver.bdrv_child_perm() that can
diff --git a/block/blkverify.c b/block/blkverify.c
index 1c16f86b2e..7326461f30 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -265,8 +265,9 @@ static int coroutine_fn GRAPH_RDLOCK blkverify_co_flush(BlockDriverState *bs)
     return bdrv_co_flush(s->test_file->bs);
 }
 
-static bool blkverify_recurse_can_replace(BlockDriverState *bs,
-                                          BlockDriverState *to_replace)
+static bool GRAPH_RDLOCK
+blkverify_recurse_can_replace(BlockDriverState *bs,
+                              BlockDriverState *to_replace)
 {
     BDRVBlkverifyState *s = bs->opaque;
 
diff --git a/block/mirror.c b/block/mirror.c
index a2f46783cf..99409f893f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -747,7 +747,10 @@ static int mirror_exit_common(Job *job)
          * Cannot use check_to_replace_node() here, because that would
          * check for an op blocker on @to_replace, and we have our own
          * there.
+         *
+         * TODO Pull out the writer lock from bdrv_replace_node() to here
          */
+        bdrv_graph_rdlock_main_loop();
         if (bdrv_recurse_can_replace(src, to_replace)) {
             bdrv_replace_node(to_replace, target_bs, &local_err);
         } else {
@@ -756,6 +759,7 @@ static int mirror_exit_common(Job *job)
                        "would not lead to an abrupt change of visible data",
                        to_replace->node_name, target_bs->node_name);
         }
+        bdrv_graph_rdunlock_main_loop();
         bdrv_drained_end(target_bs);
         if (local_err) {
             error_report_err(local_err);
diff --git a/block/quorum.c b/block/quorum.c
index ff5a0a2da3..f28758cf2b 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -825,8 +825,8 @@ static coroutine_fn GRAPH_RDLOCK int quorum_co_flush(BlockDriverState *bs)
     return result;
 }
 
-static bool quorum_recurse_can_replace(BlockDriverState *bs,
-                                       BlockDriverState *to_replace)
+static bool GRAPH_RDLOCK
+quorum_recurse_can_replace(BlockDriverState *bs, BlockDriverState *to_replace)
 {
     BDRVQuorumState *s = bs->opaque;
     int i;
diff --git a/blockdev.c b/blockdev.c
index eb509cf964..bfca91ee4e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2951,6 +2951,9 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
     BlockDriverState *unfiltered_bs;
     int job_flags = JOB_DEFAULT;
 
+    GLOBAL_STATE_CODE();
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     if (!has_speed) {
         speed = 0;
     }
-- 
2.40.0

Re: [PATCH 19/20] block: Mark bdrv_recurse_can_replace() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:57PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_recurse_can_replace() need to hold a reader lock for the graph
> because it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h     | 5 +++--
>  include/block/block_int-common.h       | 4 ++--
>  include/block/block_int-global-state.h | 4 ++--
>  block/blkverify.c                      | 5 +++--
>  block/mirror.c                         | 4 ++++
>  block/quorum.c                         | 4 ++--
>  blockdev.c                             | 3 +++
>  7 files changed, 19 insertions(+), 10 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 19/20] block: Mark bdrv_recurse_can_replace() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 782 bytes --]

On Tue, Apr 25, 2023 at 07:31:57PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_recurse_can_replace() need to hold a reader lock for the graph
> because it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h     | 5 +++--
>  include/block/block_int-common.h       | 4 ++--
>  include/block/block_int-global-state.h | 4 ++--
>  block/blkverify.c                      | 5 +++--
>  block/mirror.c                         | 4 ++++
>  block/quorum.c                         | 4 ++--
>  blockdev.c                             | 3 +++
>  7 files changed, 19 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 20/20] block: Mark bdrv_refresh_limits() and callers GRAPH_RDLOCK

[Kevin Wolf]

This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_refresh_limits() need to hold a reader lock for the graph because
it accesses the children list of a node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-global-state.h | 5 ++++-
 include/block/block_int-common.h   | 3 ++-
 block.c                            | 9 +++++++++
 block/io.c                         | 1 -
 4 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index f234bca0b6..2d93423d35 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -133,7 +133,10 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
                                           const char *backing_file);
 void bdrv_refresh_filename(BlockDriverState *bs);
-void bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp);
+
+void GRAPH_RDLOCK
+bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp);
+
 int bdrv_commit(BlockDriverState *bs);
 int bdrv_make_empty(BdrvChild *c, Error **errp);
 int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 024ded4fc2..4909876756 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -334,7 +334,8 @@ struct BlockDriver {
     int (*bdrv_debug_resume)(BlockDriverState *bs, const char *tag);
     bool (*bdrv_debug_is_suspended)(BlockDriverState *bs, const char *tag);
 
-    void (*bdrv_refresh_limits)(BlockDriverState *bs, Error **errp);
+    void GRAPH_RDLOCK_PTR (*bdrv_refresh_limits)(
+        BlockDriverState *bs, Error **errp);
 
     /*
      * Returns 1 if newly created images are guaranteed to contain only
diff --git a/block.c b/block.c
index 1bc766c778..dad9a4fa43 100644
--- a/block.c
+++ b/block.c
@@ -1667,7 +1667,10 @@ bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name,
         return ret;
     }
 
+    bdrv_graph_rdlock_main_loop();
     bdrv_refresh_limits(bs, NULL, &local_err);
+    bdrv_graph_rdunlock_main_loop();
+
     if (local_err) {
         error_propagate(errp, local_err);
         return -EINVAL;
@@ -3419,7 +3422,9 @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
     }
 
 out:
+    bdrv_graph_rdlock_main_loop();
     bdrv_refresh_limits(parent_bs, tran, NULL);
+    bdrv_graph_rdunlock_main_loop();
 
     return 0;
 }
@@ -4917,7 +4922,9 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state)
     qdict_del(bs->explicit_options, "backing");
     qdict_del(bs->options, "backing");
 
+    bdrv_graph_rdlock_main_loop();
     bdrv_refresh_limits(bs, NULL, NULL);
+    bdrv_graph_rdunlock_main_loop();
     bdrv_refresh_total_sectors(bs, bs->total_sectors);
 }
 
@@ -5316,7 +5323,9 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
 out:
     tran_finalize(tran, ret);
 
+    bdrv_graph_rdlock_main_loop();
     bdrv_refresh_limits(bs_top, NULL, NULL);
+    bdrv_graph_rdunlock_main_loop();
 
     if (new_context && old_context != new_context) {
         aio_context_release(new_context);
diff --git a/block/io.c b/block/io.c
index 3bf9ef9d87..58557f2f96 100644
--- a/block/io.c
+++ b/block/io.c
@@ -160,7 +160,6 @@ void bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp)
     bool have_limits;
 
     GLOBAL_STATE_CODE();
-    assume_graph_lock(); /* FIXME */
 
     if (tran) {
         BdrvRefreshLimitsState *s = g_new(BdrvRefreshLimitsState, 1);
-- 
2.40.0

Re: [PATCH 20/20] block: Mark bdrv_refresh_limits() and callers GRAPH_RDLOCK

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:58PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_refresh_limits() need to hold a reader lock for the graph because
> it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h | 5 ++++-
>  include/block/block_int-common.h   | 3 ++-
>  block.c                            | 9 +++++++++
>  block/io.c                         | 1 -
>  4 files changed, 15 insertions(+), 3 deletions(-)

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 20/20] block: Mark bdrv_refresh_limits() and callers GRAPH_RDLOCK

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Tue, Apr 25, 2023 at 07:31:58PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_refresh_limits() need to hold a reader lock for the graph because
> it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h | 5 ++++-
>  include/block/block_int-common.h   | 3 ++-
>  block.c                            | 9 +++++++++
>  block/io.c                         | 1 -
>  4 files changed, 15 insertions(+), 3 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]