On 2017-10-10 11:47, Kevin Wolf wrote: > 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.
Er, well, yes. >> 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). Arguably any BDS is a filter BDS regarding its backing BDS. So maybe I could add a new function filter_child_bs(). > Also, if we attack this at the backing_bs() level, we need to audit > code that it doesn't directly use bs->backing. Yup. Maybe the best idea is to leave this patch for a follow-up... >> @@ -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. OK then. :-) Max
signature.asc
Description: OpenPGP digital signature