Re: [Qemu-devel] [PATCH 03/19] block: Make bdrv_drain() driver callbacks non-recursive

[Kevin Wolf <kwolf@redhat.com>]

Am 20.12.2017 um 14:16 hat Fam Zheng geschrieben:
> On Wed, 12/20 11:33, Kevin Wolf wrote:
> > bdrv_drain_begin() doesn't increase bs->quiesce_counter recursively,
> 
> Pretty trivial but:
> 
> s/bdrv_drain_begin/bdrv_drained_begin/

Yes, thanks.

> > which means that the child nodes are not actually drained. To keep this
> > consistent, we also shouldn't call the block driver callbacks.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/io.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index b94740b8ff..91a52e2d82 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -158,7 +158,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
> >  }
> >  
> >  /* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */
> > -static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
> > +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin, bool recursive)
> >  {
> >      BdrvChild *child, *tmp;
> >      BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
> > @@ -172,8 +172,10 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
> >      bdrv_coroutine_enter(bs, data.co);
> >      BDRV_POLL_WHILE(bs, !data.done);
> >  
> > -    QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
> > -        bdrv_drain_invoke(child->bs, begin);
> > +    if (recursive) {
> > +        QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
> > +            bdrv_drain_invoke(child->bs, begin, true);
> > +        }
> >      }
> >  }
> >  
> > @@ -265,7 +267,7 @@ void bdrv_drained_begin(BlockDriverState *bs)
> >          bdrv_parent_drained_begin(bs);
> >      }
> >  
> > -    bdrv_drain_invoke(bs, true);
> > +    bdrv_drain_invoke(bs, true, false);
> 
> I'm not sure I understand this patch. If we call bdrv_drained_begin on a quorum
> BDS that has a QED child, we do want to call bdrv_qed_co_drain_begin() on the
> child, don't we? I think after this patch, we don't do that any more?
> 
> The same question arises when I look at throttle_co_drain_begin()..

We don't increase bs->quiesce_counter for child nodes and don't notify
their parents, so they aren't really drained. Calling the BlcokDriver
callbacks is the only thing we did recursively. We should do one thing
consistently, and for bdrv_drained_begin/end() that is draining a single
node without the children.

Later in the series, I introduce a bdrv_subtree_drained_begin/end()
which doesn't only call the callbacks recursively, but also increases
bs->quiesce_counter etc. so that the child nodes are actually drained.

There are use cases for both functions.

Kevin