[PATCH 0/6] Protect the block layer with a rwlock: part 2

[PATCH 0/6] Protect the block layer with a rwlock: part 2

[Emanuele Giuseppe Esposito]

Please read "Protect the block layer with a rwlock: part 1" for an additional
introduction and aim of this series.

This second part aims to add the graph rdlock to the BlockDriver functions
that already run in coroutine context and are classified as IO.
Such functions will recursively traverse the BlockDriverState graph, therefore
they need to be protected with the rdlock.

Based-on: <20221116134850.3051419-1-eesposit@redhat.com>

Thank you,
Emanuele

Emanuele Giuseppe Esposito (6):
  block: assert that bdrv_co_create is always called with graph rdlock
    taken
  block: assert that BlockDriver->bdrv_co_{amend/create} are called with
    graph rdlock taken
  block: assert that BlockDriver->bdrv_co_copy_range_{from/to} is always
    called with graph rdlock taken
  block/dirty-bitmap: assert that BlockDriver->bdrv_co_*_dirty_bitmap
    are always called with graph rdlock taken
  block/io: assert that BlockDriver->bdrv_co_*_snapshot_* are always
    called with graph rdlock taken
  block: assert that BlockDriver->bdrv_co_delete_file is always called
    with graph rdlock taken

 block.c                          |  2 ++
 block/amend.c                    |  1 +
 block/block-backend.c            |  2 ++
 block/create.c                   |  1 +
 block/dirty-bitmap.c             |  2 ++
 block/io.c                       |  7 +++++++
 include/block/block_int-common.h | 14 +++++++++++++-
 qemu-img.c                       |  4 +++-
 8 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.31.1

[PATCH 1/6] block: assert that bdrv_co_create is always called with graph rdlock taken

[Emanuele Giuseppe Esposito]

This function is either called by bdrv_create(), which always takes
care of creating a new coroutine, or by bdrv_create_file(), which
is only called by BlockDriver->bdrv_co_create_opts callbacks,
invoked by bdrv_co_create().

Protecting bdrv_co_create() implies that BlockDriver->bdrv_co_create_opts
is always called with graph rdlock taken.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                          | 1 +
 include/block/block_int-common.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/block.c b/block.c
