Re: [Qemu-devel] [PATCH 10/18] block/mirror: Make source the file child

[Kevin Wolf <kwolf@redhat.com>]

Am 13.09.2017 um 20:19 hat Max Reitz geschrieben:
> Regarding the source BDS, the mirror BDS is arguably a filter node.
> Therefore, the source BDS should be its "file" child.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

TODO: Justification why this doesn't break things like
bdrv_is_allocated_above() that iterate through the backing chain.

>  block/mirror.c             | 127 ++++++++++++++++++++++++++++++++++-----------
>  block/qapi.c               |  25 ++++++---
>  tests/qemu-iotests/141.out |   4 +-
>  3 files changed, 119 insertions(+), 37 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 7fa2437923..ee792d0cbc 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -147,9 +147,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>  
>          /* Skip automatically inserted nodes that the user isn't aware of for
>           * query-block (blk != NULL), but not for query-named-block-nodes */
> -        while (blk && bs0->drv && bs0->implicit) {
> -            bs0 = backing_bs(bs0);
> -            assert(bs0);
> +        while (blk && bs0 && bs0->drv && bs0->implicit) {
> +            if (bs0->backing) {
> +                bs0 = backing_bs(bs0);
> +            } else {
> +                assert(bs0->file);
> +                bs0 = bs0->file->bs;
> +            }
>          }
>      }

Maybe backing_bs() should skip filters? If so, need to show that all
existing users of backing_bs() really want to skip filters. If not,
explain why the missing backing_bs() callers don't need it (I'm quite
sure that some do).

Also, if we attack this at the backing_bs() level, we need to audit
code that it doesn't directly use bs->backing.

> @@ -1135,44 +1146,88 @@ static const BlockJobDriver commit_active_job_driver = {
>      .drain                  = mirror_drain,
>  };
>  
> +static void source_child_inherit_fmt_options(int *child_flags,
> +                                             QDict *child_options,
> +                                             int parent_flags,
> +                                             QDict *parent_options)
> +{
> +    child_backing.inherit_options(child_flags, child_options,
> +                                  parent_flags, parent_options);
> +}
> +
> +static char *source_child_get_parent_desc(BdrvChild *c)
> +{
> +    return child_backing.get_parent_desc(c);
> +}
> +
> +static void source_child_cb_drained_begin(BdrvChild *c)
> +{
> +    BlockDriverState *bs = c->opaque;
> +    MirrorBDSOpaque *s = bs->opaque;
> +
> +    if (s && s->job) {
> +        block_job_drained_begin(&s->job->common);
> +    }
> +    bdrv_drained_begin(bs);
> +}
> +
> +static void source_child_cb_drained_end(BdrvChild *c)
> +{
> +    BlockDriverState *bs = c->opaque;
> +    MirrorBDSOpaque *s = bs->opaque;
> +
> +    if (s && s->job) {
> +        block_job_drained_end(&s->job->common);
> +    }
> +    bdrv_drained_end(bs);
> +}
> +
> +static BdrvChildRole source_child_role = {
> +    .inherit_options    = source_child_inherit_fmt_options,
> +    .get_parent_desc    = source_child_get_parent_desc,
> +    .drained_begin      = source_child_cb_drained_begin,
> +    .drained_end        = source_child_cb_drained_end,
> +};

Wouldn't it make much more sense to use a standard child role and just
implement BlockDriver callbacks for .bdrv_drained_begin/end? It seems
that master still only has .bdrv_co_drain (which is begin), but one of
Manos' pending series adds the missing end callback.

Kevin