[PATCH v5 0/4] blockdev: avoid acquiring AioContext lock twice at do_drive_backup and do_blockdev_backup

[PATCH v5 0/4] blockdev: avoid acquiring AioContext lock twice at do_drive_backup and do_blockdev_backup

[Sergio Lopez]

do_drive_backup() acquires the AioContext lock of the corresponding
BlockDriverState. This is not a problem when it's called from
qmp_drive_backup(), but drive_backup_prepare() also acquires the lock
before calling it. The same things happens with do_blockdev_backup()
and blockdev_backup_prepare().

This patch series merges do_drive_backup() with drive_backup_prepare()
and do_blockdev_backup() with blockdev_backup_prepare(), and ensures
they're only getting called from a transaction context. This way,
there's a single code path for both transaction requests and qmp
commands, as suggested by Kevin Wolf.

We also take this opportunity to ensure we're honoring the context
acquisition semantics required by bdrv_try_set_aio_context, as
suggested by Max Reitz.

---
Changelog:

v5:
 - Include fix for iotest 141 in patch 2. (thanks Max Reitz)
 - Also fix iotest 185 and 219 in patch 2. (thanks Max Reitz)
 - Move error block after context acquisition/release, to we can use
   goto to bail out and release resources. (thanks Max Reitz)
 - Properly release old_context in qmp_blockdev_mirror. (thanks Max
   Reitz)

v4:
 - Unify patches 1-4 and 5-7 to avoid producing broken interim
   states. (thanks Max Reitz)
 - Include a fix for iotest 141. (thanks Kevin Wolf)

v3:
 - Rework the whole patch series to fix the issue by consolidating all
   operations in the transaction model. (thanks Kevin Wolf)

v2:
 - Honor bdrv_try_set_aio_context() context acquisition requirements
   (thanks Max Reitz).
 - Release the context at drive_backup_prepare() instead of avoiding
   re-acquiring it at do_drive_baclup(). (thanks Max Reitz)
 - Convert a single patch into a two-patch series.
---

Sergio Lopez (4):
  blockdev: fix coding style issues in drive_backup_prepare
  blockdev: unify qmp_drive_backup and drive-backup transaction paths
  blockdev: unify qmp_blockdev_backup and blockdev-backup transaction
    paths
  blockdev: honor bdrv_try_set_aio_context() context requirements

 blockdev.c                 | 354 ++++++++++++++++++-------------------
 tests/qemu-iotests/141.out |   2 +
 tests/qemu-iotests/185.out |   2 +
 tests/qemu-iotests/219     |   7 +-
 tests/qemu-iotests/219.out |   8 +
 5 files changed, 192 insertions(+), 181 deletions(-)

-- 
2.23.0

[PATCH v5 1/4] blockdev: fix coding style issues in drive_backup_prepare

[Sergio Lopez]

Fix a couple of minor coding style issues in drive_backup_prepare.

Signed-off-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..553e315972 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3620,7 +3620,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
 
     if (!backup->has_format) {
         backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
-                         NULL : (char*) bs->drv->format_name;
+                         NULL : (char *) bs->drv->format_name;
     }
 
     /* Early check to avoid creating target */
@@ -3630,8 +3630,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
 
     flags = bs->open_flags | BDRV_O_RDWR;
 
-    /* See if we have a backing HD we can use to create our new image
-     * on top of. */
+    /*
+     * See if we have a backing HD we can use to create our new image
+     * on top of.
+     */
     if (backup->sync == MIRROR_SYNC_MODE_TOP) {
         source = backing_bs(bs);
         if (!source) {
-- 
2.23.0

[PATCH v5 2/4] blockdev: unify qmp_drive_backup and drive-backup transaction paths

[Sergio Lopez]

Issuing a drive-backup from qmp_drive_backup takes a slightly
different path than when it's issued from a transaction. In the code,
this is manifested as some redundancy between do_drive_backup() and
drive_backup_prepare().

This change unifies both paths, merging do_drive_backup() and
drive_backup_prepare(), and changing qmp_drive_backup() to create a
transaction instead of calling do_backup_common() direcly.

As a side-effect, now qmp_drive_backup() is executed inside a drained
section, as it happens when creating a drive-backup transaction. This
change is visible from the user's perspective, as the job gets paused
and immediately resumed before starting the actual work.

Also fix tests 141, 185 and 219 to cope with the extra
JOB_STATUS_CHANGE lines.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 blockdev.c                 | 224 +++++++++++++++++--------------------
 tests/qemu-iotests/141.out |   2 +
 tests/qemu-iotests/185.out |   2 +
 tests/qemu-iotests/219     |   7 +-
 tests/qemu-iotests/219.out |   8 ++
 5 files changed, 117 insertions(+), 126 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 553e315972..5e85fc042e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1761,39 +1761,128 @@ typedef struct DriveBackupState {
     BlockJob *job;
 } DriveBackupState;
 
-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
-                            Error **errp);
+static BlockJob *do_backup_common(BackupCommon *backup,
+                                  BlockDriverState *bs,
+                                  BlockDriverState *target_bs,
+                                  AioContext *aio_context,
+                                  JobTxn *txn, Error **errp);
 
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
-    BlockDriverState *bs;
     DriveBackup *backup;
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
+    BlockDriverState *source = NULL;
     AioContext *aio_context;
+    QDict *options;
     Error *local_err = NULL;
+    int flags;
+    int64_t size;
+    bool set_backing_hd = false;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
     backup = common->action->u.drive_backup.data;
 
+    if (!backup->has_mode) {
+        backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    }
+
     bs = bdrv_lookup_bs(backup->device, backup->device, errp);
     if (!bs) {
         return;
     }
 
+    if (!bs->drv) {
+        error_setg(errp, "Device has no medium");
+        return;
+    }
+
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
 
     /* Paired with .clean() */
     bdrv_drained_begin(bs);
 
-    state->bs = bs;
+    if (!backup->has_format) {
+        backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
+                         NULL : (char *) bs->drv->format_name;
+    }
+
+    /* Early check to avoid creating target */
+    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
+        goto out;
+    }
+
+    flags = bs->open_flags | BDRV_O_RDWR;
+
+    /*
+     * See if we have a backing HD we can use to create our new image
+     * on top of.
+     */
+    if (backup->sync == MIRROR_SYNC_MODE_TOP) {
+        source = backing_bs(bs);
+        if (!source) {
+            backup->sync = MIRROR_SYNC_MODE_FULL;
+        }
+    }
+    if (backup->sync == MIRROR_SYNC_MODE_NONE) {
+        source = bs;
+        flags |= BDRV_O_NO_BACKING;
+        set_backing_hd = true;
+    }
+
+    size = bdrv_getlength(bs);
+    if (size < 0) {
+        error_setg_errno(errp, -size, "bdrv_getlength failed");
+        goto out;
+    }
+
+    if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
+        assert(backup->format);
+        if (source) {
+            bdrv_refresh_filename(source);
+            bdrv_img_create(backup->target, backup->format, source->filename,
+                            source->drv->format_name, NULL,
+                            size, flags, false, &local_err);
+        } else {
+            bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,
+                            size, flags, false, &local_err);
+        }
+    }
 
