On Thu, 09/21 16:17, Manos Pitsidianakis wrote: > BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
s/bdrv_do_drain/bdrv_co_drain/ > of the drain. The throttle driver (block/throttle.c) needs a way to mark the > end of the drain in order to toggle io_limits_disabled correctly, thus > bdrv_co_drain_end is needed. > > Signed-off-by: Manos Pitsidianakis <el13...@mail.ntua.gr> > --- > include/block/block_int.h | 6 ++++++ > block/io.c | 48 > +++++++++++++++++++++++++++++++++-------------- > 2 files changed, 40 insertions(+), 14 deletions(-) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index ba4c383393..21950cfda3 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -356,8 +356,14 @@ struct BlockDriver { > /** > * Drain and stop any internal sources of requests in the driver, and > * remain so until next I/O callback (e.g. bdrv_co_writev) is called. This line needs update too, maybe: /** * bdrv_co_drain drains and stops any ... and remain so until * bdrv_co_drain_end is called. > + * > + * The callbacks are called at the beginning and ending of the drain > + * respectively. They should be implemented if the driver needs to e.g. As implied by above change, should we explicitly require "should both be implemented"? It may not be technically required, but I think is cleaner and easier to reason about. > + * manage scheduled I/O requests, or toggle an internal state. After an s/After an/After a/ > + * bdrv_co_drain_end() invocation new requests will continue normally. > */ > void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs); > + void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs); > > void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child, > Error **errp); > diff --git a/block/io.c b/block/io.c > index 4378ae4c7d..b0a10ad3ef 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -153,6 +153,7 @@ typedef struct { > Coroutine *co; > BlockDriverState *bs; > bool done; > + bool begin; > } BdrvCoDrainData; > > static void coroutine_fn bdrv_drain_invoke_entry(void *opaque) > @@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void > *opaque) > BdrvCoDrainData *data = opaque; > BlockDriverState *bs = data->bs; > > - bs->drv->bdrv_co_drain(bs); > + if (data->begin) { > + bs->drv->bdrv_co_drain(bs); > + } else { > + bs->drv->bdrv_co_drain_end(bs); > + } > > /* Set data->done before reading bs->wakeup. */ > atomic_mb_set(&data->done, true); > bdrv_wakeup(bs); > } > > -static void bdrv_drain_invoke(BlockDriverState *bs) > +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin) > { > - BdrvCoDrainData data = { .bs = bs, .done = false }; > + BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin}; > > - if (!bs->drv || !bs->drv->bdrv_co_drain) { > + if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) || > + (!begin && !bs->drv->bdrv_co_drain_end)) { > return; > } > > @@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs) > BDRV_POLL_WHILE(bs, !data.done); > } > > -static bool bdrv_drain_recurse(BlockDriverState *bs) > +static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin) > { > BdrvChild *child, *tmp; > bool waited; > > - waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0); > - > /* Ensure any pending metadata writes are submitted to bs->file. */ > - bdrv_drain_invoke(bs); > + bdrv_drain_invoke(bs, begin); > + > + /* Wait for drained requests to finish */ > + waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0); > > QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { > BlockDriverState *bs = child->bs; > @@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs) > */ > bdrv_ref(bs); > } > - waited |= bdrv_drain_recurse(bs); > + waited |= bdrv_drain_recurse(bs, begin); > if (in_main_loop) { > bdrv_unref(bs); > } > @@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque) > BlockDriverState *bs = data->bs; > > bdrv_dec_in_flight(bs); > - bdrv_drained_begin(bs); > + if (data->begin) { > + bdrv_drained_begin(bs); > + } else { > + bdrv_drained_end(bs); > + } > + > data->done = true; > aio_co_wake(co); > } > > -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs) > +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > + bool begin) > { > BdrvCoDrainData data; > > @@ -239,6 +252,7 @@ static void coroutine_fn > bdrv_co_yield_to_drain(BlockDriverState *bs) > .co = qemu_coroutine_self(), > .bs = bs, > .done = false, > + .begin = begin, > }; > bdrv_inc_in_flight(bs); > aio_bh_schedule_oneshot(bdrv_get_aio_context(bs), > @@ -253,7 +267,7 @@ static void coroutine_fn > bdrv_co_yield_to_drain(BlockDriverState *bs) > void bdrv_drained_begin(BlockDriverState *bs) > { > if (qemu_in_coroutine()) { > - bdrv_co_yield_to_drain(bs); > + bdrv_co_yield_to_drain(bs, true); > return; > } > > @@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs) > bdrv_parent_drained_begin(bs); > } > > - bdrv_drain_recurse(bs); > + bdrv_drain_recurse(bs, true); > } > > void bdrv_drained_end(BlockDriverState *bs) > { > + if (qemu_in_coroutine()) { > + bdrv_co_yield_to_drain(bs, false); > + return; > + } > assert(bs->quiesce_counter > 0); > if (atomic_fetch_dec(&bs->quiesce_counter) > 1) { > return; > } > > bdrv_parent_drained_end(bs); > + bdrv_drain_recurse(bs, false); > aio_enable_external(bdrv_get_aio_context(bs)); > } > > @@ -350,7 +369,7 @@ 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)) { > - waited |= bdrv_drain_recurse(bs); > + waited |= bdrv_drain_recurse(bs, true); > } > } > aio_context_release(aio_context); > @@ -371,6 +390,7 @@ void bdrv_drain_all_end(void) > aio_context_acquire(aio_context); > aio_enable_external(aio_context); > bdrv_parent_drained_end(bs); > + bdrv_drain_recurse(bs, false); > aio_context_release(aio_context); > } > > -- > 2.11.0 >