Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()

[Kevin Wolf]

Am 25.11.2022 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 11/18/22 20:41, Kevin Wolf wrote:
> > In order to make sure that bdrv_replace_child_noperm() doesn't have to
> > poll any more, get rid of the bdrv_parent_drained_begin_single() call.
> > 
> > This is possible now because we can require that the parent is already
> > drained through the child in question when the function is called and we
> > don't call the parent drain callbacks more than once.
> > 
> > The additional drain calls needed in callers cause the test case to run
> > its code in the drain handler too early (bdrv_attach_child() drains
> > now), so modify it to only enable the code after the test setup has
> > completed.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> It's still hard to keep this all in mind, so weak:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> > ---
> >   include/block/block-io.h     |   8 +++
> >   block.c                      | 103 ++++++++++++++++++++++++++++++-----
> >   block/io.c                   |   2 +-
> >   tests/unit/test-bdrv-drain.c |  10 ++++
> >   4 files changed, 108 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/block/block-io.h b/include/block/block-io.h
> > index 8f5e75756a..65e6d2569b 100644
> > --- a/include/block/block-io.h
> > +++ b/include/block/block-io.h
> > @@ -292,6 +292,14 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector 
> > *qiov, int64_t pos);
> >    */
> >   void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll);
> > +/**
> > + * bdrv_parent_drained_poll_single:
> > + *
> > + * Returns true if there is any pending activity to cease before @c can be
> > + * called quiesced, false otherwise.
> > + */
> > +bool bdrv_parent_drained_poll_single(BdrvChild *c);
> > +
> >   /**
> >    * bdrv_parent_drained_end_single:
> >    *
> > diff --git a/block.c b/block.c
> > index d18512944d..3f12aba6ce 100644
> > --- a/block.c
> > +++ b/block.c
> 
> [..]
> 
> > @@ -2863,11 +2905,9 @@ static void bdrv_replace_child_noperm(BdrvChild 
> > *child,
> >       }
> >       /*
> > -     * If the old child node was drained but the new one is not, allow
> > -     * requests to come in only after the new node has been attached.
> > -     *
> > -     * Update new_bs_quiesce_counter because 
> > bdrv_parent_drained_begin_single()
> > -     * polls, which could have changed the value.
> > +     * If the parent was drained through this BdrvChild previously, but 
> > new_bs
> > +     * is not drained, allow requests to come in only after the new node 
> > has
> > +     * been attached.
> 
> As I understand,the main reason why we MUST do this automatic undrain, is the 
> contract with the user:
> 
> User things that:
> 
> 1. It's enough to drain node X to drain all its parents (thanks to recursion)
> 2. User should undrain exactly same nodes that was drained by hand, 
> everything that was drained automatically would be automatically undrained.
> 
> So here we break the connection between X and its parent, therefore recursion 
> will not help on final undrain. So, we should do the automation here.

Yes, I think that's the idea behind our interface.

> I have an idea how to (probably) make things even more simple.
> 
> 1. drop .quiesced_parent
> 
> 2. consider the Full graph, including non-bds parents, and support having 
> .quiesce_counter for non-bds parents. (probably need some structure to unify 
> bds and non-bds nodes of the Full graph - Generic ndoe, that's not the first 
> time we are saying about that)
> 
> 3. drop any recursion and automatic drain/undrain
> 
> 4. user is responsible to drain all the nodes and their parents as needed to 
> proceed with some block graph modification
> 
> 5. user keeps the list of all nodes that was drained to undrain them in the 
> end
> 
> 6. node may be drained only when all its parents are already drained (add an 
> assertion)
> 
> And of course we need helpers for the user to do 4-6. That should work 
> similar to permissions update. Add a function to produce a topologically 
> sorted list of Generic nodes (starting from some node and include all its 
> parents and their parents and so on), and  simple functions that to 
> drain/undrain of such list in a loop.

I understand your idea and it looks nice at the first sight.

However, on second thought, I'm not sure how easy and nice this would
actually turn out: You will lose the invariant that if a node is
drained, its parent will always be drained, too - it depends on the
caller now. You also can't delete a node that is still drained, you need
to keep it around for undraining. Same thing with graph modifications
in a drained section: You won't automatically get a consistent state
regarding drain in the new graph layout, so you would have to manually
make sure that both old and new children are drained.

It feels like this will lead to new complications that might not be any
easier to manage than the old ones.

Kevin