[Qemu-devel] [PATCH v2 0/2] blockdev: avoid acquiring AioContext lock twice at do_drive_backup()

[Qemu-devel] [PATCH v2 0/2] blockdev: avoid acquiring AioContext lock twice at do_drive_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.

Additionally, Max Reitz pointed out that bdrv_try_set_aio_context() is
called at do_backup_common() with the new context held, and the old
context not held, while it expects it to be the other way
around. This is also the case for other uses of that function on
blockdev.c.

This patch series fixes all occurrences of bdrv_try_set_aio_context()
to honor the context requirements. It also changes
drive_backup_prepare() to release the context before calling
do_drive_backup().

---
Changelog

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 (2):
  blockdev: release the AioContext at drive_backup_prepare
  blockdev: honor bdrv_try_set_aio_context() context requirements

 blockdev.c | 127 ++++++++++++++++++++++++++++++++---------------------
 1 file changed, 76 insertions(+), 51 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Sergio Lopez]

do_drive_backup() already acquires the AioContext, so release it
before the call.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 blockdev.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index fbef6845c8..3927fdab80 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
-
     /* Paired with .clean() */
     bdrv_drained_begin(bs);
+    aio_context_release(aio_context);
 
     state->bs = bs;
 
     state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
     }
-
-out:
-    aio_context_release(aio_context);
 }
 
 static void drive_backup_commit(BlkActionState *common)
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/2] 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 ocurrences where it's not
done this way.

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

diff --git a/blockdev.c b/blockdev.c
index 3927fdab80..2534fef389 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1536,6 +1536,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
@@ -1575,30 +1576,30 @@ static void external_snapshot_prepare(BlkActionState *common,
 
     aio_context = bdrv_get_aio_context(state->old_bs);
     aio_context_acquire(aio_context);
-
     /* Paired with .clean() */
     bdrv_drained_begin(state->old_bs);
+    aio_context_release(aio_context);
 
     if (!bdrv_is_inserted(state->old_bs)) {
         error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        goto out;
+        return;
     }
 
     if (bdrv_op_is_blocked(state->old_bs,
                            BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) {
-        goto out;
+        return;
     }
 
     if (!bdrv_is_read_only(state->old_bs)) {
         if (bdrv_flush(state->old_bs)) {
             error_setg(errp, QERR_IO_ERROR);
-            goto out;
+            return;
         }
     }
 
     if (!bdrv_is_first_non_filter(state->old_bs)) {
         error_setg(errp, QERR_FEATURE_DISABLED, "snapshot");
-        goto out;
+        return;
     }
 
     if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) {
@@ -1610,13 +1611,13 @@ static void external_snapshot_prepare(BlkActionState *common,
 
         if (node_name && !snapshot_node_name) {
             error_setg(errp, "New overlay node name missing");
-            goto out;
+            return;
         }
 
         if (snapshot_node_name &&
             bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) {
             error_setg(errp, "New overlay node name already in use");
-            goto out;
+            return;
         }
 
         flags = state->old_bs->open_flags;
@@ -1629,7 +1630,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");
-                goto out;
+                return;
             }
             bdrv_refresh_filename(state->old_bs);
             bdrv_img_create(new_image_file, format,
@@ -1638,7 +1639,7 @@ static void external_snapshot_prepare(BlkActionState *common,
                             NULL, size, flags, false, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
-                goto out;
+                return;
             }
         }
 
@@ -1653,34 +1654,41 @@ static void external_snapshot_prepare(BlkActionState *common,
                               errp);
     /* We will manually add the backing_hd field to the bs later */
     if (!state->new_bs) {
-        goto out;
+        return;
     }
 
     if (bdrv_has_blk(state->new_bs)) {
         error_setg(errp, "The overlay is already in use");
-        goto out;
+        return;
     }
 
     if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT,
                            errp)) {
-        goto out;
+        return;
     }
 
     if (state->new_bs->backing != NULL) {
         error_setg(errp, "The overlay already has a backing image");
-        goto out;
+        return;
     }
 
     if (!state->new_bs->drv->supports_backing) {
         error_setg(errp, "The overlay does not support backing images");
-        goto out;
+        return;
     }
 
+    old_context = bdrv_get_aio_context(state->new_bs);
+    aio_context_acquire(old_context);
+
     ret = bdrv_try_set_aio_context(state->new_bs, aio_context, errp);
     if (ret < 0) {
-        goto out;
+        aio_context_release(old_context);
+        return;
     }
 
