[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 03/25] assertions for block global state API

From: Hanna Reitz
Subject: Re: [PATCH v4 03/25] assertions for block global state API
Date: Thu, 11 Nov 2021 17:32:46 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
All the global state (GS) API functions will check that
qemu_in_main_thread() returns true. If not, it means
that the safety of BQL cannot be guaranteed, and
they need to be moved to I/O.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
  block.c        | 136 +++++++++++++++++++++++++++++++++++++++++++++++--
  block/commit.c |   2 +
  block/io.c     |  20 ++++++++
  blockdev.c     |   1 +
  4 files changed, 156 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 6fdb4d7712..672f946065 100644
--- a/block.c
+++ b/block.c


@@ -5606,7 +5678,6 @@ int64_t bdrv_getlength(BlockDriverState *bs)
  void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
      int64_t nb_sectors = bdrv_nb_sectors(bs);
      *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;

This hunk seems at least unrelated.


@@ -5958,6 +6043,7 @@ const char *bdrv_get_parent_name(const BlockDriverState 
  /* TODO check what callers really want: bs->node_name or blk_name() */
  const char *bdrv_get_device_name(const BlockDriverState *bs)
+    assert(qemu_in_main_thread());
      return bdrv_get_parent_name(bs) ?: "";

This function is invoked from qcow2_signal_corruption(), which comes generally from an I/O path.  Is it safe to assert that we’re in the main thread here?

Well, the question is probably rather whether this needs really be a considered a global-state function, or whether putting it in common or I/O is fine.  I believe you’re right given that it invokes bdrv_get_parent_name(), it cannot be thread-safe, but then we’ll have to change qcow2_signal_corruption() so it doesn’t invoke this function.


diff --git a/block/io.c b/block/io.c
index bb0a254def..c5d7f8495e 100644
--- a/block/io.c
+++ b/block/io.c


@@ -544,6 +546,7 @@ void bdrv_drained_end(BlockDriverState *bs)
void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter)
+    assert(qemu_in_main_thread());
      bdrv_do_drained_end(bs, false, NULL, false, drained_end_counter);

Why is bdrv_drained_end an I/O function and this is a GS function, even though it does just a subset?

@@ -586,12 +589,14 @@ void bdrv_unapply_subtree_drain(BdrvChild *child, 
BlockDriverState *old_parent)
  void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
+    assert(qemu_in_main_thread());
void bdrv_drain(BlockDriverState *bs)
+    assert(qemu_in_main_thread());

Why are these GS functions when both bdrv_drained_begin() and bdrv_drained_end() are I/O functions?

I can understand making the drain_all functions GS functions, but it seems weird to say it’s an I/O function when a single BDS is drained via bdrv_drained_begin() and bdrv_drained_end(), but not via bdrv_drain(), which just does both.

(I can see that there are no I/O path callers, but I still find it strange.)


@@ -2731,6 +2742,7 @@ int bdrv_block_status_above(BlockDriverState *bs, 
BlockDriverState *base,
  int bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes,
                        int64_t *pnum, int64_t *map, BlockDriverState **file)
+    assert(qemu_in_main_thread());
      return bdrv_block_status_above(bs, bdrv_filter_or_cow_bs(bs),
                                     offset, bytes, pnum, map, file);

Why is this a GS function as opposed to all other block-status functions?  Because of the bdrv_filter_or_cow_bs() call?

And isn’t the call from nvme_block_status_all() basically an I/O path?  (Or is that always run in the main thread?)

@@ -2800,6 +2812,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
                              int64_t bytes, int64_t *pnum)
      int depth;
      int ret = bdrv_common_block_status_above(top, base, include_base, false,
                                               offset, bytes, pnum, NULL, NULL,

This hunk too seems unrelated.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]