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

[Sergio Lopez <slp@redhat.com>]

[-- 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 --]