-    state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
     }
 
+    options = qdict_new();
+    qdict_put_str(options, "discard", "unmap");
+    qdict_put_str(options, "detect-zeroes", "unmap");
+    if (backup->format) {
+        qdict_put_str(options, "driver", backup->format);
+    }
+
+    target_bs = bdrv_open(backup->target, NULL, options, flags, errp);
+    if (!target_bs) {
+        goto out;
+    }
+
+    if (set_backing_hd) {
+        bdrv_set_backing_hd(target_bs, source, &local_err);
+        if (local_err) {
+            goto unref;
+        }
+    }
+
+    state->bs = bs;
+
+    state->job = do_backup_common(qapi_DriveBackup_base(backup),
+                                  bs, target_bs, aio_context,
+                                  common->block_job_txn, errp);
+
+unref:
+    bdrv_unref(target_bs);
 out:
     aio_context_release(aio_context);
 }
@@ -3587,126 +3676,13 @@ static BlockJob *do_backup_common(BackupCommon *backup,
     return job;
 }
 
-static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
-                                 Error **errp)
-{
-    BlockDriverState *bs;
-    BlockDriverState *target_bs;
-    BlockDriverState *source = NULL;
-    BlockJob *job = NULL;
-    AioContext *aio_context;
-    QDict *options;
-    Error *local_err = NULL;
-    int flags;
-    int64_t size;
-    bool set_backing_hd = false;
-
-    if (!backup->has_mode) {
-        backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    }
-
-    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
-    if (!bs) {
-        return NULL;
-    }
-
-    if (!bs->drv) {
-        error_setg(errp, "Device has no medium");
-        return NULL;
-    }
-
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
-
-    if (!backup->has_format) {
-        backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
-                         NULL : (char *) bs->drv->format_name;
-    }
-
-    /* Early check to avoid creating target */
-    if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
-        goto out;
-    }
-
-    flags = bs->open_flags | BDRV_O_RDWR;
-
-    /*
-     * See if we have a backing HD we can use to create our new image
-     * on top of.
-     */
-    if (backup->sync == MIRROR_SYNC_MODE_TOP) {
-        source = backing_bs(bs);
-        if (!source) {
-            backup->sync = MIRROR_SYNC_MODE_FULL;
-        }
-    }
-    if (backup->sync == MIRROR_SYNC_MODE_NONE) {
-        source = bs;
-        flags |= BDRV_O_NO_BACKING;
-        set_backing_hd = true;
-    }
-
-    size = bdrv_getlength(bs);
-    if (size < 0) {
-        error_setg_errno(errp, -size, "bdrv_getlength failed");
-        goto out;
-    }
-
-    if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
-        assert(backup->format);
-        if (source) {
-            bdrv_refresh_filename(source);
-            bdrv_img_create(backup->target, backup->format, source->filename,
-                            source->drv->format_name, NULL,
-                            size, flags, false, &local_err);
-        } else {
-            bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,
-                            size, flags, false, &local_err);
-        }
-    }
-
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto out;
-    }
-
-    options = qdict_new();
-    qdict_put_str(options, "discard", "unmap");
-    qdict_put_str(options, "detect-zeroes", "unmap");
-    if (backup->format) {
-        qdict_put_str(options, "driver", backup->format);
-    }
-
-    target_bs = bdrv_open(backup->target, NULL, options, flags, errp);
-    if (!target_bs) {
-        goto out;
-    }
-
-    if (set_backing_hd) {
-        bdrv_set_backing_hd(target_bs, source, &local_err);
-        if (local_err) {
-            goto unref;
-        }
-    }
-
-    job = do_backup_common(qapi_DriveBackup_base(backup),
-                           bs, target_bs, aio_context, txn, errp);
-
-unref:
-    bdrv_unref(target_bs);
-out:
-    aio_context_release(aio_context);
-    return job;
-}
-
-void qmp_drive_backup(DriveBackup *arg, Error **errp)
+void qmp_drive_backup(DriveBackup *backup, Error **errp)
 {
-
-    BlockJob *job;
-    job = do_drive_backup(arg, NULL, errp);
-    if (job) {
-        job_start(&job->job);
-    }
+    TransactionAction action = {
+        .type = TRANSACTION_ACTION_KIND_DRIVE_BACKUP,
+        .u.drive_backup.data = backup,
+    };
+    blockdev_do_action(&action, errp);
 }
 
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index 3645675ce8..263b680bdf 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -13,6 +13,8 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/m.
 Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "paused", "id": "job0"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "job0"}}
 {'execute': 'blockdev-del', 'arguments': {'node-name': 'drv0'}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: node is used as backing hd of 'NODE_NAME'"}}
 {'execute': 'block-job-cancel', 'arguments': {'device': 'job0'}}
diff --git a/tests/qemu-iotests/185.out b/tests/qemu-iotests/185.out
index 8379ac5854..9a3b65782b 100644
--- a/tests/qemu-iotests/185.out
+++ b/tests/qemu-iotests/185.out
@@ -65,6 +65,8 @@ Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 l
 Formatting 'TEST_DIR/t.qcow2.copy', fmt=qcow2 size=67108864 cluster_size=65536 lazy_refcounts=off refcount_bits=16
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "disk"}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "paused", "id": "disk"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "disk"}}
 {"return": {}}
 { 'execute': 'quit' }
 {"return": {}}
