Re: [RFC v4 7/9] config: add check to block layer

[Stefan Hajnoczi <stefanha@gmail.com>]

On Mon, 11 Jul 2022 at 22:22, Sam Li <faithilikerun@gmail.com> wrote:
>
> Putting zoned/non-zoned BlockDrivers on top of each other is not
> allowed.
>
> Signed-off-by: Sam Li <faithilikerun@gmail.com>
> ---
>  block.c                          | 7 +++++++
>  block/file-posix.c               | 2 ++
>  include/block/block_int-common.h | 5 +++++
>  3 files changed, 14 insertions(+)
>
> diff --git a/block.c b/block.c
> index 2c00dddd80..0e24582c7d 100644
> --- a/block.c
> +++ b/block.c
> @@ -7945,6 +7945,13 @@ void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs,
>          return;
>      }
>
> +    if (parent_bs->drv->is_zoned != child_bs->drv->is_zoned) {
> +        error_setg(errp, "Cannot add a %s child to a %s parent",
> +                   child_bs->drv->is_zoned ? "zoned" : "non-zoned",
> +                   parent_bs->drv->is_zoned ? "zoned" : "non-zoned");
> +        return;
> +    }

Please explain the rationale:

/*
 * Non-zoned block drivers do not follow zoned storage constraints
(i.e. sequential writes
 * to zones). Refuse mixing zoned and non-zoned drivers in a graph.
 */

> +
>      if (!QLIST_EMPTY(&child_bs->parents)) {
>          error_setg(errp, "The node %s already has a parent",
>                     child_bs->node_name);
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 42708012ff..e9ad1d8e1e 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -3924,6 +3924,7 @@ static BlockDriver bdrv_host_device = {
>      .format_name        = "host_device",
>      .protocol_name        = "host_device",
>      .instance_size      = sizeof(BDRVRawState),
> +    .is_zoned = false,

In C static struct fields are automatically initialized to 0/false.
This line can be omitted.

>      .bdrv_needs_filename = true,
>      .bdrv_probe_device  = hdev_probe_device,
>      .bdrv_parse_filename = hdev_parse_filename,
> @@ -3971,6 +3972,7 @@ static BlockDriver bdrv_zoned_host_device = {
>          .format_name = "zoned_host_device",
>          .protocol_name = "zoned_host_device",
>          .instance_size = sizeof(BDRVRawState),
> +        .is_zoned = true,
>          .bdrv_needs_filename = true,
>          .bdrv_probe_device  = hdev_probe_device,
>          .bdrv_parse_filename = hdev_parse_filename,
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index 6037871089..29f1ec9184 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -141,6 +141,11 @@ struct BlockDriver {
>       */
>      bool is_format;
>
> +    /*
> +     * Set to true if the BlockDriver is a zoned block driver.
> +     */
> +    bool is_zoned;

This isn't powerful enough to express the constraints.
block/raw-format.c supports both non-zoned and zoned children (after
your patch) but it won't pass the check.

Perhaps add bool supports_zoned_children and change the check to:

if (child_bs->drv->is_zoned &&
!parent_bs->drv->supports_zoned_children) { ...refuse... }

Then raw-format would work on top of zoned_host_device as well as
still working on non-zoned BDSes.

Stefan