Kevin Wolf <[email protected]> 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 <[email protected]> >> Signed-off-by: Sergio Lopez <[email protected]> >> --- >> 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
signature.asc
Description: PGP signature