diff --git a/tests/qemu-iotests/219 b/tests/qemu-iotests/219
index e0c51662c0..655f54d881 100755
--- a/tests/qemu-iotests/219
+++ b/tests/qemu-iotests/219
@@ -63,7 +63,7 @@ def test_pause_resume(vm):
                 # logged immediately
                 iotests.log(vm.qmp('query-jobs'))
 
-def test_job_lifecycle(vm, job, job_args, has_ready=False):
+def test_job_lifecycle(vm, job, job_args, has_ready=False, is_mirror=False):
     global img_size
 
     iotests.log('')
@@ -135,6 +135,9 @@ def test_job_lifecycle(vm, job, job_args, has_ready=False):
     iotests.log('Waiting for PENDING state...')
     iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
     iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
+    if is_mirror:
+        iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
+        iotests.log(iotests.filter_qmp_event(vm.event_wait('JOB_STATUS_CHANGE')))
 
     if not job_args.get('auto-finalize', True):
         # PENDING state:
@@ -218,7 +221,7 @@ with iotests.FilePath('disk.img') as disk_path, \
 
     for auto_finalize in [True, False]:
         for auto_dismiss in [True, False]:
-            test_job_lifecycle(vm, 'drive-backup', job_args={
+            test_job_lifecycle(vm, 'drive-backup', is_mirror=True, job_args={
                 'device': 'drive0-node',
                 'target': copy_path,
                 'sync': 'full',
diff --git a/tests/qemu-iotests/219.out b/tests/qemu-iotests/219.out
index 8ebd3fee60..0ea5d0b9d5 100644
--- a/tests/qemu-iotests/219.out
+++ b/tests/qemu-iotests/219.out
@@ -135,6 +135,8 @@ Pause/resume in RUNNING
 {"return": {}}
 
 Waiting for PENDING state...
+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "concluded"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
@@ -186,6 +188,8 @@ Pause/resume in RUNNING
 {"return": {}}
 
 Waiting for PENDING state...
+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "concluded"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
@@ -245,6 +249,8 @@ Pause/resume in RUNNING
 {"return": {}}
 
 Waiting for PENDING state...
+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"return": [{"current-progress": 4194304, "id": "job0", "status": "pending", "total-progress": 4194304, "type": "backup"}]}
@@ -304,6 +310,8 @@ Pause/resume in RUNNING
 {"return": {}}
 
 Waiting for PENDING state...
+{"data": {"id": "job0", "status": "paused"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"id": "job0", "status": "running"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "waiting"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"data": {"id": "job0", "status": "pending"}, "event": "JOB_STATUS_CHANGE", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
 {"return": [{"current-progress": 4194304, "id": "job0", "status": "pending", "total-progress": 4194304, "type": "backup"}]}
-- 
2.23.0

[PATCH v5 3/4] blockdev: unify qmp_blockdev_backup and blockdev-backup transaction paths

[Sergio Lopez]

Issuing a blockdev-backup from qmp_blockdev_backup takes a slightly
different path than when it's issued from a transaction. In the code,
this is manifested as some redundancy between do_blockdev_backup() and
blockdev_backup_prepare().

This change unifies both paths, merging do_blockdev_backup() and
blockdev_backup_prepare(), and changing qmp_blockdev_backup() to
create a transaction instead of calling do_backup_common() direcly.

As a side-effect, now qmp_blockdev_backup() is executed inside a
drained section, as it happens when creating a blockdev-backup
transaction. This change is visible from the user's perspective, as
the job gets paused and immediately resumed before starting the actual
work.

Signed-off-by: Sergio Lopez <slp@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 60 ++++++++++++------------------------------------------
 1 file changed, 13 insertions(+), 47 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5e85fc042e..152a0f7454 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,16 +1940,13 @@ typedef struct BlockdevBackupState {
     BlockJob *job;
 } BlockdevBackupState;
 
-static BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
-                                    Error **errp);
-
 static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;
-    BlockDriverState *bs, *target;
+    BlockDriverState *bs;
+    BlockDriverState *target_bs;
     AioContext *aio_context;
-    Error *local_err = NULL;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
     backup = common->action->u.blockdev_backup.data;
@@ -1959,8 +1956,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
-    target = bdrv_lookup_bs(backup->target, backup->target, errp);
-    if (!target) {
+    target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
+    if (!target_bs) {
         return;
     }
 
@@ -1971,13 +1968,10 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     /* Paired with .clean() */
     bdrv_drained_begin(state->bs);
 
-    state->job = do_blockdev_backup(backup, common->block_job_txn, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        goto out;
-    }
+    state->job = do_backup_common(qapi_BlockdevBackup_base(backup),
+                                  bs, target_bs, aio_context,
+                                  common->block_job_txn, errp);
 
-out:
     aio_context_release(aio_context);
 }
 
@@ -3695,41 +3689,13 @@ XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp)
     return bdrv_get_xdbg_block_graph(errp);
 }
 
-BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
-                             Error **errp)
+void qmp_blockdev_backup(BlockdevBackup *backup, Error **errp)
 {
-    BlockDriverState *bs;
-    BlockDriverState *target_bs;
-    AioContext *aio_context;
-    BlockJob *job;
-
-    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
-    if (!bs) {
-        return NULL;
-    }
-
-    target_bs = bdrv_lookup_bs(backup->target, backup->target, errp);
-    if (!target_bs) {
-        return NULL;
-    }
-
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
-
-    job = do_backup_common(qapi_BlockdevBackup_base(backup),
-                           bs, target_bs, aio_context, txn, errp);
-
-    aio_context_release(aio_context);
-    return job;
-}
-
-void qmp_blockdev_backup(BlockdevBackup *arg, Error **errp)
-{
-    BlockJob *job;
-    job = do_blockdev_backup(arg, NULL, errp);
-    if (job) {
-        job_start(&job->job);
-    }
+    TransactionAction action = {
+        .type = TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP,
+        .u.blockdev_backup.data = backup,
+    };
+    blockdev_do_action(&action, errp);
 }
 
 /* Parameter check and block job starting for drive mirroring.
-- 
2.23.0

[PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Sergio Lopez]

bdrv_try_set_aio_context() requires that the old context is held, and
the new context is not held. Fix all the occurrences where it's not
done this way.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 blockdev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 152a0f7454..e33abd7fd2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1535,6 +1535,7 @@ static void external_snapshot_prepare(BlkActionState *common,
                              DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
     AioContext *aio_context;
+    AioContext *old_context;
     int ret;
 
     /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
@@ -1675,7 +1676,16 @@ static void external_snapshot_prepare(BlkActionState *common,
         goto out;
     }
 
+    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    old_context = bdrv_get_aio_context(state->new_bs);
+    aio_context_release(aio_context);
+    aio_context_acquire(old_context);
+
     ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp);
+
+    aio_context_release(old_context);
+    aio_context_acquire(aio_context);
+
     if (ret < 0) {
         goto out;
     }
@@ -1775,11 +1785,13 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     BlockDriverState *target_bs;
     BlockDriverState *source = NULL;
     AioContext *aio_context;
+    AioContext *old_context;
     QDict *options;
     Error *local_err = NULL;
     int flags;
     int64_t size;
     bool set_backing_hd = false;
+    int ret;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
     backup = common->action->u.drive_backup.data;
@@ -1868,6 +1880,20 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
         goto out;
     }
 
+    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    old_context = bdrv_get_aio_context(target_bs);
+    aio_context_release(aio_context);
+    aio_context_acquire(old_context);
+
+    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+
+    aio_context_release(old_context);
+    aio_context_acquire(aio_context);
+
+    if (ret < 0) {
+        goto out;
+    }
+
     if (set_backing_hd) {
         bdrv_set_backing_hd(target_bs, source, &local_err);
         if (local_err) {
@@ -1947,6 +1973,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     AioContext *aio_context;
+    AioContext *old_context;
+    int ret;
 
     assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
     backup = common->action->u.blockdev_backup.data;
@@ -1961,7 +1989,18 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
         return;
     }
 
+    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
     aio_context = bdrv_get_aio_context(bs);
+    old_context = bdrv_get_aio_context(target_bs);
+    aio_context_acquire(old_context);
+
+    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+    if (ret < 0) {
+        aio_context_release(old_context);
+        return;
+    }
+
+    aio_context_release(old_context);
     aio_context_acquire(aio_context);
     state->bs = bs;
 
@@ -3562,7 +3601,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
     BlockJob *job = NULL;
     BdrvDirtyBitmap *bmap = NULL;
     int job_flags = JOB_DEFAULT;
-    int ret;
 
     if (!backup->has_speed) {
         backup->speed = 0;
@@ -3586,11 +3624,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
         backup->compress = false;
     }
 
-    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
-    if (ret < 0) {
-        return NULL;
-    }
-
     if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
         (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) {
         /* done before desugaring 'incremental' to print the right message */
@@ -3825,6 +3858,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
     BlockDriverState *bs;
     BlockDriverState *source, *target_bs;
     AioContext *aio_context;
+    AioContext *old_context;
     BlockMirrorBackingMode backing_mode;
     Error *local_err = NULL;
     QDict *options = NULL;
@@ -3937,10 +3971,19 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
                    (arg->mode == NEW_IMAGE_MODE_EXISTING ||
                     !bdrv_has_zero_init(target_bs)));
 
