Re: [PATCH v4 02/25] include/block/block: split header into I/O and global state API

[Hanna Reitz]

On 25.10.21 12:17, Emanuele Giuseppe Esposito wrote:
> block.h currently contains a mix of functions:
> some of them run under the BQL and modify the block layer graph,
> others are instead thread-safe and perform I/O in iothreads.
> It is not easy to understand which function is part of which
> group (I/O vs GS), and this patch aims to clarify it.
>
> The "GS" functions need the BQL, and often use
> aio_context_acquire/release and/or drain to be sure they
> can modify the graph safely.
> The I/O function are instead thread safe, and can run in
> any AioContext.
>
> By splitting the header in two files, block-io.h
> and block-global-state.h we have a clearer view on what
> needs what kind of protection. block-common.h
> contains common structures shared by both headers.
>
> block.h is left there for legacy and to avoid changing
> all includes in all c files that use the block APIs.
>
> Assertions are added in the next patch.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block.c                            |   3 +
>   block/meson.build                  |   7 +-
>   include/block/block-common.h       | 389 +++++++++++++
>   include/block/block-global-state.h | 286 ++++++++++
>   include/block/block-io.h           | 306 ++++++++++
>   include/block/block.h              | 878 +----------------------------
>   6 files changed, 1012 insertions(+), 857 deletions(-)
>   create mode 100644 include/block/block-common.h
>   create mode 100644 include/block/block-global-state.h
>   create mode 100644 include/block/block-io.h

[...]

> diff --git a/include/block/block-io.h b/include/block/block-io.h
> new file mode 100644
> index 0000000000..9af4609ccb
> --- /dev/null
> +++ b/include/block/block-io.h

[...]

> +/*
> + * I/O API functions. These functions are thread-safe, and therefore
> + * can run in any thread as long as the thread has called
> + * aio_context_acquire/release().
> + */
> +
> +int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs,
> +                          Error **errp);

Why is this function here?  Naïvely, I would’ve assumed as a 
graph-modifying function it should be in block-global-state.h.

I mean, perhaps it’s thread-safe and then it can fit here, too. Still, 
it surprises me a bit to find this here.

Hanna