On 13/05/2015 13:08, Fam Zheng wrote:
> > I think this isn't enough. It's the callers of bdrv_drain and
> > bdrv_drain_all that need to block before drain and unblock before
> > aio_context_release.
>
> Which callers do you mean? qmp_transaction is covered in this series.
All of them. :(
In some cases it may be unnecessary. I'm thinking of bdrv_set_aio_context,
and mig_save_device_dirty, but that's probably the exception rather than
the rule.
In some cases, like bdrv_snapshot_delete, the change is trivial.
The only somewhat more complex case is probably block/mirror.c. There,
the aio_context_release happens through block_job_sleep_ns or
qemu_coroutine_yield. I guess the change would look something like
this sketch:
diff --git a/block/mirror.c b/block/mirror.c
index 58f391a..dcfede0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -513,16 +513,29 @@ static void coroutine_fn mirror_run(void *opaque)
if (cnt == 0 && should_complete) {
/* The dirty bitmap is not updated while operations are pending.
- * If we're about to exit, wait for pending operations before
- * calling bdrv_get_dirty_count(bs), or we may exit while the
+ * If we're about to exit, wait for pending operations and
+ * recheck bdrv_get_dirty_count(bs), or we may exit while the
* source has dirty data to copy!
*
* Note that I/O can be submitted by the guest while
- * mirror_populate runs.
+ * the rest of mirror_populate runs, but we lock it out here.
*/
trace_mirror_before_drain(s, cnt);
+
+ // ... block ...
bdrv_drain(bs);
cnt = bdrv_get_dirty_count(s->dirty_bitmap);
+
+ if (cnt == 0) {
+ /* The two disks are in sync. Exit and report successful
+ * completion.
+ */
+ assert(s->synced && QLIST_EMPTY(&bs->tracked_requests));
+ s->common.cancelled = false;
+ break; // ... and unblock somewhere else...
+ }
+
+ // ... unblock ...
}
ret = 0;
@@ -535,13 +549,6 @@ static void coroutine_fn mirror_run(void *opaque)
} else if (!should_complete) {
delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
- } else if (cnt == 0) {
- /* The two disks are in sync. Exit and report successful
- * completion.
- */
- assert(QLIST_EMPTY(&bs->tracked_requests));
- s->common.cancelled = false;
- break;
}
last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
}
It can be the topic of a separate series. But this patch brings a
false sense of security (either the blocker is unnecessary, or it
needs to last after bdrv_drain returns), so I think it should be
dropped.
Paolo