+    aio_context_release(old_context);
+    aio_context_acquire(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
      * always possible. */
@@ -1688,11 +1696,10 @@ 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);
-        goto out;
+    } else {
+        state->overlay_appended = true;
     }
-    state->overlay_appended = true;
 
-out:
     aio_context_release(aio_context);
 }
 
@@ -3490,13 +3497,11 @@ out:
 static BlockJob *do_backup_common(BackupCommon *backup,
                                   BlockDriverState *bs,
                                   BlockDriverState *target_bs,
-                                  AioContext *aio_context,
                                   JobTxn *txn, Error **errp)
 {
     BlockJob *job = NULL;
     BdrvDirtyBitmap *bmap = NULL;
     int job_flags = JOB_DEFAULT;
-    int ret;
 
     if (!backup->has_speed) {
         backup->speed = 0;
@@ -3520,11 +3525,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 */
@@ -3611,11 +3611,13 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
     BlockDriverState *source = NULL;
     BlockJob *job = 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;
 
     if (!backup->has_mode) {
         backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
@@ -3631,9 +3633,6 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
         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;
@@ -3641,7 +3640,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
 
     /* Early check to avoid creating target */
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
-        goto out;
+        return NULL;
     }
 
     flags = bs->open_flags | BDRV_O_RDWR;
@@ -3663,7 +3662,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
     size = bdrv_getlength(bs);
     if (size < 0) {
         error_setg_errno(errp, -size, "bdrv_getlength failed");
-        goto out;
+        return NULL;
     }
 
     if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
@@ -3681,7 +3680,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
 
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
+        return NULL;
     }
 
     options = qdict_new();
@@ -3693,22 +3692,34 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
 
     target_bs = bdrv_open(backup->target, NULL, options, flags, errp);
     if (!target_bs) {
-        goto out;
+        return NULL;
+    }
+
+    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 NULL;
     }
 
+    aio_context_release(old_context);
+    aio_context_acquire(aio_context);
+
     if (set_backing_hd) {
         bdrv_set_backing_hd(target_bs, source, &local_err);
         if (local_err) {
-            goto unref;
+            goto out;
         }
     }
 
     job = do_backup_common(qapi_DriveBackup_base(backup),
-                           bs, target_bs, aio_context, txn, errp);
+                           bs, target_bs, txn, errp);
 
-unref:
-    bdrv_unref(target_bs);
 out:
+    bdrv_unref(target_bs);
     aio_context_release(aio_context);
     return job;
 }
@@ -3739,7 +3750,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
     BlockDriverState *bs;
     BlockDriverState *target_bs;
     AioContext *aio_context;
+    AioContext *old_context;
     BlockJob *job;
+    int ret;
 
     bs = bdrv_lookup_bs(backup->device, backup->device, errp);
     if (!bs) {
@@ -3752,10 +3765,20 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn,
     }
 
     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 NULL;
+    }
+
+    aio_context_release(old_context);
     aio_context_acquire(aio_context);
 
     job = do_backup_common(qapi_BlockdevBackup_base(backup),
-                           bs, target_bs, aio_context, txn, errp);
+                           bs, target_bs, txn, errp);
 
     aio_context_release(aio_context);
     return job;
@@ -3897,6 +3920,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;
@@ -3916,9 +3940,6 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         return;
     }
 
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
-
     if (!arg->has_mode) {
         arg->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
@@ -3940,14 +3961,14 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
     size = bdrv_getlength(bs);
     if (size < 0) {
         error_setg_errno(errp, -size, "bdrv_getlength failed");
-        goto out;
+        return;
     }
 
     if (arg->has_replaces) {
         if (!arg->has_node_name) {
             error_setg(errp, "a node-name must be provided when replacing a"
                              " named node of the graph");
-            goto out;
+            return;
         }
     }
 
@@ -3986,7 +4007,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
 
     if (local_err) {
         error_propagate(errp, local_err);
-        goto out;
+        return;
     }
 
     options = qdict_new();
@@ -4002,19 +4023,27 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
      */
     target_bs = bdrv_open(arg->target, NULL, options, flags, errp);
     if (!target_bs) {
-        goto out;
+        return;
     }
 
     zero_target = (arg->sync == MIRROR_SYNC_MODE_FULL &&
                    (arg->mode == NEW_IMAGE_MODE_EXISTING ||
                     !bdrv_has_zero_init(target_bs)));
 
+    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) {
         bdrv_unref(target_bs);
-        goto out;
+        aio_context_release(old_context);
+        return;
     }
 
