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-common.h b/include/block/block-common.h
> new file mode 100644
> index 0000000000..4f1fd8de21
> --- /dev/null
> +++ b/include/block/block-common.h

[...]

> +#define BLKDBG_EVENT(child, evt) \
> +    do { \
> +        if (child) { \
> +            bdrv_debug_event(child->bs, evt); \
> +        } \
> +    } while (0)

This is defined twice, once here, and...

> 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

[...]

> +#define BLKDBG_EVENT(child, evt) \
> +    do { \
> +        if (child) { \
> +            bdrv_debug_event(child->bs, evt); \
> +        } \
> +    } while (0)

...once here.

[...]

> +/**
> + * bdrv_drained_begin:
> + *
> + * Begin a quiesced section for exclusive access to the BDS, by disabling
> + * external request sources including NBD server and device model. Note that
> + * this doesn't block timers or coroutines from submitting more requests, which
> + * means block_job_pause is still necessary.

Where does this sentence come from?  I can’t see it in master or in the 
lines removed from block.h:

> + *
> + * This function can be recursive.
> + */
> +void bdrv_drained_begin(BlockDriverState *bs);

[...]

> diff --git a/include/block/block.h b/include/block/block.h
> index e5dd22b034..1e6b8fef1e 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h

[...]

> -/**
> - * bdrv_drained_begin:
> - *
> - * Begin a quiesced section for exclusive access to the BDS, by disabling
> - * external request sources including NBD server, block jobs, and device model.
> - *
> - * This function can be recursive.
> - */
> -void bdrv_drained_begin(BlockDriverState *bs);