+
+    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    old_context = bdrv_get_aio_context(target_bs);
+    aio_context_release(aio_context);
+    aio_context_acquire(old_context);
+
     ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+
+    aio_context_release(old_context);
+    aio_context_acquire(aio_context);
+
     if (ret < 0) {
-        bdrv_unref(target_bs);
-        goto out;
+        goto unref;
     }
 
     blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
@@ -3957,8 +4000,10 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
                            arg->has_auto_finalize, arg->auto_finalize,
                            arg->has_auto_dismiss, arg->auto_dismiss,
                            &local_err);
-    bdrv_unref(target_bs);
     error_propagate(errp, local_err);
+
+unref:
+    bdrv_unref(target_bs);
 out:
     aio_context_release(aio_context);
 }
@@ -3984,6 +4029,7 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     AioContext *aio_context;
+    AioContext *old_context;
     BlockMirrorBackingMode backing_mode = MIRROR_LEAVE_BACKING_CHAIN;
     Error *local_err = NULL;
     bool zero_target;
@@ -4001,10 +4047,16 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
 
     zero_target = (sync == MIRROR_SYNC_MODE_FULL);
 
+    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
+    old_context = bdrv_get_aio_context(target_bs);
     aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
+    aio_context_acquire(old_context);
 
     ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
+
+    aio_context_release(old_context);
+    aio_context_acquire(aio_context);
+
     if (ret < 0) {
         goto out;
     }
