Re: [Qemu-devel] [Qemu-block] [PATCH 16/19] block: Allow graph changes in subtree drained section

[Kevin Wolf]

Am 20.12.2017 um 12:31 hat Paolo Bonzini geschrieben:
> On 20/12/2017 12:18, Kevin Wolf wrote:
> > Am 20.12.2017 um 11:51 hat Paolo Bonzini geschrieben:
> >> On 20/12/2017 11:34, Kevin Wolf wrote:
> >>>      .inherit_options = bdrv_inherited_options,
> >>>      .drained_begin   = bdrv_child_cb_drained_begin,
> >>>      .drained_end     = bdrv_child_cb_drained_end,
> >>> +    .attach          = bdrv_child_cb_attach,
> >>> +    .detach          = bdrv_child_cb_detach,
> >>>      .inactivate      = bdrv_child_cb_inactivate,
> >>>  };
> >>>  
> >>> @@ -911,6 +933,8 @@ const BdrvChildRole child_format = {
> >>>      .inherit_options = bdrv_inherited_fmt_options,
> >>>      .drained_begin   = bdrv_child_cb_drained_begin,
> >>>      .drained_end     = bdrv_child_cb_drained_end,
> >>> +    .attach          = bdrv_child_cb_attach,
> >>> +    .detach          = bdrv_child_cb_detach,
> >>>      .inactivate      = bdrv_child_cb_inactivate,
> >>
> >> Is there any case of a BdrvChildRole that doesn't want these callbacks?
> >> Maybe the functions should be called after ->attach and before ->detach
> >> (e.g. bdrv_{,un}apply_subtree_drain), rather than modifying the
> >> BdrvChildRole implementations.
> > 
> > At first I intended to implement it directly in
> > bdrv_replace_child_noperm(), but the thing is that you need the
> > bs->recursive_quiesce_counter of the parent BDS - but not all parents of
> > a BdrvChild are even a BDS. It could also be a BB root child or a block
> > job child. This is why we only have a void *opaque rather than a BDS
> > pointer for the parent.
> > 
> > The other option would be an additional BdrvChildRole callback like
> > .get_recursive_quiesce_counter, but compared to that, I like some code
> > in .attach/.detach better.
> 
> I see.  What about keeping the callbacks, but exporting
> 
> void bdrv_apply_subtree_drain(BlockDriverState *child,
>                             BlockDriverState *new_parent);
> void bdrv_unapply_subtree_drain(BlockDriverState *child,
>                               BlockDriverState *old_parent);
> 
> instead of bdrv_do_drained_{begin,end}?

Sure, that can be done. (BdrvChild *child, though.)

Kevin