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 <mre...@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
