From nobody Tue Feb 10 05:50:31 2026 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 1512470920139423.88730849218484; Tue, 5 Dec 2017 02:48:40 -0800 (PST) Received: from localhost ([::1]:47790 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eMAm9-0000Z9-Dh for importer@patchew.org; Tue, 05 Dec 2017 05:48:33 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54514) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eMAfp-00027d-VV for qemu-devel@nongnu.org; Tue, 05 Dec 2017 05:42:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eMAfo-0002OW-JV for qemu-devel@nongnu.org; Tue, 05 Dec 2017 05:42:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37614) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eMAfl-0002M8-GA; Tue, 05 Dec 2017 05:41:57 -0500 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 984EF883AB; Tue, 5 Dec 2017 10:41:56 +0000 (UTC) Received: from localhost (ovpn-117-172.ams2.redhat.com [10.36.117.172]) by smtp.corp.redhat.com (Postfix) with ESMTP id 23F8B60841; Tue, 5 Dec 2017 10:41:55 +0000 (UTC) From: Stefan Hajnoczi To: Date: Tue, 5 Dec 2017 10:41:34 +0000 Message-Id: <20171205104141.28882-3-stefanha@redhat.com> In-Reply-To: <20171205104141.28882-1-stefanha@redhat.com> References: <20171205104141.28882-1-stefanha@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.26]); Tue, 05 Dec 2017 10:41: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 2/9] block: don't keep AioContext acquired after external_snapshot_prepare() 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: Kevin Wolf , John Snow , Stefan Hajnoczi , 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" It is not necessary to hold AioContext across transactions anymore since bdrv_drained_begin/end() is used to keep the nodes quiesced. In fact, using the AioContext lock for this purpose was always buggy. This patch reduces the scope of AioContext locked regions. This is not just a cleanup but also fixes hangs that occur in BDRV_POLL_WHILE() because it is unware of recursive locking and does not release the AioContext the necessary number of times to allow progress to be made. Signed-off-by: Stefan Hajnoczi --- blockdev.c | 71 ++++++++++++++++++++++++++++++++++++++++++----------------= ---- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3c8d994ced..3b598f8f0e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1606,7 +1606,6 @@ typedef struct ExternalSnapshotState { BlkActionState common; BlockDriverState *old_bs; BlockDriverState *new_bs; - AioContext *aio_context; bool overlay_appended; } ExternalSnapshotState; =20 @@ -1626,6 +1625,7 @@ static void external_snapshot_prepare(BlkActionState = *common, ExternalSnapshotState *state =3D DO_UPCAST(ExternalSnapshotState, common, comm= on); TransactionAction *action =3D common->action; + AioContext *aio_context; =20 /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar * purpose but a different set of parameters */ @@ -1662,31 +1662,32 @@ static void external_snapshot_prepare(BlkActionStat= e *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); + aio_context =3D bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + + /* Paired with .clean() */ bdrv_drained_begin(state->old_bs); =20 if (!bdrv_is_inserted(state->old_bs)) { error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); - return; + goto out; } =20 if (bdrv_op_is_blocked(state->old_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { - return; + goto out; } =20 if (!bdrv_is_read_only(state->old_bs)) { if (bdrv_flush(state->old_bs)) { error_setg(errp, QERR_IO_ERROR); - return; + goto out; } } =20 if (!bdrv_is_first_non_filter(state->old_bs)) { error_setg(errp, QERR_FEATURE_DISABLED, "snapshot"); - return; + goto out; } =20 if (action->type =3D=3D TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC= ) { @@ -1698,13 +1699,13 @@ static void external_snapshot_prepare(BlkActionStat= e *common, =20 if (node_name && !snapshot_node_name) { error_setg(errp, "New snapshot node name missing"); - return; + goto out; } =20 if (snapshot_node_name && bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) { error_setg(errp, "New snapshot node name already in use"); - return; + goto out; } =20 flags =3D state->old_bs->open_flags; @@ -1717,7 +1718,7 @@ static void external_snapshot_prepare(BlkActionState = *common, int64_t size =3D bdrv_getlength(state->old_bs); if (size < 0) { error_setg_errno(errp, -size, "bdrv_getlength failed"); - return; + goto out; } bdrv_img_create(new_image_file, format, state->old_bs->filename, @@ -1725,7 +1726,7 @@ static void external_snapshot_prepare(BlkActionState = *common, NULL, size, flags, false, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } } =20 @@ -1740,30 +1741,30 @@ static void external_snapshot_prepare(BlkActionStat= e *common, errp); /* We will manually add the backing_hd field to the bs later */ if (!state->new_bs) { - return; + goto out; } =20 if (bdrv_has_blk(state->new_bs)) { error_setg(errp, "The snapshot is already in use"); - return; + goto out; } =20 if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { - return; + goto out; } =20 if (state->new_bs->backing !=3D NULL) { error_setg(errp, "The snapshot already has a backing image"); - return; + goto out; } =20 if (!state->new_bs->drv->supports_backing) { error_setg(errp, "The snapshot does not support backing images"); - return; + goto out; } =20 - bdrv_set_aio_context(state->new_bs, state->aio_context); + bdrv_set_aio_context(state->new_bs, aio_context); =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 @@ -1772,15 +1773,22 @@ static void external_snapshot_prepare(BlkActionStat= e *common, bdrv_append(state->new_bs, state->old_bs, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } state->overlay_appended =3D true; + +out: + aio_context_release(aio_context); } =20 static void external_snapshot_commit(BlkActionState *common) { ExternalSnapshotState *state =3D DO_UPCAST(ExternalSnapshotState, common, comm= on); + AioContext *aio_context; + + aio_context =3D bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); =20 /* We don't need (or want) to use the transactional * bdrv_reopen_multiple() across all the entries at once, because we @@ -1789,6 +1797,8 @@ static void external_snapshot_commit(BlkActionState *= common) bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDW= R, NULL); } + + aio_context_release(aio_context); } =20 static void external_snapshot_abort(BlkActionState *common) @@ -1797,11 +1807,18 @@ static void external_snapshot_abort(BlkActionState = *common) DO_UPCAST(ExternalSnapshotState, common, comm= on); if (state->new_bs) { if (state->overlay_appended) { + AioContext *aio_context; + + aio_context =3D bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + bdrv_ref(state->old_bs); /* we can't let bdrv_set_backind_hd= () close state->old_bs; we need it = */ bdrv_set_backing_hd(state->new_bs, NULL, &error_abort); bdrv_replace_node(state->new_bs, state->old_bs, &error_abort); bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_b= s */ + + aio_context_release(aio_context); } } } @@ -1810,11 +1827,19 @@ static void external_snapshot_clean(BlkActionState = *common) { ExternalSnapshotState *state =3D DO_UPCAST(ExternalSnapshotState, common, comm= on); - if (state->aio_context) { - bdrv_drained_end(state->old_bs); - bdrv_unref(state->new_bs); - aio_context_release(state->aio_context); + AioContext *aio_context; + + if (!state->old_bs) { + return; } + + aio_context =3D bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->old_bs); + bdrv_unref(state->new_bs); + + aio_context_release(aio_context); } =20 typedef struct DriveBackupState { --=20 2.14.3