-- 
2.23.0

Re: [PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Kevin Wolf]

Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
> bdrv_try_set_aio_context() requires that the old context is held, and
> the new context is not held. Fix all the occurrences where it's not
> done this way.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  blockdev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 152a0f7454..e33abd7fd2 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1535,6 +1535,7 @@ static void external_snapshot_prepare(BlkActionState *common,
>                               DO_UPCAST(ExternalSnapshotState, common, common);
>      TransactionAction *action = common->action;
>      AioContext *aio_context;
> +    AioContext *old_context;
>      int ret;
>  
>      /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
> @@ -1675,7 +1676,16 @@ static void external_snapshot_prepare(BlkActionState *common,
>          goto out;
>      }
>  
> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
> +    old_context = bdrv_get_aio_context(state->new_bs);
> +    aio_context_release(aio_context);
> +    aio_context_acquire(old_context);
> +
>      ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp);
> +
> +    aio_context_release(old_context);
> +    aio_context_acquire(aio_context);
> +
>      if (ret < 0) {
>          goto out;
>      }
> @@ -1775,11 +1785,13 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>      BlockDriverState *target_bs;
>      BlockDriverState *source = NULL;
>      AioContext *aio_context;
> +    AioContext *old_context;
>      QDict *options;
>      Error *local_err = NULL;
>      int flags;
>      int64_t size;
>      bool set_backing_hd = false;
> +    int ret;
>  
>      assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
>      backup = common->action->u.drive_backup.data;
> @@ -1868,6 +1880,20 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>          goto out;
>      }
>  
> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
> +    old_context = bdrv_get_aio_context(target_bs);
> +    aio_context_release(aio_context);
> +    aio_context_acquire(old_context);
> +
> +    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
> +    aio_context_release(old_context);
> +    aio_context_acquire(aio_context);
> +
> +    if (ret < 0) {
> +        goto out;

I think this needs to be 'goto unref'.

Or in fact, I think you need to hold the AioContext of a bs to
bdrv_unref() it, so maybe 'goto out' is right, but you need to unref
target_bs while you still hold old_context.

> +    }
> +
>      if (set_backing_hd) {
>          bdrv_set_backing_hd(target_bs, source, &local_err);
>          if (local_err) {
> @@ -1947,6 +1973,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>      BlockDriverState *bs;
>      BlockDriverState *target_bs;
>      AioContext *aio_context;
> +    AioContext *old_context;
> +    int ret;
>  
>      assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
>      backup = common->action->u.blockdev_backup.data;
> @@ -1961,7 +1989,18 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>          return;
>      }
>  
> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
>      aio_context = bdrv_get_aio_context(bs);
> +    old_context = bdrv_get_aio_context(target_bs);
> +    aio_context_acquire(old_context);
> +
> +    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
> +    if (ret < 0) {
> +        aio_context_release(old_context);
> +        return;
> +    }
> +
> +    aio_context_release(old_context);
>      aio_context_acquire(aio_context);
>      state->bs = bs;
>  
> @@ -3562,7 +3601,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>      BlockJob *job = NULL;
>      BdrvDirtyBitmap *bmap = NULL;
>      int job_flags = JOB_DEFAULT;
> -    int ret;
>  
>      if (!backup->has_speed) {
>          backup->speed = 0;
> @@ -3586,11 +3624,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>          backup->compress = false;
>      }
>  
> -    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
> -    if (ret < 0) {
> -        return NULL;
> -    }
> -
>      if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
>          (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) {
>          /* done before desugaring 'incremental' to print the right message */
> @@ -3825,6 +3858,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>      BlockDriverState *bs;
>      BlockDriverState *source, *target_bs;
>      AioContext *aio_context;
> +    AioContext *old_context;
>      BlockMirrorBackingMode backing_mode;
>      Error *local_err = NULL;
>      QDict *options = NULL;
> @@ -3937,10 +3971,19 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>                     (arg->mode == NEW_IMAGE_MODE_EXISTING ||
>                      !bdrv_has_zero_init(target_bs)));
>  
> +
> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
> +    old_context = bdrv_get_aio_context(target_bs);
> +    aio_context_release(aio_context);
> +    aio_context_acquire(old_context);
> +
>      ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
> +
> +    aio_context_release(old_context);
> +    aio_context_acquire(aio_context);
> +
>      if (ret < 0) {
> -        bdrv_unref(target_bs);
> -        goto out;
> +        goto unref;
>      }

Here you don't forget to unref target_bs, but it has still the same
problem as above that you need to hold old_context during bdrv_unref().

Kevin

Re: [PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Eric Blake]

On 12/9/19 10:06 AM, Kevin Wolf wrote:
> Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
>> bdrv_try_set_aio_context() requires that the old context is held, and
>> the new context is not held. Fix all the occurrences where it's not
>> done this way.
>>
>> Suggested-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> ---

> Or in fact, I think you need to hold the AioContext of a bs to
> bdrv_unref() it, so maybe 'goto out' is right, but you need to unref
> target_bs while you still hold old_context.

I suspect https://bugzilla.redhat.com/show_bug.cgi?id=1779036 is also a 
symptom of this.  The v5 patch did not fix this simple test case:


$ qemu-img create -f qcow2 f1 100m
$ qemu-img create -f qcow2 f2 100m
$ ./qemu-kvm -nodefaults -nographic -qmp stdio -object iothread,id=io0 \
  -drive driver=qcow2,id=drive1,file=f1,if=none -device 
virtio-scsi-pci,id=scsi0,iothread=io0 -device 
scsi-hd,id=image1,drive=drive1 \
  -drive driver=qcow2,id=drive2,file=f2,if=none -device 
virtio-blk-pci,id=image2,drive=drive2,iothread=io0

{'execute':'qmp_capabilities'}

{'execute':'transaction','arguments':{'actions':[ 
{'type':'blockdev-snapshot-sync','data':{'device':'drive1', 
'snapshot-file':'sn1','mode':'absolute-paths','format':'qcow2'}}, 
{'type':'blockdev-snapshot-sync','data':{'device':'drive2', 
'snapshot-file':'/aa/sn1','mode':'absolute-paths','format':'qcow2'}}]}}

which is an aio context bug somewhere on the error path of 
blockdev-snapshot-sync (the first one has to be rolled back because the 
second part of the transaction fails early on a nonexistent directory)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Kevin Wolf]

