| Message ID | 1445569694-2765-6-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Oct 23, 2015 at 11:08:09AM +0800, Fam Zheng wrote: > +void bdrv_drained_begin(BlockDriverState *bs) > +{ > + if (!bs->quiesce_counter++) { > + aio_disable_external(bdrv_get_aio_context(bs)); > + } > + bdrv_drain(bs); > +} > + > +void bdrv_drained_end(BlockDriverState *bs) > +{ > + assert(bs->quiesce_counter > 0); > + if (--bs->quiesce_counter > 0) { > + return; > + } > + aio_enable_external(bdrv_get_aio_context(bs)); > +} Why is quiesce_counter necessary? Can't we just rely on AioContext's disable_external_cnt? void bdrv_drained_begin(BlockDriverState *bs) { aio_disable_external(bdrv_get_aio_context(bs)); bdrv_drain(bs); } void bdrv_drained_end(BlockDriverState *bs) { aio_enable_external(bdrv_get_aio_context(bs)); }
On Fri, Oct 23, 2015 at 11:08:09AM +0800, Fam Zheng wrote: > +/** > + * bdrv_drained_begin: > + * > + * Begin a quiesced section for exclusive access to the BDS, by disabling > + * external request sources including NBD server and device model. Note that > + * this doesn't block timers or coroutines from submitting more requests, which > + * means block_job_pause is still necessary. How is the 'transaction' command protected from block jobs? Maybe I missed a block_job_pause() call that you added in this series...
On Fri, 10/23 16:13, Stefan Hajnoczi wrote: > On Fri, Oct 23, 2015 at 11:08:09AM +0800, Fam Zheng wrote: > > +void bdrv_drained_begin(BlockDriverState *bs) > > +{ > > + if (!bs->quiesce_counter++) { > > + aio_disable_external(bdrv_get_aio_context(bs)); > > + } > > + bdrv_drain(bs); > > +} > > + > > +void bdrv_drained_end(BlockDriverState *bs) > > +{ > > + assert(bs->quiesce_counter > 0); > > + if (--bs->quiesce_counter > 0) { > > + return; > > + } > > + aio_enable_external(bdrv_get_aio_context(bs)); > > +} > > Why is quiesce_counter necessary? Can't we just rely on AioContext's > disable_external_cnt? It was added because bdrv_drain was conditional in a previous version, so yes it can now be dropped, but we lose the explcitness of the assertion in bdrv_drained_end. I was thinking that more "assert(!bs->quiesce_counter)" can be useful in blk_aio_read/write etc.. Fam
On Fri, 10/23 16:24, Stefan Hajnoczi wrote: > On Fri, Oct 23, 2015 at 11:08:09AM +0800, Fam Zheng wrote: > > +/** > > + * bdrv_drained_begin: > > + * > > + * Begin a quiesced section for exclusive access to the BDS, by disabling > > + * external request sources including NBD server and device model. Note that > > + * this doesn't block timers or coroutines from submitting more requests, which > > + * means block_job_pause is still necessary. > > How is the 'transaction' command protected from block jobs? Maybe I > missed a block_job_pause() call that you added in this series... There is no block job that can change guest visible data now, and we don't support multiple block jobs on one BDS. That's why a call wasn't added in this series. The comment actually is speaking generally for potential callers other than qmp_transaction. Fam
diff --git a/block/io.c b/block/io.c index 2fd7a1d..5ac6256 100644 --- a/block/io.c +++ b/block/io.c @@ -2624,3 +2624,20 @@ void bdrv_flush_io_queue(BlockDriverState *bs) } bdrv_start_throttled_reqs(bs); } + +void bdrv_drained_begin(BlockDriverState *bs) +{ + if (!bs->quiesce_counter++) { + aio_disable_external(bdrv_get_aio_context(bs)); + } + bdrv_drain(bs); +} + +void bdrv_drained_end(BlockDriverState *bs) +{ + assert(bs->quiesce_counter > 0); + if (--bs->quiesce_counter > 0) { + return; + } + aio_enable_external(bdrv_get_aio_context(bs)); +} diff --git a/include/block/block.h b/include/block/block.h index 28d903c..5d722a7 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -610,4 +610,23 @@ void bdrv_io_plug(BlockDriverState *bs); void bdrv_io_unplug(BlockDriverState *bs); void bdrv_flush_io_queue(BlockDriverState *bs); +/** + * bdrv_drained_begin: + * + * Begin a quiesced section for exclusive access to the BDS, by disabling + * external request sources including NBD server and device model. Note that + * this doesn't block timers or coroutines from submitting more requests, which + * means block_job_pause is still necessary. + * + * This function can be recursive. + */ +void bdrv_drained_begin(BlockDriverState *bs); + +/** + * bdrv_drained_end: + * + * End a quiescent section started by bdrv_drained_begin(). + */ +void bdrv_drained_end(BlockDriverState *bs); + #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index e472a03..e317b14 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -448,6 +448,8 @@ struct BlockDriverState { /* threshold limit for writes, in bytes. "High water mark". */ uint64_t write_threshold_offset; NotifierWithReturn write_threshold_notifier; + + int quiesce_counter; }; struct BlockBackendRootState {
The semantics is that after bdrv_drained_begin(bs), bs will not get new external requests until the matching bdrv_drained_end(bs). Signed-off-by: Fam Zheng <famz@redhat.com> --- block/io.c | 17 +++++++++++++++++ include/block/block.h | 19 +++++++++++++++++++ include/block/block_int.h | 2 ++ 3 files changed, 38 insertions(+)