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 <kw...@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 <f...@redhat.com>