Am 13.12.2019 um 21:59 hat Eric Blake geschrieben:
> On 12/9/19 10:06 AM, Kevin Wolf wrote:
> > Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
> > > bdrv_try_set_aio_context() requires that the old context is held, and
> > > the new context is not held. Fix all the occurrences where it's not
> > > done this way.
> > > 
> > > Suggested-by: Max Reitz <mreitz@redhat.com>
> > > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > > ---
> 
> > Or in fact, I think you need to hold the AioContext of a bs to
> > bdrv_unref() it, so maybe 'goto out' is right, but you need to unref
> > target_bs while you still hold old_context.
> 
> I suspect https://bugzilla.redhat.com/show_bug.cgi?id=1779036 is also a
> symptom of this.  The v5 patch did not fix this simple test case:

Speaking of a test case... I think this series should probably add
something to iotests.

Kevin

Re: [PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Sergio Lopez]

[-- Attachment #1: Type: text/plain, Size: 1002 bytes --]


Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.12.2019 um 21:59 hat Eric Blake geschrieben:
>> On 12/9/19 10:06 AM, Kevin Wolf wrote:
>> > Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
>> > > bdrv_try_set_aio_context() requires that the old context is held, and
>> > > the new context is not held. Fix all the occurrences where it's not
>> > > done this way.
>> > > 
>> > > Suggested-by: Max Reitz <mreitz@redhat.com>
>> > > Signed-off-by: Sergio Lopez <slp@redhat.com>
>> > > ---
>> 
>> > Or in fact, I think you need to hold the AioContext of a bs to
>> > bdrv_unref() it, so maybe 'goto out' is right, but you need to unref
>> > target_bs while you still hold old_context.
>> 
>> I suspect https://bugzilla.redhat.com/show_bug.cgi?id=1779036 is also a
>> symptom of this.  The v5 patch did not fix this simple test case:
>
> Speaking of a test case... I think this series should probably add
> something to iotests.

Okay, I'll try to add one.

Thanks,
Sergio.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Sergio Lopez]

