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

[Kevin Wolf]

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.

> Then they can be put in block/io.c, and bdrv_do_drained_* can remain
> static.  (I would also consider extracting block/drain.c, but it is
> painful to do it now that you have this nice series---so let's do it after).

I can keep that in mind for part 3 (or 4, whatever it may become).

Kevin