| Message ID | 20171205145424.24598-2-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Fix BlockDriver callbacks in bdrv_drain_all_begin() | expand |
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
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(-) >
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
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
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); }
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(-)