[-- Attachment #1: Type: text/plain, Size: 4833 bytes --]


Eric Blake <eblake@redhat.com> writes:

> On 12/9/19 10:06 AM, Kevin Wolf wrote:
>> Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
>>> bdrv_try_set_aio_context() requires that the old context is held, and
>>> the new context is not held. Fix all the occurrences where it's not
>>> done this way.
>>>
>>> Suggested-by: Max Reitz <mreitz@redhat.com>
>>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>>> ---
>
>> Or in fact, I think you need to hold the AioContext of a bs to
>> bdrv_unref() it, so maybe 'goto out' is right, but you need to unref
>> target_bs while you still hold old_context.
>
> I suspect https://bugzilla.redhat.com/show_bug.cgi?id=1779036 is also
> a symptom of this.  The v5 patch did not fix this simple test case:
>
>
> $ qemu-img create -f qcow2 f1 100m
> $ qemu-img create -f qcow2 f2 100m
> $ ./qemu-kvm -nodefaults -nographic -qmp stdio -object iothread,id=io0 \
>  -drive driver=qcow2,id=drive1,file=f1,if=none -device
> virtio-scsi-pci,id=scsi0,iothread=io0 -device
> scsi-hd,id=image1,drive=drive1 \
>  -drive driver=qcow2,id=drive2,file=f2,if=none -device
> virtio-blk-pci,id=image2,drive=drive2,iothread=io0
>
> {'execute':'qmp_capabilities'}
>
> {'execute':'transaction','arguments':{'actions':[
> {'type':'blockdev-snapshot-sync','data':{'device':'drive1',
> 'snapshot-file':'sn1','mode':'absolute-paths','format':'qcow2'}},
> {'type':'blockdev-snapshot-sync','data':{'device':'drive2',
> 'snapshot-file':'/aa/sn1','mode':'absolute-paths','format':'qcow2'}}]}}
>
> which is an aio context bug somewhere on the error path of
> blockdev-snapshot-sync (the first one has to be rolled back because
> the second part of the transaction fails early on a nonexistent
> directory)

This is slightly different. The problem resides in
external_snapshot_abort():

   1717 static void external_snapshot_abort(BlkActionState *common)
   1718 {
   1719     ExternalSnapshotState *state =
   1720                              DO_UPCAST(ExternalSnapshotState, common, common);
   1721     if (state->new_bs) {
   1722         if (state->overlay_appended) {
   1723             AioContext *aio_context;
   1724 
   1725             aio_context = bdrv_get_aio_context(state->old_bs);
   1726             aio_context_acquire(aio_context);
   1727 
   1728             bdrv_ref(state->old_bs);   /* we can't let bdrv_set_backind_hd()
   1729                                           close state->old_bs; we need it */
   1730             bdrv_set_backing_hd(state->new_bs, NULL, &error_abort);
   1731             bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
   1732             bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */
   1733 
   1734             aio_context_release(aio_context);
   1735         }
   1736     }
   1737 }

bdrv_set_backing_hd() returns state->old_bs to the main AioContext,
while bdrv_replace_node() expects state->new_bs and state->old_bs to be
using the same AioContext.

I'm thinking sending this as a separate patch:

diff --git a/blockdev.c b/blockdev.c
index e33abd7fd2..6c73ac4e32 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1731,6 +1731,8 @@ static void external_snapshot_abort(BlkActionState *common)
     if (state->new_bs) {
         if (state->overlay_appended) {
             AioContext *aio_context;
+            AioContext *tmp_context;
+            int ret;
 
             aio_context = bdrv_get_aio_context(state->old_bs);
             aio_context_acquire(aio_context);
@@ -1738,6 +1740,25 @@ static void external_snapshot_abort(BlkActionState *common)
             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);
+
+            /*
+             * The call to bdrv_set_backing_hd() above returns state->old_bs to
+             * the main AioContext. As we're still going to be using it, return
+             * it to the AioContext it was before.
+             */
+            tmp_context = bdrv_get_aio_context(state->old_bs);
+            if (aio_context != tmp_context) {
+                aio_context_release(aio_context);
+                aio_context_acquire(tmp_context);
+
+                ret = bdrv_try_set_aio_context(state->old_bs,
+                                               aio_context, NULL);
+                assert(ret == 0);
+
+                aio_context_release(tmp_context);
+                aio_context_acquire(aio_context);
+            }
+
             bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
             bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */

What do you think?

Sergio.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 4/4] blockdev: honor bdrv_try_set_aio_context() context requirements

[Sergio Lopez]

[-- Attachment #1: Type: text/plain, Size: 6425 bytes --]


Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
>> bdrv_try_set_aio_context() requires that the old context is held, and
>> the new context is not held. Fix all the occurrences where it's not
>> done this way.
>> 
>> Suggested-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> ---
>>  blockdev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 62 insertions(+), 10 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index 152a0f7454..e33abd7fd2 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1535,6 +1535,7 @@ static void external_snapshot_prepare(BlkActionState *common,
>>                               DO_UPCAST(ExternalSnapshotState, common, common);
>>      TransactionAction *action = common->action;
>>      AioContext *aio_context;
>> +    AioContext *old_context;
>>      int ret;
>>  
>>      /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar
>> @@ -1675,7 +1676,16 @@ static void external_snapshot_prepare(BlkActionState *common,
>>          goto out;
>>      }
>>  
>> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
>> +    old_context = bdrv_get_aio_context(state->new_bs);
>> +    aio_context_release(aio_context);
>> +    aio_context_acquire(old_context);
>> +
>>      ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp);
>> +
>> +    aio_context_release(old_context);
>> +    aio_context_acquire(aio_context);
>> +
>>      if (ret < 0) {
>>          goto out;
>>      }
>> @@ -1775,11 +1785,13 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>>      BlockDriverState *target_bs;
>>      BlockDriverState *source = NULL;
>>      AioContext *aio_context;
>> +    AioContext *old_context;
>>      QDict *options;
>>      Error *local_err = NULL;
>>      int flags;
>>      int64_t size;
>>      bool set_backing_hd = false;
>> +    int ret;
>>  
>>      assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
>>      backup = common->action->u.drive_backup.data;
>> @@ -1868,6 +1880,20 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>>          goto out;
>>      }
>>  
>> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
>> +    old_context = bdrv_get_aio_context(target_bs);
>> +    aio_context_release(aio_context);
>> +    aio_context_acquire(old_context);
>> +
>> +    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>> +    aio_context_release(old_context);
>> +    aio_context_acquire(aio_context);
>> +
>> +    if (ret < 0) {
>> +        goto out;
>
> I think this needs to be 'goto unref'.
>
> Or in fact, I think you need to hold the AioContext of a bs to
> bdrv_unref() it, so maybe 'goto out' is right, but you need to unref
> target_bs while you still hold old_context.

Thanks for catching this one. To avoid making the error bailout path
even more complicated, in v6 I'll be moving the check just after
bdrv_try_set_aio_context(), doing the unref, the release of old_context,
and a direct return.

>> +    }
>> +
>>      if (set_backing_hd) {
>>          bdrv_set_backing_hd(target_bs, source, &local_err);
>>          if (local_err) {
>> @@ -1947,6 +1973,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>>      BlockDriverState *bs;
>>      BlockDriverState *target_bs;
>>      AioContext *aio_context;
>> +    AioContext *old_context;
>> +    int ret;
>>  
>>      assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
>>      backup = common->action->u.blockdev_backup.data;
>> @@ -1961,7 +1989,18 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
>>          return;
>>      }
>>  
>> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
>>      aio_context = bdrv_get_aio_context(bs);
>> +    old_context = bdrv_get_aio_context(target_bs);
>> +    aio_context_acquire(old_context);
>> +
>> +    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>> +    if (ret < 0) {
>> +        aio_context_release(old_context);
>> +        return;
>> +    }
>> +
>> +    aio_context_release(old_context);
>>      aio_context_acquire(aio_context);
>>      state->bs = bs;
>>  
>> @@ -3562,7 +3601,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>>      BlockJob *job = NULL;
>>      BdrvDirtyBitmap *bmap = NULL;
>>      int job_flags = JOB_DEFAULT;
>> -    int ret;
>>  
>>      if (!backup->has_speed) {
>>          backup->speed = 0;
>> @@ -3586,11 +3624,6 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>>          backup->compress = false;
>>      }
>>  
>> -    ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>> -    if (ret < 0) {
>> -        return NULL;
>> -    }
>> -
>>      if ((backup->sync == MIRROR_SYNC_MODE_BITMAP) ||
>>          (backup->sync == MIRROR_SYNC_MODE_INCREMENTAL)) {
>>          /* done before desugaring 'incremental' to print the right message */
>> @@ -3825,6 +3858,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>      BlockDriverState *bs;
>>      BlockDriverState *source, *target_bs;
>>      AioContext *aio_context;
>> +    AioContext *old_context;
>>      BlockMirrorBackingMode backing_mode;
>>      Error *local_err = NULL;
>>      QDict *options = NULL;
>> @@ -3937,10 +3971,19 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
>>                     (arg->mode == NEW_IMAGE_MODE_EXISTING ||
>>                      !bdrv_has_zero_init(target_bs)));
>>  
>> +
>> +    /* Honor bdrv_try_set_aio_context() context acquisition requirements. */
>> +    old_context = bdrv_get_aio_context(target_bs);
>> +    aio_context_release(aio_context);
>> +    aio_context_acquire(old_context);
>> +
>>      ret = bdrv_try_set_aio_context(target_bs, aio_context, errp);
>> +
>> +    aio_context_release(old_context);
>> +    aio_context_acquire(aio_context);
>> +
>>      if (ret < 0) {
>> -        bdrv_unref(target_bs);
>> -        goto out;
>> +        goto unref;
>>      }
>
> Here you don't forget to unref target_bs, but it has still the same
> problem as above that you need to hold old_context during bdrv_unref().
>
> Kevin


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 0/4] blockdev: avoid acquiring AioContext lock twice at do_drive_backup and do_blockdev_backup

[Kevin Wolf]

Am 28.11.2019 um 11:41 hat Sergio Lopez geschrieben:
> do_drive_backup() acquires the AioContext lock of the corresponding
> BlockDriverState. This is not a problem when it's called from
> qmp_drive_backup(), but drive_backup_prepare() also acquires the lock
> before calling it. The same things happens with do_blockdev_backup()
> and blockdev_backup_prepare().
> 
> This patch series merges do_drive_backup() with drive_backup_prepare()
> and do_blockdev_backup() with blockdev_backup_prepare(), and ensures
> they're only getting called from a transaction context. This way,
> there's a single code path for both transaction requests and qmp
> commands, as suggested by Kevin Wolf.
> 
> We also take this opportunity to ensure we're honoring the context
> acquisition semantics required by bdrv_try_set_aio_context, as
> suggested by Max Reitz.

I think there is a small problem with the error paths in patch 4, but
for patches 1 to 3 you can add:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>