[03/19] block: Make bdrv_drain() driver callbacks non-recursive
diff mbox

Message ID 20171220103412.13048-4-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf Dec. 20, 2017, 10:33 a.m. UTC
bdrv_drain_begin() doesn't increase bs->quiesce_counter recursively,
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(-)

Comments

Fam Zheng Dec. 20, 2017, 1:16 p.m. UTC | #1
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/

> 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()..

Fam
Kevin Wolf Dec. 20, 2017, 1:24 p.m. UTC | #2
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
Fam Zheng Dec. 20, 2017, 1:31 p.m. UTC | #3
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>

Patch
diff mbox

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);
     bdrv_drain_recurse(bs);
 }
 
@@ -281,7 +283,7 @@  void bdrv_drained_end(BlockDriverState *bs)
     }
 
     /* Re-enable things in child-to-parent order */
-    bdrv_drain_invoke(bs, false);
+    bdrv_drain_invoke(bs, false, false);
     bdrv_parent_drained_end(bs);
     aio_enable_external(bdrv_get_aio_context(bs));
 }
@@ -345,7 +347,7 @@  void bdrv_drain_all_begin(void)
         aio_context_acquire(aio_context);
         aio_disable_external(aio_context);
         bdrv_parent_drained_begin(bs);
-        bdrv_drain_invoke(bs, true);
+        bdrv_drain_invoke(bs, true, true);
         aio_context_release(aio_context);
 
         if (!g_slist_find(aio_ctxs, aio_context)) {
@@ -388,7 +390,7 @@  void bdrv_drain_all_end(void)
 
         /* Re-enable things in child-to-parent order */
         aio_context_acquire(aio_context);
-        bdrv_drain_invoke(bs, false);
+        bdrv_drain_invoke(bs, false, true);
         bdrv_parent_drained_end(bs);
         aio_enable_external(aio_context);
         aio_context_release(aio_context);