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 <mre...@redhat.com>
> Signed-off-by: Sergio Lopez <s...@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
