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

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

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

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

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.

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.

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