From nobody Sat Apr 12 09:13:22 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 (208.118.235.17 [208.118.235.17]) by mx.zohomail.com with SMTPS id 1513697280276782.5250530494042; Tue, 19 Dec 2017 07:28:00 -0800 (PST) Received: from localhost ([::1]:49360 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eRJo4-0008Mz-4c for importer@patchew.org; Tue, 19 Dec 2017 10:27:48 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57539) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eRJZX-0004cp-07 for qemu-devel@nongnu.org; Tue, 19 Dec 2017 10:12:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eRJZV-0006s6-D4 for qemu-devel@nongnu.org; Tue, 19 Dec 2017 10:12:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36334) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eRJZV-0006qg-2S for qemu-devel@nongnu.org; Tue, 19 Dec 2017 10:12:45 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 51FC35BEC9; Tue, 19 Dec 2017 15:12:44 +0000 (UTC) Received: from localhost (ovpn-117-161.ams2.redhat.com [10.36.117.161]) by smtp.corp.redhat.com (Postfix) with ESMTP id B37F35C552; Tue, 19 Dec 2017 15:12:38 +0000 (UTC) From: Stefan Hajnoczi To: Date: Tue, 19 Dec 2017 15:11:29 +0000 Message-Id: <20171219151144.11120-9-stefanha@redhat.com> In-Reply-To: <20171219151144.11120-1-stefanha@redhat.com> References: <20171219151144.11120-1-stefanha@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 19 Dec 2017 15:12:44 +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] [PULL v2 08/23] 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: Peter Maydell , Stefan Hajnoczi 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 Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-3-stefanha@redhat.com 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