Snapshots are only created/destroyed/loaded under the BQL, while no
other I/O is happening. Snapshot information could be accessed while
other I/O is happening, but also under the BQL so they cannot be
modified concurrently. The AioContext lock is overkill. If needed,
in the future the BQL could be split to a separate lock covering all
snapshot operations, and the create/destroy/goto callbacks changed
to run in a coroutine (so the driver can do mutual exclusion as usual).
Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
---
block/snapshot.c | 28 +---------------------------
blockdev.c | 43 ++++++++++++-------------------------------
hmp.c | 7 -------
include/block/block_int.h | 5 +++++
include/block/snapshot.h | 4 +---
migration/savevm.c | 22 ----------------------
monitor.c | 10 ++--------
7 files changed, 21 insertions(+), 98 deletions(-)
diff --git a/block/snapshot.c b/block/snapshot.c
index a46564e7b7..08c59d6166 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -384,9 +384,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState
*bs,
}
-/* Group operations. All block drivers are involved.
- * These functions will properly handle dataplane (take aio_context_acquire
- * when appropriate for appropriate block drivers) */
+/* Group operations. All block drivers are involved. */
bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
{
@@ -395,13 +393,9 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- AioContext *ctx = bdrv_get_aio_context(bs);
-
- aio_context_acquire(ctx);
if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) {
ok = bdrv_can_snapshot(bs);
}
- aio_context_release(ctx);
if (!ok) {
goto fail;
}
@@ -421,14 +415,10 @@ int bdrv_all_delete_snapshot(const char *name,
BlockDriverState **first_bad_bs,
QEMUSnapshotInfo sn1, *snapshot = &sn1;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- AioContext *ctx = bdrv_get_aio_context(bs);
-
- aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0) {
ret = bdrv_snapshot_delete_by_id_or_name(bs, name, err);
}
- aio_context_release(ctx);
if (ret < 0) {
goto fail;
}
@@ -447,13 +437,9 @@ int bdrv_all_goto_snapshot(const char *name,
BlockDriverState **first_bad_bs)
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- AioContext *ctx = bdrv_get_aio_context(bs);
-
- aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs)) {
err = bdrv_snapshot_goto(bs, name);
}
- aio_context_release(ctx);
if (err < 0) {
goto fail;
}
@@ -472,13 +458,9 @@ int bdrv_all_find_snapshot(const char *name,
BlockDriverState **first_bad_bs)
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- AioContext *ctx = bdrv_get_aio_context(bs);
-
- aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs)) {
err = bdrv_snapshot_find(bs, &sn, name);
}
- aio_context_release(ctx);
if (err < 0) {
goto fail;
}
@@ -499,9 +481,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- AioContext *ctx = bdrv_get_aio_context(bs);
-
- aio_context_acquire(ctx);
if (bs == vm_state_bs) {
sn->vm_state_size = vm_state_size;
err = bdrv_snapshot_create(bs, sn);
@@ -509,7 +488,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
sn->vm_state_size = 0;
err = bdrv_snapshot_create(bs, sn);
}
- aio_context_release(ctx);
if (err < 0) {
goto fail;
}
@@ -526,13 +504,9 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
- AioContext *ctx = bdrv_get_aio_context(bs);
bool found;
- aio_context_acquire(ctx);
found = bdrv_can_snapshot(bs);
- aio_context_release(ctx);
-
if (found) {
break;
}
diff --git a/blockdev.c b/blockdev.c
index 88ab606949..56ef9c41a3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1280,7 +1280,6 @@ SnapshotInfo
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
Error **errp)
{
BlockDriverState *bs;
- AioContext *aio_context;
QEMUSnapshotInfo sn;
Error *local_err = NULL;
SnapshotInfo *info = NULL;
@@ -1290,8 +1289,6 @@ SnapshotInfo
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
if (!bs) {
return NULL;
}
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
if (!has_id) {
id = NULL;
@@ -1303,34 +1300,32 @@ SnapshotInfo
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
if (!id && !name) {
error_setg(errp, "Name or id must be provided");
- goto out_aio_context;
+ return NULL;
}
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) {
- goto out_aio_context;
+ return NULL;
}
ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- goto out_aio_context;
+ return NULL;
}
if (!ret) {
error_setg(errp,
"Snapshot with id '%s' and name '%s' does not exist on "
"device '%s'",
STR_OR_NULL(id), STR_OR_NULL(name), device);
- goto out_aio_context;
+ return NULL;
}
bdrv_snapshot_delete(bs, id, name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- goto out_aio_context;
+ return NULL;
}
- aio_context_release(aio_context);
-
info = g_new0(SnapshotInfo, 1);
info->id = g_strdup(sn.id_str);
info->name = g_strdup(sn.name);
@@ -1341,10 +1336,6 @@ SnapshotInfo
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
return info;
-
-out_aio_context:
- aio_context_release(aio_context);
- return NULL;
}
/**
@@ -1446,7 +1437,6 @@ struct BlkActionState {
typedef struct InternalSnapshotState {
BlkActionState common;
BlockDriverState *bs;
- AioContext *aio_context;
QEMUSnapshotInfo sn;
bool created;
} InternalSnapshotState;
@@ -1498,10 +1488,6 @@ static void internal_snapshot_prepare(BlkActionState
*common,
return;
}
- /* AioContext is released in .clean() */
- state->aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(state->aio_context);
-
state->bs = bs;
bdrv_drained_begin(bs);
@@ -1585,11 +1571,8 @@ static void internal_snapshot_clean(BlkActionState
*common)
InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
common, common);
- if (state->aio_context) {
- if (state->bs) {
- bdrv_drained_end(state->bs);
- }
- aio_context_release(state->aio_context);
+ if (state->bs) {
+ bdrv_drained_end(state->bs);
}
}
@@ -1598,7 +1581,6 @@ typedef struct ExternalSnapshotState {
BlkActionState common;
BlockDriverState *old_bs;
BlockDriverState *new_bs;
- AioContext *aio_context;
bool overlay_appended;
} ExternalSnapshotState;
@@ -1654,9 +1636,7 @@ static void external_snapshot_prepare(BlkActionState
*common,
return;
}
- /* Acquire AioContext now so any threads operating on old_bs stop */
- state->aio_context = bdrv_get_aio_context(state->old_bs);
- aio_context_acquire(state->aio_context);
+ /* Make any threads operating on old_bs stop */
bdrv_drained_begin(state->old_bs);
if (!bdrv_is_inserted(state->old_bs)) {
@@ -1756,7 +1736,7 @@ static void external_snapshot_prepare(BlkActionState
*common,
return;
}
- bdrv_set_aio_context(state->new_bs, state->aio_context);
+ bdrv_set_aio_context(state->new_bs, bdrv_get_aio_context(state->old_bs));
/* This removes our old bs and adds the new bs. This is an operation that
* can fail, so we need to do it in .prepare; undoing it for abort is
@@ -1775,6 +1755,8 @@ static void external_snapshot_commit(BlkActionState
*common)
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
+ bdrv_set_aio_context(state->new_bs, bdrv_get_aio_context(state->old_bs));
+
/* We don't need (or want) to use the transactional
* bdrv_reopen_multiple() across all the entries at once, because we
* don't want to abort all of them if one of them fails the reopen */
@@ -1803,9 +1785,8 @@ static void external_snapshot_clean(BlkActionState
*common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
- if (state->aio_context) {
+ if (state->old_bs) {
bdrv_drained_end(state->old_bs);
- aio_context_release(state->aio_context);
bdrv_unref(state->new_bs);
}
}
diff --git a/hmp.c b/hmp.c
index 8c72c58b20..3c63ea7a72 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1321,7 +1321,6 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
int nb_sns, i;
int total;
int *global_snapshots;
- AioContext *aio_context;
typedef struct SnapshotEntry {
QEMUSnapshotInfo sn;
@@ -1345,11 +1344,8 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
monitor_printf(mon, "No available block device supports snapshots\n");
return;
}
- aio_context = bdrv_get_aio_context(bs);
- aio_context_acquire(aio_context);
nb_sns = bdrv_snapshot_list(bs, &sn_tab);
- aio_context_release(aio_context);
if (nb_sns < 0) {
monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
@@ -1360,9 +1356,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
int bs1_nb_sns = 0;
ImageEntry *ie;
SnapshotEntry *se;
- AioContext *ctx = bdrv_get_aio_context(bs1);
- aio_context_acquire(ctx);
if (bdrv_can_snapshot(bs1)) {
sn = NULL;
bs1_nb_sns = bdrv_snapshot_list(bs1, &sn);
@@ -1380,7 +1374,6 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
}
g_free(sn);
}
- aio_context_release(ctx);
}
if (no_snapshot) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 43c9f4fcae..38d1067fa8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -217,6 +217,11 @@ struct BlockDriver {
int coroutine_fn (*bdrv_co_pwritev_compressed)(BlockDriverState *bs,
uint64_t offset, uint64_t bytes, QEMUIOVector *qiov);
+ /*
+ * Snapshots are only created/destroyed/loaded under the BQL, while no
+ * other I/O is happening. snapshots/nb_snapshots is read while other
+ * I/O is happening, but also under the BQL.
+ */
int (*bdrv_snapshot_create)(BlockDriverState *bs,
QEMUSnapshotInfo *sn_info);
int (*bdrv_snapshot_goto)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index e5c0553115..735d0f694c 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -76,9 +76,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
Error **errp);
-/* Group operations. All block drivers are involved.
- * These functions will properly handle dataplane (take aio_context_acquire
- * when appropriate for appropriate block drivers */
+/* Group operations. All block drivers are involved. */
bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
diff --git a/migration/savevm.c b/migration/savevm.c
index c7a49c93c5..1b168c3ba8 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2073,7 +2073,6 @@ int save_snapshot(const char *name, Error **errp)
uint64_t vm_state_size;
qemu_timeval tv;
struct tm tm;
- AioContext *aio_context;
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp, "Device '%s' is writable but does not support "
@@ -2096,7 +2095,6 @@ int save_snapshot(const char *name, Error **errp)
error_setg(errp, "No block device can accept snapshots");
return ret;
}
- aio_context = bdrv_get_aio_context(bs);
saved_vm_running = runstate_is_running();
@@ -2109,8 +2107,6 @@ int save_snapshot(const char *name, Error **errp)
bdrv_drain_all_begin();
- aio_context_acquire(aio_context);
-
memset(sn, 0, sizeof(*sn));
/* fill auxiliary fields */
@@ -2146,14 +2142,6 @@ int save_snapshot(const char *name, Error **errp)
goto the_end;
}
- /* The bdrv_all_create_snapshot() call that follows acquires the AioContext
- * for itself. BDRV_POLL_WHILE() does not support nested locking because
- * it only releases the lock once. Therefore synchronous I/O will deadlock
- * unless we release the AioContext before bdrv_all_create_snapshot().
- */
- aio_context_release(aio_context);
- aio_context = NULL;
-
ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
if (ret < 0) {
error_setg(errp, "Error while creating snapshot on '%s'",
@@ -2164,10 +2152,6 @@ int save_snapshot(const char *name, Error **errp)
ret = 0;
the_end:
- if (aio_context) {
- aio_context_release(aio_context);
- }
-
bdrv_drain_all_end();
if (saved_vm_running) {
@@ -2241,7 +2225,6 @@ int load_snapshot(const char *name, Error **errp)
QEMUSnapshotInfo sn;
QEMUFile *f;
int ret;
- AioContext *aio_context;
MigrationIncomingState *mis = migration_incoming_get_current();
if (!bdrv_all_can_snapshot(&bs)) {
@@ -2263,12 +2246,9 @@ int load_snapshot(const char *name, Error **errp)
error_setg(errp, "No block device supports snapshots");
return -ENOTSUP;
}
- aio_context = bdrv_get_aio_context(bs_vm_state);
/* Don't even try to load empty VM states */
- aio_context_acquire(aio_context);
ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
- aio_context_release(aio_context);
if (ret < 0) {
return ret;
} else if (sn.vm_state_size == 0) {
@@ -2298,10 +2278,8 @@ int load_snapshot(const char *name, Error **errp)
qemu_system_reset(SHUTDOWN_CAUSE_NONE);
mis->from_src_file = f;
- aio_context_acquire(aio_context);
ret = qemu_loadvm_state(f);
migration_incoming_state_destroy();
- aio_context_release(aio_context);
bdrv_drain_all_end();
diff --git a/monitor.c b/monitor.c
index 3c369f4dd5..48687aa94d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3639,15 +3639,9 @@ static void vm_completion(ReadLineState *rs, const char
*str)
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
SnapshotInfoList *snapshots, *snapshot;
- AioContext *ctx = bdrv_get_aio_context(bs);
- bool ok = false;
- aio_context_acquire(ctx);
- if (bdrv_can_snapshot(bs)) {
- ok = bdrv_query_snapshot_info_list(bs, &snapshots, NULL) == 0;
- }
- aio_context_release(ctx);
- if (!ok) {
+ if (!bdrv_can_snapshot(bs) ||
+ bdrv_query_snapshot_info_list(bs, &snapshots, NULL) != 0) {
continue;
}
--
2.13.0
