[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PULL 08/23] block: don't keep AioContext acquired after ex
From: |
Stefan Hajnoczi |
Subject: |
[Qemu-devel] [PULL 08/23] block: don't keep AioContext acquired after external_snapshot_prepare() |
Date: |
Mon, 18 Dec 2017 14:35:15 +0000 |
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 <address@hidden>
Reviewed-by: Kevin Wolf <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
Message-id: address@hidden
Signed-off-by: Stefan Hajnoczi <address@hidden>
---
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;
@@ -1626,6 +1625,7 @@ static void external_snapshot_prepare(BlkActionState
*common,
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
TransactionAction *action = common->action;
+ AioContext *aio_context;
/* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
* purpose but a different set of parameters */
@@ -1662,31 +1662,32 @@ 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);
+ aio_context = bdrv_get_aio_context(state->old_bs);
+ aio_context_acquire(aio_context);
+
+ /* Paired with .clean() */
bdrv_drained_begin(state->old_bs);
if (!bdrv_is_inserted(state->old_bs)) {
error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
- return;
+ goto out;
}
if (bdrv_op_is_blocked(state->old_bs,
BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
- return;
+ goto out;
}
if (!bdrv_is_read_only(state->old_bs)) {
if (bdrv_flush(state->old_bs)) {
error_setg(errp, QERR_IO_ERROR);
- return;
+ goto out;
}
}
if (!bdrv_is_first_non_filter(state->old_bs)) {
error_setg(errp, QERR_FEATURE_DISABLED, "snapshot");
- return;
+ goto out;
}
if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
@@ -1698,13 +1699,13 @@ static void external_snapshot_prepare(BlkActionState
*common,
if (node_name && !snapshot_node_name) {
error_setg(errp, "New snapshot node name missing");
- return;
+ goto out;
}
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;
}
flags = state->old_bs->open_flags;
@@ -1717,7 +1718,7 @@ static void external_snapshot_prepare(BlkActionState
*common,
int64_t size = 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;
}
}
@@ -1740,30 +1741,30 @@ static void external_snapshot_prepare(BlkActionState
*common,
errp);
/* We will manually add the backing_hd field to the bs later */
if (!state->new_bs) {
- return;
+ goto out;
}
if (bdrv_has_blk(state->new_bs)) {
error_setg(errp, "The snapshot is already in use");
- return;
+ goto out;
}
if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
errp)) {
- return;
+ goto out;
}
if (state->new_bs->backing != NULL) {
error_setg(errp, "The snapshot already has a backing image");
- return;
+ goto out;
}
if (!state->new_bs->drv->supports_backing) {
error_setg(errp, "The snapshot does not support backing images");
- return;
+ goto out;
}
- bdrv_set_aio_context(state->new_bs, state->aio_context);
+ bdrv_set_aio_context(state->new_bs, aio_context);
/* 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
@@ -1772,15 +1773,22 @@ static void external_snapshot_prepare(BlkActionState
*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 = true;
+
+out:
+ aio_context_release(aio_context);
}
static void external_snapshot_commit(BlkActionState *common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
+ AioContext *aio_context;
+
+ aio_context = bdrv_get_aio_context(state->old_bs);
+ aio_context_acquire(aio_context);
/* 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_RDWR,
NULL);
}
+
+ aio_context_release(aio_context);
}
static void external_snapshot_abort(BlkActionState *common)
@@ -1797,11 +1807,18 @@ static void external_snapshot_abort(BlkActionState
*common)
DO_UPCAST(ExternalSnapshotState, common, common);
if (state->new_bs) {
if (state->overlay_appended) {
+ AioContext *aio_context;
+
+ aio_context = 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_bs */
+
+ aio_context_release(aio_context);
}
}
}
@@ -1810,11 +1827,19 @@ static void external_snapshot_clean(BlkActionState
*common)
{
ExternalSnapshotState *state =
DO_UPCAST(ExternalSnapshotState, common, common);
- 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 = 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);
}
typedef struct DriveBackupState {
--
2.14.3
- [Qemu-devel] [PULL 00/23] Block patches, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 01/23] coroutine: simplify co_aio_sleep_ns() prototype, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 03/23] hw/block: Fix the return type, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 02/23] hw/block/nvme: Convert to realize, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 04/23] hw/block: Use errp directly rather than local_err, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 05/23] dev-storage: Fix the unusual function name, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 07/23] blockdev: hold AioContext for bdrv_unref() in external_snapshot_clean(), Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 06/23] qdev: drop unused #include "sysemu/iothread.h", Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 09/23] block: don't keep AioContext acquired after drive_backup_prepare(), Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 08/23] block: don't keep AioContext acquired after external_snapshot_prepare(),
Stefan Hajnoczi <=
- [Qemu-devel] [PULL 10/23] block: don't keep AioContext acquired after blockdev_backup_prepare(), Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 11/23] block: don't keep AioContext acquired after internal_snapshot_prepare(), Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 12/23] block: drop unused BlockDirtyBitmapState->aio_context field, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 13/23] iothread: add iothread_by_id() API, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 14/23] blockdev: add x-blockdev-set-iothread testing command, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 15/23] qemu-iotests: add 202 external snapshots IOThread test, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 16/23] virtio-blk: make queue size configurable, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 17/23] virtio-blk: reject configs with logical block size > physical block size, Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 18/23] block: avoid recursive AioContext acquire in bdrv_inactivate_all(), Stefan Hajnoczi, 2017/12/18
- [Qemu-devel] [PULL 19/23] docs: mark nested AioContext locking as a legacy API, Stefan Hajnoczi, 2017/12/18