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

[Hanna Reitz]

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 
> *bs)
>   /* 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_coroutine());
> +    assert(qemu_in_main_thread());
>       bdrv_drained_begin(bs);
>       bdrv_drained_end(bs);
>   }
>   
>   void bdrv_drain(BlockDriverState *bs)
>   {
> +    assert(qemu_in_main_thread());
>       bdrv_drained_begin(bs);
>       bdrv_drained_end(bs);
>   }

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,
>                                                &depth);

This hunk too seems unrelated.

Hanna