From nobody Wed Nov 5 10:33:18 2025 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1499359626471307.82726553442865; Thu, 6 Jul 2017 09:47:06 -0700 (PDT) Received: from localhost ([::1]:52421 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dT9vj-0003Of-Rs for importer@patchew.org; Thu, 06 Jul 2017 12:47:03 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33334) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dT9o1-0005AP-Ig for qemu-devel@nongnu.org; Thu, 06 Jul 2017 12:39:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dT9nz-0006E6-N7 for qemu-devel@nongnu.org; Thu, 06 Jul 2017 12:39:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46910) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dT9nt-00069P-3G; Thu, 06 Jul 2017 12:38:57 -0400 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0CC4C4DD49; Thu, 6 Jul 2017 16:38:56 +0000 (UTC) Received: from donizetti.redhat.com (ovpn-117-229.ams2.redhat.com [10.36.117.229]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7075C60A9D; Thu, 6 Jul 2017 16:38:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0CC4C4DD49 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=pbonzini@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 0CC4C4DD49 From: Paolo Bonzini To: qemu-devel@nongnu.org Date: Thu, 6 Jul 2017 18:38:28 +0200 Message-Id: <20170706163828.24082-12-pbonzini@redhat.com> In-Reply-To: <20170706163828.24082-1-pbonzini@redhat.com> References: <20170706163828.24082-1-pbonzini@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 06 Jul 2017 16:38:56 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 11/11] block/snapshot: do not take AioContext lock X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: famz@redhat.com, stefanha@redhat.com, qemu-block@nongnu.org Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" 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 --- 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(BlockDriverSta= te *bs, } =20 =20 -/* 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. */ =20 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs) { @@ -395,13 +393,9 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_ba= d_bs) BdrvNextIterator it; =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { - AioContext *ctx =3D bdrv_get_aio_context(bs); - - aio_context_acquire(ctx); if (bdrv_is_inserted(bs) && !bdrv_is_read_only(bs)) { ok =3D bdrv_can_snapshot(bs); } - aio_context_release(ctx); if (!ok) { goto fail; } @@ -421,14 +415,10 @@ int bdrv_all_delete_snapshot(const char *name, BlockD= riverState **first_bad_bs, QEMUSnapshotInfo sn1, *snapshot =3D &sn1; =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { - AioContext *ctx =3D bdrv_get_aio_context(bs); - - aio_context_acquire(ctx); if (bdrv_can_snapshot(bs) && bdrv_snapshot_find(bs, snapshot, name) >=3D 0) { ret =3D 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, BlockDriv= erState **first_bad_bs) BdrvNextIterator it; =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { - AioContext *ctx =3D bdrv_get_aio_context(bs); - - aio_context_acquire(ctx); if (bdrv_can_snapshot(bs)) { err =3D 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, BlockDriv= erState **first_bad_bs) BdrvNextIterator it; =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { - AioContext *ctx =3D bdrv_get_aio_context(bs); - - aio_context_acquire(ctx); if (bdrv_can_snapshot(bs)) { err =3D 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; =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { - AioContext *ctx =3D bdrv_get_aio_context(bs); - - aio_context_acquire(ctx); if (bs =3D=3D vm_state_bs) { sn->vm_state_size =3D vm_state_size; err =3D bdrv_snapshot_create(bs, sn); @@ -509,7 +488,6 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn, sn->vm_state_size =3D 0; err =3D 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; =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { - AioContext *ctx =3D bdrv_get_aio_context(bs); bool found; =20 - aio_context_acquire(ctx); found =3D 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_s= ync(const char *device, Error **errp) { BlockDriverState *bs; - AioContext *aio_context; QEMUSnapshotInfo sn; Error *local_err =3D NULL; SnapshotInfo *info =3D NULL; @@ -1290,8 +1289,6 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_s= ync(const char *device, if (!bs) { return NULL; } - aio_context =3D bdrv_get_aio_context(bs); - aio_context_acquire(aio_context); =20 if (!has_id) { id =3D NULL; @@ -1303,34 +1300,32 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal= _sync(const char *device, =20 if (!id && !name) { error_setg(errp, "Name or id must be provided"); - goto out_aio_context; + return NULL; } =20 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, err= p)) { - goto out_aio_context; + return NULL; } =20 ret =3D bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_er= r); 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; } =20 bdrv_snapshot_delete(bs, id, name, &local_err); if (local_err) { error_propagate(errp, local_err); - goto out_aio_context; + return NULL; } =20 - aio_context_release(aio_context); - info =3D g_new0(SnapshotInfo, 1); info->id =3D g_strdup(sn.id_str); info->name =3D g_strdup(sn.name); @@ -1341,10 +1336,6 @@ SnapshotInfo *qmp_blockdev_snapshot_delete_internal_= sync(const char *device, info->vm_clock_sec =3D sn.vm_clock_nsec / 1000000000; =20 return info; - -out_aio_context: - aio_context_release(aio_context); - return NULL; } =20 /** @@ -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; } =20 - /* AioContext is released in .clean() */ - state->aio_context =3D bdrv_get_aio_context(bs); - aio_context_acquire(state->aio_context); - state->bs =3D bs; bdrv_drained_begin(bs); =20 @@ -1585,11 +1571,8 @@ static void internal_snapshot_clean(BlkActionState *= common) InternalSnapshotState *state =3D DO_UPCAST(InternalSnapshotState, common, common); =20 - 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); } } =20 @@ -1598,7 +1581,6 @@ typedef struct ExternalSnapshotState { BlkActionState common; BlockDriverState *old_bs; BlockDriverState *new_bs; - AioContext *aio_context; bool overlay_appended; } ExternalSnapshotState; =20 @@ -1654,9 +1636,7 @@ static void external_snapshot_prepare(BlkActionState = *common, return; } =20 - /* Acquire AioContext now so any threads operating on old_bs stop */ - state->aio_context =3D 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); =20 if (!bdrv_is_inserted(state->old_bs)) { @@ -1756,7 +1736,7 @@ static void external_snapshot_prepare(BlkActionState = *common, return; } =20 - bdrv_set_aio_context(state->new_bs, state->aio_context); + bdrv_set_aio_context(state->new_bs, bdrv_get_aio_context(state->old_bs= )); =20 /* This removes our old bs and adds the new bs. This is an operation t= hat * 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 =3D DO_UPCAST(ExternalSnapshotState, common, comm= on); =20 + 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 *c= ommon) { ExternalSnapshotState *state =3D DO_UPCAST(ExternalSnapshotState, common, comm= on); - 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 *qd= ict) int nb_sns, i; int total; int *global_snapshots; - AioContext *aio_context; =20 typedef struct SnapshotEntry { QEMUSnapshotInfo sn; @@ -1345,11 +1344,8 @@ void hmp_info_snapshots(Monitor *mon, const QDict *q= dict) monitor_printf(mon, "No available block device supports snapshots\= n"); return; } - aio_context =3D bdrv_get_aio_context(bs); =20 - aio_context_acquire(aio_context); nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); - aio_context_release(aio_context); =20 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 *qd= ict) int bs1_nb_sns =3D 0; ImageEntry *ie; SnapshotEntry *se; - AioContext *ctx =3D bdrv_get_aio_context(bs1); =20 - aio_context_acquire(ctx); if (bdrv_can_snapshot(bs1)) { sn =3D NULL; bs1_nb_sns =3D bdrv_snapshot_list(bs1, &sn); @@ -1380,7 +1374,6 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qd= ict) } g_free(sn); } - aio_context_release(ctx); } =20 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); =20 + /* + * 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); =20 =20 -/* 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. */ =20 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs); int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bs= d_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; =20 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 =3D bdrv_get_aio_context(bs); =20 saved_vm_running =3D runstate_is_running(); =20 @@ -2109,8 +2107,6 @@ int save_snapshot(const char *name, Error **errp) =20 bdrv_drain_all_begin(); =20 - aio_context_acquire(aio_context); - memset(sn, 0, sizeof(*sn)); =20 /* fill auxiliary fields */ @@ -2146,14 +2142,6 @@ int save_snapshot(const char *name, Error **errp) goto the_end; } =20 - /* The bdrv_all_create_snapshot() call that follows acquires the AioCo= ntext - * for itself. BDRV_POLL_WHILE() does not support nested locking beca= use - * it only releases the lock once. Therefore synchronous I/O will dea= dlock - * unless we release the AioContext before bdrv_all_create_snapshot(). - */ - aio_context_release(aio_context); - aio_context =3D NULL; - ret =3D 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 =3D 0; =20 the_end: - if (aio_context) { - aio_context_release(aio_context); - } - bdrv_drain_all_end(); =20 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 =3D migration_incoming_get_current(); =20 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 =3D bdrv_get_aio_context(bs_vm_state); =20 /* Don't even try to load empty VM states */ - aio_context_acquire(aio_context); ret =3D bdrv_snapshot_find(bs_vm_state, &sn, name); - aio_context_release(aio_context); if (ret < 0) { return ret; } else if (sn.vm_state_size =3D=3D 0) { @@ -2298,10 +2278,8 @@ int load_snapshot(const char *name, Error **errp) qemu_system_reset(SHUTDOWN_CAUSE_NONE); mis->from_src_file =3D f; =20 - aio_context_acquire(aio_context); ret =3D qemu_loadvm_state(f); migration_incoming_state_destroy(); - aio_context_release(aio_context); =20 bdrv_drain_all_end(); =20 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 c= har *str) =20 for (bs =3D bdrv_first(&it); bs; bs =3D bdrv_next(&it)) { SnapshotInfoList *snapshots, *snapshot; - AioContext *ctx =3D bdrv_get_aio_context(bs); - bool ok =3D false; =20 - aio_context_acquire(ctx); - if (bdrv_can_snapshot(bs)) { - ok =3D bdrv_query_snapshot_info_list(bs, &snapshots, NULL) =3D= =3D 0; - } - aio_context_release(ctx); - if (!ok) { + if (!bdrv_can_snapshot(bs) || + bdrv_query_snapshot_info_list(bs, &snapshots, NULL) !=3D 0) { continue; } =20 --=20 2.13.0