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

[Fam Zheng <famz@redhat.com>]

On Wed, 12/20 14:24, Kevin Wolf wrote:
> 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.
> 

OK, thanks.

Reviewed-by: Fam Zheng <famz@redhat.com>