[1/4] block: Make bdrv_drain_invoke() recursive

Message ID 20171205145424.24598-2-kwolf@redhat.com
State New
Headers show
Series
  • block: Fix BlockDriver callbacks in bdrv_drain_all_begin()
Related show

Commit Message

Kevin Wolf Dec. 5, 2017, 2:54 p.m.
This change separates bdrv_drain_invoke(), which calls the BlockDriver
drain callbacks, from bdrv_drain_recurse(). Instead, the function
performs its own recursion now.

One reason for this is that bdrv_drain_recurse() can be called multiple
times bdrv_drain_recurse(), but the callbacks may only be called once.
The separation is necessary to fix this bug.

The other reasons is that we intend to go to model where we call all
driver callbacks first, and only then start polling. This is not fully
achieved yet with this patch, as bdrv_drain_invoke() contains a
BDRV_POLL_WHILE() loop for the block driver callbacks which can still
call callbacks for any unrelated event. It's a step in this direction
anyway.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Paolo Bonzini Dec. 5, 2017, 3:02 p.m. | #1
On 05/12/2017 15:54, Kevin Wolf wrote:
>      }
>  
> +    bdrv_drain_invoke(bs, true);
>      bdrv_drain_recurse(bs, true);
>  }
>  
> @@ -294,6 +298,7 @@ void bdrv_drained_end(BlockDriverState *bs)
>      }
>  
>      bdrv_parent_drained_end(bs);
> +    bdrv_drain_invoke(bs, false);
>      bdrv_drain_recurse(bs, false);
>      aio_enable_external(bdrv_get_aio_context(bs));

I think invoke should be done after recurse from bdrv_drain*end.  In the
end aio_enable_external is a special kind of drain_end callback, so
bdrv_drain_invoke should go together with it.

Thanks,

Paolo
Eric Blake Dec. 5, 2017, 3:15 p.m. | #2
On 12/05/2017 08:54 AM, Kevin Wolf wrote:
> This change separates bdrv_drain_invoke(), which calls the BlockDriver
> drain callbacks, from bdrv_drain_recurse(). Instead, the function
> performs its own recursion now.
> 
> One reason for this is that bdrv_drain_recurse() can be called multiple
> times bdrv_drain_recurse(),

Doesn't read right.  Should the second instance be changed to 'by
itself', or is the correct meaning something else?

> but the callbacks may only be called once.
> The separation is necessary to fix this bug.
> 
> The other reasons is that we intend to go to model where we call all

s/reasons/reason/

> driver callbacks first, and only then start polling. This is not fully
> achieved yet with this patch, as bdrv_drain_invoke() contains a
> BDRV_POLL_WHILE() loop for the block driver callbacks which can still
> call callbacks for any unrelated event. It's a step in this direction
> anyway.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
Kevin Wolf Dec. 5, 2017, 3:22 p.m. | #3
Am 05.12.2017 um 16:02 hat Paolo Bonzini geschrieben:
> On 05/12/2017 15:54, Kevin Wolf wrote:
> >      }
> >  
> > +    bdrv_drain_invoke(bs, true);
> >      bdrv_drain_recurse(bs, true);
> >  }
> >  
> > @@ -294,6 +298,7 @@ void bdrv_drained_end(BlockDriverState *bs)
> >      }
> >  
> >      bdrv_parent_drained_end(bs);
> > +    bdrv_drain_invoke(bs, false);
> >      bdrv_drain_recurse(bs, false);
> >      aio_enable_external(bdrv_get_aio_context(bs));
> 
> I think invoke should be done after recurse from bdrv_drain*end.  In the
> end aio_enable_external is a special kind of drain_end callback, so
> bdrv_drain_invoke should go together with it.

Yes and no.

Calling them together makes sense to me. Not for this patch, though,
which is just mechanical refactoring. bdrv_drain_invoke() was at the
beginning of bdrv_drain_recurse(), so it ends up before it after the
refactoring.

But there is really no reason at all to call bdrv_drain_recurse() in
bdrv_drained_end(). We only called it because it happened to involve
bdrv_drain_invoke(). After this patch, the call can be removed. I can do
that in a v2 (in a separate patch).

What we probably also should do is move bdrv_parent_drained_end() after
bdrv_drain_invoke() so that children are ready to accept new requests
when the parent callback is called. There is also some inconsistency
between bdrv_drain() and bdrv_drain_all() about the order of these
operations. I hope that in the end they would just call the same helper
and only the actual polling loop stays separate (as long as needed).

Kevin
Kevin Wolf Dec. 5, 2017, 3:23 p.m. | #4
Am 05.12.2017 um 16:15 hat Eric Blake geschrieben:
> On 12/05/2017 08:54 AM, Kevin Wolf wrote:
> > This change separates bdrv_drain_invoke(), which calls the BlockDriver
> > drain callbacks, from bdrv_drain_recurse(). Instead, the function
> > performs its own recursion now.
> > 
> > One reason for this is that bdrv_drain_recurse() can be called multiple
> > times bdrv_drain_recurse(),
> 
> Doesn't read right.  Should the second instance be changed to 'by
> itself', or is the correct meaning something else?

This should be "by bdrv_drain_all_begin()". Thanks!

Kevin

Patch

diff --git a/block/io.c b/block/io.c
index 6773926fc1..096468b761 100644
--- a/block/io.c
+++ b/block/io.c
@@ -175,8 +175,10 @@  static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
     bdrv_wakeup(bs);
 }
 
+/* Recursively call BlockDriver.bdrv_co_drain_begin/end callbacks */
 static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
+    BdrvChild *child, *tmp;
     BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
     if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
@@ -187,6 +189,10 @@  static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
     data.co = qemu_coroutine_create(bdrv_drain_invoke_entry, &data);
     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);
+    }
 }
 
 static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
@@ -194,9 +200,6 @@  static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
     BdrvChild *child, *tmp;
     bool waited;
 
-    /* Ensure any pending metadata writes are submitted to bs->file.  */
-    bdrv_drain_invoke(bs, begin);
-
     /* Wait for drained requests to finish */
     waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
 
@@ -279,6 +282,7 @@  void bdrv_drained_begin(BlockDriverState *bs)
         bdrv_parent_drained_begin(bs);
     }
 
+    bdrv_drain_invoke(bs, true);
     bdrv_drain_recurse(bs, true);
 }
 
@@ -294,6 +298,7 @@  void bdrv_drained_end(BlockDriverState *bs)
     }
 
     bdrv_parent_drained_end(bs);
+    bdrv_drain_invoke(bs, false);
     bdrv_drain_recurse(bs, false);
     aio_enable_external(bdrv_get_aio_context(bs));
 }
@@ -372,6 +377,8 @@  void bdrv_drain_all_begin(void)
             aio_context_acquire(aio_context);
             for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
                 if (aio_context == bdrv_get_aio_context(bs)) {
+                    /* FIXME Calling this multiple times is wrong */
+                    bdrv_drain_invoke(bs, true);
                     waited |= bdrv_drain_recurse(bs, true);
                 }
             }
@@ -393,6 +400,7 @@  void bdrv_drain_all_end(void)
         aio_context_acquire(aio_context);
         aio_enable_external(aio_context);
         bdrv_parent_drained_end(bs);
+        bdrv_drain_invoke(bs, false);
         bdrv_drain_recurse(bs, false);
         aio_context_release(aio_context);
     }