index c7611bed9e..e54ed300d7 100644
--- a/block.c
+++ b/block.c
@@ -537,6 +537,7 @@ int coroutine_fn bdrv_co_create(BlockDriver *drv, const char *filename,
     assert(qemu_in_coroutine());
     assert(*errp == NULL);
     assert(drv);
+    assert_bdrv_graph_readable();
 
     if (!drv->bdrv_co_create_opts) {
         error_setg(errp, "Driver '%s' does not support image creation",
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index db97d61836..d45961a1d1 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -253,6 +253,7 @@ struct BlockDriver {
 
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
                                        Error **errp);
+    /* Called with graph rdlock taken */
     int coroutine_fn (*bdrv_co_create_opts)(BlockDriver *drv,
                                             const char *filename,
                                             QemuOpts *opts,
-- 
2.31.1

[PATCH 2/6] block: assert that BlockDriver->bdrv_co_{amend/create} are called with graph rdlock taken

[Emanuele Giuseppe Esposito]

Both functions are only called by Job->run() callbacks, therefore
they must take the lock in the *_run() implementation.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/amend.c                    | 1 +
 block/create.c                   | 1 +
 include/block/block_int-common.h | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/block/amend.c b/block/amend.c
index f696a006e3..a155b6889b 100644
--- a/block/amend.c
+++ b/block/amend.c
@@ -45,6 +45,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);
diff --git a/block/create.c b/block/create.c
index 4df43f11f4..4048d71265 100644
--- a/block/create.c
+++ b/block/create.c
@@ -43,6 +43,7 @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
     int ret;
 
     GLOBAL_STATE_CODE();
+    GRAPH_RDLOCK_GUARD();
 
     job_progress_set_remaining(&s->common, 1);
     ret = s->drv->bdrv_co_create(s->opts, errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index d45961a1d1..1e9bb91c98 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -251,6 +251,7 @@ struct BlockDriver {
                           Error **errp);
     void (*bdrv_close)(BlockDriverState *bs);
 
+    /* Called with graph rdlock taken */
     int coroutine_fn (*bdrv_co_create)(BlockdevCreateOptions *opts,
                                        Error **errp);
     /* Called with graph rdlock taken */
@@ -471,6 +472,7 @@ struct BlockDriver {
 
     int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
 
+    /* Called with graph rdlock taken. */
     int coroutine_fn (*bdrv_co_amend)(BlockDriverState *bs,
                                       BlockdevAmendOptions *opts,
                                       bool force,
-- 
2.31.1

[PATCH 3/6] block: assert that BlockDriver->bdrv_co_copy_range_{from/to} is always called with graph rdlock taken

[Emanuele Giuseppe Esposito]

The only non-protected caller is convert_co_copy_range(), all other
callers are BlockDriver callbacks that already take the rdlock.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/block-backend.c            | 2 ++
 block/io.c                       | 5 +++++
 include/block/block_int-common.h | 4 ++++
 qemu-img.c                       | 4 +++-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 9e1c689e84..6f0dd15808 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2631,6 +2631,8 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in,
     if (r) {
         return r;
     }
+
+    GRAPH_RDLOCK_GUARD();
     return bdrv_co_copy_range(blk_in->root, off_in,
                               blk_out->root, off_out,
                               bytes, read_flags, write_flags);
diff --git a/block/io.c b/block/io.c
index 831f277e85..62c0b3a390 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3165,6 +3165,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
 {
     BdrvTrackedRequest req;
     int ret;
+    assert_bdrv_graph_readable();
 
     /* TODO We can support BDRV_REQ_NO_FALLBACK here */
     assert(!(read_flags & BDRV_REQ_NO_FALLBACK));
@@ -3246,6 +3247,7 @@ int coroutine_fn bdrv_co_copy_range_from(BdrvChild *src, int64_t src_offset,
                                          BdrvRequestFlags write_flags)
 {
     IO_CODE();
+    assert_bdrv_graph_readable();
     trace_bdrv_co_copy_range_from(src, src_offset, dst, dst_offset, bytes,
                                   read_flags, write_flags);
     return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
@@ -3263,6 +3265,7 @@ int coroutine_fn bdrv_co_copy_range_to(BdrvChild *src, int64_t src_offset,
                                        BdrvRequestFlags write_flags)
 {
     IO_CODE();
+    assert_bdrv_graph_readable();
     trace_bdrv_co_copy_range_to(src, src_offset, dst, dst_offset, bytes,
                                 read_flags, write_flags);
     return bdrv_co_copy_range_internal(src, src_offset, dst, dst_offset,
@@ -3275,6 +3278,8 @@ int coroutine_fn bdrv_co_copy_range(BdrvChild *src, int64_t src_offset,
                                     BdrvRequestFlags write_flags)
 {
     IO_CODE();
+    assert_bdrv_graph_readable();
+
     return bdrv_co_copy_range_from(src, src_offset,
                                    dst, dst_offset,
                                    bytes, read_flags, write_flags);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 1e9bb91c98..9e441cb93b 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -574,6 +574,8 @@ struct BlockDriver {
      *
      * See the comment of bdrv_co_copy_range for the parameter and return value
      * semantics.
+     *
+     * Called with graph rdlock taken.
      */
     int coroutine_fn (*bdrv_co_copy_range_from)(BlockDriverState *bs,
                                                 BdrvChild *src,
@@ -592,6 +594,8 @@ struct BlockDriver {
      *
      * See the comment of bdrv_co_copy_range for the parameter and return value
      * semantics.
+     *
+     * Called with graph rdlock taken.
      */
     int coroutine_fn (*bdrv_co_copy_range_to)(BlockDriverState *bs,
                                               BdrvChild *src,
diff --git a/qemu-img.c b/qemu-img.c
index 33703a6d92..2086cf6eed 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2027,7 +2027,9 @@ retry:
 
         if (s->ret == -EINPROGRESS) {
             if (copy_range) {
-                ret = convert_co_copy_range(s, sector_num, n);
+                WITH_GRAPH_RDLOCK_GUARD() {
+                    ret = convert_co_copy_range(s, sector_num, n);
+                }
                 if (ret) {
                     s->copy_range = false;
                     goto retry;
-- 
2.31.1

[PATCH 4/6] block/dirty-bitmap: assert that BlockDriver->bdrv_co_*_dirty_bitmap are always called with graph rdlock taken

[Emanuele Giuseppe Esposito]

The only callers are the respective bdrv_*_dirty_bitmap() functions that
take care of creating a new coroutine (that already takes the graph
rdlock).

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/dirty-bitmap.c             | 2 ++
 include/block/block_int-common.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 21cf592889..92c70a7282 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -392,6 +392,7 @@ int coroutine_fn
 bdrv_co_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name,
                                        Error **errp)
 {
+    assert_bdrv_graph_readable();
     if (bs->drv && bs->drv->bdrv_co_remove_persistent_dirty_bitmap) {
         return bs->drv->bdrv_co_remove_persistent_dirty_bitmap(bs, name, errp);
     }
@@ -413,6 +414,7 @@ bdrv_co_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name,
                                    uint32_t granularity, Error **errp)
 {
     BlockDriver *drv = bs->drv;
+    assert_bdrv_graph_readable();
 
     if (!drv) {
         error_setg_errno(errp, ENOMEDIUM,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 9e441cb93b..3064822508 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -789,9 +789,11 @@ struct BlockDriver {
     void (*bdrv_drain_end)(BlockDriverState *bs);
 
     bool (*bdrv_supports_persistent_dirty_bitmap)(BlockDriverState *bs);
+    /* Called with graph rdlock held. */
     bool coroutine_fn (*bdrv_co_can_store_new_dirty_bitmap)(
         BlockDriverState *bs, const char *name, uint32_t granularity,
         Error **errp);
+    /* Called with graph rdlock held. */
     int coroutine_fn (*bdrv_co_remove_persistent_dirty_bitmap)(
         BlockDriverState *bs, const char *name, Error **errp);
 };
-- 
2.31.1

[PATCH 5/6] block/io: assert that BlockDriver->bdrv_co_*_snapshot_* are always called with graph rdlock taken

[Emanuele Giuseppe Esposito]

The only callers are other callback functions that already run with the
graph rdlock taken.

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

diff --git a/block/io.c b/block/io.c
index 62c0b3a390..7d1d0c48b0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3449,6 +3449,7 @@ bdrv_co_preadv_snapshot(BdrvChild *child, int64_t offset, int64_t bytes,
     BlockDriver *drv = bs->drv;
     int ret;
     IO_CODE();
+    assert_bdrv_graph_readable();
 
     if (!drv) {
         return -ENOMEDIUM;
@@ -3474,6 +3475,7 @@ bdrv_co_snapshot_block_status(BlockDriverState *bs,
     BlockDriver *drv = bs->drv;
     int ret;
     IO_CODE();
+    assert_bdrv_graph_readable();
 
     if (!drv) {
         return -ENOMEDIUM;
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 3064822508..03bd28e3c9 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -652,9 +652,12 @@ struct BlockDriver {
      * - be able to select a specific snapshot
      * - receive the snapshot's actual length (which may differ from bs's
      *   length)
+     *
+     * Called with graph rdlock taken.
      */
     int coroutine_fn (*bdrv_co_preadv_snapshot)(BlockDriverState *bs,
         int64_t offset, int64_t bytes, QEMUIOVector *qiov, size_t qiov_offset);
+    /* Called with graph rdlock taken. */
     int coroutine_fn (*bdrv_co_snapshot_block_status)(BlockDriverState *bs,
         bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
         int64_t *map, BlockDriverState **file);
-- 
2.31.1

[PATCH 6/6] block: assert that BlockDriver->bdrv_co_delete_file is always called with graph rdlock taken

[Emanuele Giuseppe Esposito]

The only callers are other callback functions that already run with the graph
rdlock taken.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                          | 1 +
 include/block/block_int-common.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index e54ed300d7..1a6ae08879 100644
--- a/block.c
+++ b/block.c
@@ -747,6 +747,7 @@ int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
 
     IO_CODE();
     assert(bs != NULL);
+    assert_bdrv_graph_readable();
 
     if (!bs->drv) {
         error_setg(errp, "Block node '%s' is not opened", bs->filename);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 03bd28e3c9..20308376c6 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -681,7 +681,7 @@ struct BlockDriver {
      */
     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
 
-    /* Delete a created file. */
+    /* Delete a created file. Called with graph rdlock taken. */
     int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,
                                             Error **errp);
 
-- 
2.31.1

Re: [PATCH 0/6] Protect the block layer with a rwlock: part 2

[Emanuele Giuseppe Esposito]

Ok, as I expected simple changes in a previous based-on serie provoke a
cascade of changes that inevitably affect these patches too.

While I strongly suggest to have an initial look at these patches
because it gives an idea on what am I trying to accomplish, I would not
go looking at nitpicks and trivial errors that came up from the based-on
series (ie "just as in the previous serie, fix this").

The order of the series is:
1. Still more coroutine and various fixes in block layer
2. Protect the block layer with a rwlock: part 1
3. Protect the block layer with a rwlock: part 2
4. Protect the block layer with a rwlock: part 3

Thank you,
Emanuele

Am 16/11/2022 um 14:53 schrieb Emanuele Giuseppe Esposito:
> Please read "Protect the block layer with a rwlock: part 1" for an additional
> introduction and aim of this series.
> 
> This second part aims to add the graph rdlock to the BlockDriver functions
> that already run in coroutine context and are classified as IO.
> Such functions will recursively traverse the BlockDriverState graph, therefore
> they need to be protected with the rdlock.
> 
> Based-on: <20221116134850.3051419-1-eesposit@redhat.com>
> 
> Thank you,
> Emanuele
> 
> Emanuele Giuseppe Esposito (6):
>   block: assert that bdrv_co_create is always called with graph rdlock
>     taken
>   block: assert that BlockDriver->bdrv_co_{amend/create} are called with
>     graph rdlock taken
>   block: assert that BlockDriver->bdrv_co_copy_range_{from/to} is always
>     called with graph rdlock taken
>   block/dirty-bitmap: assert that BlockDriver->bdrv_co_*_dirty_bitmap
>     are always called with graph rdlock taken
>   block/io: assert that BlockDriver->bdrv_co_*_snapshot_* are always
>     called with graph rdlock taken
>   block: assert that BlockDriver->bdrv_co_delete_file is always called
>     with graph rdlock taken
> 
>  block.c                          |  2 ++
>  block/amend.c                    |  1 +
>  block/block-backend.c            |  2 ++
>  block/create.c                   |  1 +
>  block/dirty-bitmap.c             |  2 ++
>  block/io.c                       |  7 +++++++
>  include/block/block_int-common.h | 14 +++++++++++++-
>  qemu-img.c                       |  4 +++-
>  8 files changed, 31 insertions(+), 2 deletions(-)
>