| Message ID | 20170706163828.24082-12-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 06, 2017 at 06:38:28PM +0200, Paolo Bonzini wrote: > 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 <pbonzini@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. */ Perhaps "These functions must be called under the BQL"? My concern with this patch series in general is that it will lead to bugs due to inconsistencies and lack of locking documentation: bdrv_all_delete_snapshot() is called by hmp_delvm() outside a bdrv_drained_begin() region. That's okay because internally bdrv_snapshot_delete() will call bdrv_drained_begin() for the crucial operations that require a quiesced BDS. Compare that with bdrv_all_goto_snapshot(), which is called inside a bdrv_drained_begin() region by load_snapshot(). Internally it doesn't drain. Previously the bdrv_all_*() functions behaved consistently. We could say that they will acquire AioContexts themselves. Now they behave inconsistently and while the code currently happens to work, there is no structure that will keep it working as it is modified. I think we're reaching a point where every BlockDriver callback and every bdrv_*() function needs the following information: 1. Must be called under BQL? 2. Can I/O requests be in flight? 3. Is it thread-safe? Otherwise it will be a nightmare to modify the code since these constraints are not enforced by the compiler and undocumented.
On 10/07/2017 18:24, Stefan Hajnoczi wrote: > On Thu, Jul 06, 2017 at 06:38:28PM +0200, Paolo Bonzini wrote: >> 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 <pbonzini@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. */ > > Perhaps "These functions must be called under the BQL"? > > My concern with this patch series in general is that it will lead to > bugs due to inconsistencies and lack of locking documentation: > > bdrv_all_delete_snapshot() is called by hmp_delvm() outside a > bdrv_drained_begin() region. That's okay because internally > bdrv_snapshot_delete() will call bdrv_drained_begin() for the crucial > operations that require a quiesced BDS. > > Compare that with bdrv_all_goto_snapshot(), which is called inside a > bdrv_drained_begin() region by load_snapshot(). Internally it doesn't > drain. I think generally we should move bdrv_drained_begin/end calls _out_ of block.c and into qmp_*. If you agree, I can add this before this patch. > Previously the bdrv_all_*() functions behaved consistently. We could > say that they will acquire AioContexts themselves. Now they behave > inconsistently and while the code currently happens to work, there is no > structure that will keep it working as it is modified. > > I think we're reaching a point where every BlockDriver callback and > every bdrv_*() function needs the following information: > > 1. Must be called under BQL? > 2. Can I/O requests be in flight? > 3. Is it thread-safe? > > Otherwise it will be a nightmare to modify the code since these > constraints are not enforced by the compiler and undocumented. Good point. Are (1) and (3) different ways to say the same thing or do you have other differences in mind? More long term, I think snapshot functions should be changed to run in coroutines. This way they can just take the driver CoMutex. Paolo
On Mon, Jul 10, 2017 at 06:27:27PM +0200, Paolo Bonzini wrote: > On 10/07/2017 18:24, Stefan Hajnoczi wrote: > > On Thu, Jul 06, 2017 at 06:38:28PM +0200, Paolo Bonzini wrote: > >> 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 <pbonzini@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. */ > > > > Perhaps "These functions must be called under the BQL"? > > > > My concern with this patch series in general is that it will lead to > > bugs due to inconsistencies and lack of locking documentation: > > > > bdrv_all_delete_snapshot() is called by hmp_delvm() outside a > > bdrv_drained_begin() region. That's okay because internally > > bdrv_snapshot_delete() will call bdrv_drained_begin() for the crucial > > operations that require a quiesced BDS. > > > > Compare that with bdrv_all_goto_snapshot(), which is called inside a > > bdrv_drained_begin() region by load_snapshot(). Internally it doesn't > > drain. > > I think generally we should move bdrv_drained_begin/end calls _out_ of > block.c and into qmp_*. If you agree, I can add this before this patch. Yes, I think drained regions should be in callers. That way callers can perform multiple operations in sequence and the result is atomic. > > Previously the bdrv_all_*() functions behaved consistently. We could > > say that they will acquire AioContexts themselves. Now they behave > > inconsistently and while the code currently happens to work, there is no > > structure that will keep it working as it is modified. > > > > I think we're reaching a point where every BlockDriver callback and > > every bdrv_*() function needs the following information: > > > > 1. Must be called under BQL? > > 2. Can I/O requests be in flight? > > 3. Is it thread-safe? > > > > Otherwise it will be a nightmare to modify the code since these > > constraints are not enforced by the compiler and undocumented. > > Good point. Are (1) and (3) different ways to say the same thing or do > you have other differences in mind? There is a difference between (1) and (3). If a function is thread-safe then callers can invoke it without any constraints. If a function requires a specific lock to be held then that it puts a constraint on the caller. We do have thread-safe functions in the block layer: simple functions that fetch a BDS field atomically. There is no need to hold the BQL for them.
On 11/07/2017 11:43, Stefan Hajnoczi wrote: >>> >>> 1. Must be called under BQL? >>> 2. Can I/O requests be in flight? >>> 3. Is it thread-safe? >>> >>> Otherwise it will be a nightmare to modify the code since these >>> constraints are not enforced by the compiler and undocumented. >> Good point. Are (1) and (3) different ways to say the same thing or do >> you have other differences in mind? > There is a difference between (1) and (3). If a function is thread-safe > then callers can invoke it without any constraints. If a function > requires a specific lock to be held then that it puts a constraint on > the caller. > > We do have thread-safe functions in the block layer: simple functions > that fetch a BDS field atomically. There is no need to hold the BQL for > them. Ok, so we are in (somewhat violent) agreement. "It's not thread-safe" pretty much means "must be called under BQL". I look forward to extending Marc-André's -Wthread-safety experiments to cover bdrv_drained_begin/end! Paolo
On Tue, Jul 11, 2017 at 11:48:02AM +0200, Paolo Bonzini wrote: > On 11/07/2017 11:43, Stefan Hajnoczi wrote: > >>> > >>> 1. Must be called under BQL? > >>> 2. Can I/O requests be in flight? > >>> 3. Is it thread-safe? > >>> > >>> Otherwise it will be a nightmare to modify the code since these > >>> constraints are not enforced by the compiler and undocumented. > >> Good point. Are (1) and (3) different ways to say the same thing or do > >> you have other differences in mind? > > There is a difference between (1) and (3). If a function is thread-safe > > then callers can invoke it without any constraints. If a function > > requires a specific lock to be held then that it puts a constraint on > > the caller. > > > > We do have thread-safe functions in the block layer: simple functions > > that fetch a BDS field atomically. There is no need to hold the BQL for > > them. > > Ok, so we are in (somewhat violent) agreement. "It's not thread-safe" > pretty much means "must be called under BQL". > > I look forward to extending Marc-André's -Wthread-safety experiments to > cover bdrv_drained_begin/end! Sounds good :). Stefan
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; }
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 <pbonzini@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(-)