+    aio_context_release(old_context);
+    aio_context_acquire(aio_context);
+
     blockdev_mirror_common(arg->has_job_id ? arg->job_id : NULL, bs, target_bs,
                            arg->has_replaces, arg->replaces, arg->sync,
                            backing_mode, zero_target,
@@ -4031,7 +4060,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
                            &local_err);
     bdrv_unref(target_bs);
     error_propagate(errp, local_err);
-out:
+
     aio_context_release(aio_context);
 }
 
-- 
2.21.0

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[John Snow]

On 9/13/19 11:25 AM, Sergio Lopez wrote:
> do_drive_backup() already acquires the AioContext, so release it
> before the call.
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  blockdev.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index fbef6845c8..3927fdab80 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>  
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
> -
>      /* Paired with .clean() */
>      bdrv_drained_begin(bs);

Do we need to make this change to blockdev_backup_prepare as well?

> +    aio_context_release(aio_context);
>  
>      state->bs = bs;
>  
>      state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> -        goto out;
>      }
> -
> -out:
> -    aio_context_release(aio_context);
>  }
>  
>  static void drive_backup_commit(BlkActionState *common)
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Kevin Wolf]

Am 13.09.2019 um 21:54 hat John Snow geschrieben:
> 
> 
> On 9/13/19 11:25 AM, Sergio Lopez wrote:
> > do_drive_backup() already acquires the AioContext, so release it
> > before the call.
> > 
> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> > ---
> >  blockdev.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index fbef6845c8..3927fdab80 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
> >  
> >      aio_context = bdrv_get_aio_context(bs);
> >      aio_context_acquire(aio_context);
> > -

Are you removing this unrelated empty line intentionally?

> >      /* Paired with .clean() */
> >      bdrv_drained_begin(bs);
> 
> Do we need to make this change to blockdev_backup_prepare as well?

Actually, the whole structure feels a bit wrong. We get the bs here and
take its lock, then release the lock again and forget the reference,
only to do both things again inside do_drive_backup().

The way snapshots work is that the "normal" snapshot commands are
wrappers that turn it into a single-entry transaction. Then you have
only one code path where you can resolve the ID and take the lock just
once. So maybe backup should work like this, too?

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Sergio Lopez]

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


Kevin Wolf <kwolf@redhat.com> writes:

> Am 13.09.2019 um 21:54 hat John Snow geschrieben:
>> 
>> 
>> On 9/13/19 11:25 AM, Sergio Lopez wrote:
>> > do_drive_backup() already acquires the AioContext, so release it
>> > before the call.
>> > 
>> > Signed-off-by: Sergio Lopez <slp@redhat.com>
>> > ---
>> >  blockdev.c | 6 +-----
>> >  1 file changed, 1 insertion(+), 5 deletions(-)
>> > 
>> > diff --git a/blockdev.c b/blockdev.c
>> > index fbef6845c8..3927fdab80 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>> >  
>> >      aio_context = bdrv_get_aio_context(bs);
>> >      aio_context_acquire(aio_context);
>> > -
>
> Are you removing this unrelated empty line intentionally?

Yes. In the sense of that whole set of lines being a "open drained
section" block.

>> >      /* Paired with .clean() */
>> >      bdrv_drained_begin(bs);
>> 
>> Do we need to make this change to blockdev_backup_prepare as well?
>
> Actually, the whole structure feels a bit wrong. We get the bs here and
> take its lock, then release the lock again and forget the reference,
> only to do both things again inside do_drive_backup().
>
> The way snapshots work is that the "normal" snapshot commands are
> wrappers that turn it into a single-entry transaction. Then you have
> only one code path where you can resolve the ID and take the lock just
> once. So maybe backup should work like this, too?

I'm neither opposed nor in favor, but I think this is outside the scope
of this patch series.

Sergio.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Sergio Lopez]

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


John Snow <jsnow@redhat.com> writes:

> On 9/13/19 11:25 AM, Sergio Lopez wrote:
>> do_drive_backup() already acquires the AioContext, so release it
>> before the call.
>> 
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> ---
>>  blockdev.c | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>> 
>> diff --git a/blockdev.c b/blockdev.c
>> index fbef6845c8..3927fdab80 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>>  
>>      aio_context = bdrv_get_aio_context(bs);
>>      aio_context_acquire(aio_context);
>> -
>>      /* Paired with .clean() */
>>      bdrv_drained_begin(bs);
>
> Do we need to make this change to blockdev_backup_prepare as well?

Yes, we do. Thanks.

>> +    aio_context_release(aio_context);
>>  
>>      state->bs = bs;
>>  
>>      state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>> -        goto out;
>>      }
>> -
>> -out:
>> -    aio_context_release(aio_context);
>>  }
>>  
>>  static void drive_backup_commit(BlkActionState *common)
>> 


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

Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Sergio Lopez]

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


Sergio Lopez <slp@redhat.com> writes:

> Kevin Wolf <kwolf@redhat.com> writes:
>
>> Am 13.09.2019 um 21:54 hat John Snow geschrieben:
>>> 
>>> 
>>> On 9/13/19 11:25 AM, Sergio Lopez wrote:
>>> > do_drive_backup() already acquires the AioContext, so release it
>>> > before the call.
>>> > 
>>> > Signed-off-by: Sergio Lopez <slp@redhat.com>
>>> > ---
>>> >  blockdev.c | 6 +-----
>>> >  1 file changed, 1 insertion(+), 5 deletions(-)
>>> > 
>>> > diff --git a/blockdev.c b/blockdev.c
>>> > index fbef6845c8..3927fdab80 100644
>>> > --- a/blockdev.c
>>> > +++ b/blockdev.c
>>> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
>>> >  
>>> >      aio_context = bdrv_get_aio_context(bs);
>>> >      aio_context_acquire(aio_context);
>>> > -
>>
>> Are you removing this unrelated empty line intentionally?
>
> Yes. In the sense of that whole set of lines being a "open drained
> section" block.
>
>>> >      /* Paired with .clean() */
>>> >      bdrv_drained_begin(bs);
>>> 
>>> Do we need to make this change to blockdev_backup_prepare as well?
>>
>> Actually, the whole structure feels a bit wrong. We get the bs here and
>> take its lock, then release the lock again and forget the reference,
>> only to do both things again inside do_drive_backup().
>>
>> The way snapshots work is that the "normal" snapshot commands are
>> wrappers that turn it into a single-entry transaction. Then you have
>> only one code path where you can resolve the ID and take the lock just
>> once. So maybe backup should work like this, too?
>
> I'm neither opposed nor in favor, but I think this is outside the scope
> of this patch series.

Kevin, do you think we should attempt to just fix this issue (which
would make a possible backport easier) or try to move all blockdev
actions to be transaction-based?

Cheers,
Sergio.

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

Re: [Qemu-block] [PATCH v2 1/2] blockdev: release the AioContext at drive_backup_prepare

[Kevin Wolf]

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

Am 03.10.2019 um 11:33 hat Sergio Lopez geschrieben:
> 
> Sergio Lopez <slp@redhat.com> writes:
> 
> > Kevin Wolf <kwolf@redhat.com> writes:
> >
> >> Am 13.09.2019 um 21:54 hat John Snow geschrieben:
> >>> 
> >>> 
> >>> On 9/13/19 11:25 AM, Sergio Lopez wrote:
> >>> > do_drive_backup() already acquires the AioContext, so release it
> >>> > before the call.
> >>> > 
> >>> > Signed-off-by: Sergio Lopez <slp@redhat.com>
> >>> > ---
> >>> >  blockdev.c | 6 +-----
> >>> >  1 file changed, 1 insertion(+), 5 deletions(-)
> >>> > 
> >>> > diff --git a/blockdev.c b/blockdev.c
> >>> > index fbef6845c8..3927fdab80 100644
> >>> > --- a/blockdev.c
> >>> > +++ b/blockdev.c
> >>> > @@ -1783,20 +1783,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
> >>> >  
> >>> >      aio_context = bdrv_get_aio_context(bs);
> >>> >      aio_context_acquire(aio_context);
> >>> > -
> >>
> >> Are you removing this unrelated empty line intentionally?
> >
> > Yes. In the sense of that whole set of lines being a "open drained
> > section" block.
> >
> >>> >      /* Paired with .clean() */
> >>> >      bdrv_drained_begin(bs);
> >>> 
> >>> Do we need to make this change to blockdev_backup_prepare as well?
> >>
> >> Actually, the whole structure feels a bit wrong. We get the bs here and
> >> take its lock, then release the lock again and forget the reference,
> >> only to do both things again inside do_drive_backup().
> >>
> >> The way snapshots work is that the "normal" snapshot commands are
> >> wrappers that turn it into a single-entry transaction. Then you have
> >> only one code path where you can resolve the ID and take the lock just
> >> once. So maybe backup should work like this, too?
> >
> > I'm neither opposed nor in favor, but I think this is outside the scope
> > of this patch series.
> 
> Kevin, do you think we should attempt to just fix this issue (which
> would make a possible backport easier) or try to move all blockdev
> actions to be transaction-based?

Maybe fix it and then do the cleanup on top, though possibly in the same
series?

Kevin

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