This patch actually implements the transactional callback system for the drive_backup action.
(1) We create a functional closure that envelops the original drive_backup callback, to be able to intercept the completion status and return code for the job. (2) We inform the BlockJob layer that this job is involved in a transaction, which just increments a reference on the bitmap. (3) We add the drive_backup_cb method for the drive_backup action, which unpacks the completion information and invokes the final cleanup. (4) backup_action_complete will perform the final cleanup on the opaque object (a BdrvDirtyBitmap, here) returned earlier. (5) In the case of transaction cancellation, drive_backup_cb is still responsible for cleaning up the mess we may have already made. Signed-off-by: John Snow <js...@redhat.com> --- block/backup.c | 20 ++++++++++++++++++++ blockdev.c | 33 ++++++++++++++++++++++++++++++--- include/block/block_int.h | 16 ++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/block/backup.c b/block/backup.c index 4ac0be8..e681f1b 100644 --- a/block/backup.c +++ b/block/backup.c @@ -233,6 +233,26 @@ typedef struct { int ret; } BackupCompleteData; +void *backup_action_start(BlockJob *job) +{ + BackupBlockJob *s = container_of(job, BackupBlockJob, common); + + if (s->sync_bitmap) { + bdrv_frozen_bitmap_incref(s->sync_bitmap); + } + + return s->sync_bitmap; +} + +void backup_action_complete(BlockDriverState *bs, void *opaque, int ret) +{ + BdrvDirtyBitmap *bitmap = opaque; + + if (bitmap) { + bdrv_frozen_bitmap_decref(bs, bitmap, ret); + } +} + static void backup_complete(BlockJob *job, void *opaque) { BackupBlockJob *s = container_of(job, BackupBlockJob, common); diff --git a/blockdev.c b/blockdev.c index 4cb4179..905bf84 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1423,7 +1423,6 @@ static void transaction_action_callback(void *opaque, int ret) * * @return The callback to be used instead of @callback. */ -__attribute__((__unused__)) static CallbackFn *new_action_cb_wrapper(BlkActionState *common, void *opaque, CallbackFn *callback, @@ -1451,7 +1450,6 @@ static CallbackFn *new_action_cb_wrapper(BlkActionState *common, /** * Undo any actions performed by the above call. */ -__attribute__((__unused__)) static void cancel_action_cb_wrapper(BlkActionState *common) { /* Stage 0: Wrapper was never created: */ @@ -1789,6 +1787,7 @@ typedef struct DriveBackupState { BlockDriverState *bs; AioContext *aio_context; BlockJob *job; + void *opaque; } DriveBackupState; static void do_drive_backup(const char *device, const char *target, @@ -1803,6 +1802,7 @@ static void do_drive_backup(const char *device, const char *target, BlockdevOnError on_target_error, BlockCompletionFunc *cb, void *opaque, Error **errp); +static void block_job_cb(void *opaque, int ret); static void drive_backup_prepare(BlkActionState *common, Error **errp) { @@ -1811,6 +1811,8 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) BlockBackend *blk; DriveBackup *backup; Error *local_err = NULL; + CallbackFn *cb; + void *opaque; assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); backup = common->action->drive_backup; @@ -1826,6 +1828,10 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) state->aio_context = bdrv_get_aio_context(bs); aio_context_acquire(state->aio_context); + /* Create our transactional callback wrapper, + and register that we'd like to call .cb() later. */ + cb = new_action_cb_wrapper(common, bs, block_job_cb, &opaque); + do_drive_backup(backup->device, backup->target, backup->has_format, backup->format, backup->sync, @@ -1834,7 +1840,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) backup->has_bitmap, backup->bitmap, backup->has_on_source_error, backup->on_source_error, backup->has_on_target_error, backup->on_target_error, - NULL, NULL, + cb, opaque, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -1843,6 +1849,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) state->bs = bs; state->job = state->bs->job; + state->opaque = backup_action_start(state->job); } static void drive_backup_abort(BlkActionState *common) @@ -1854,6 +1861,10 @@ static void drive_backup_abort(BlkActionState *common) if (bs && bs->job && bs->job == state->job) { block_job_cancel_sync(bs->job); } + + /* Undo any callback actions we may have done. Putting down references + * obtained during prepare() is handled by cb(). */ + cancel_action_cb_wrapper(common); } static void drive_backup_clean(BlkActionState *common) @@ -1863,6 +1874,21 @@ static void drive_backup_clean(BlkActionState *common) if (state->aio_context) { aio_context_release(state->aio_context); } + state->job = NULL; +} + +static void drive_backup_cb(BlkActionState *common) +{ + BlkActionCallbackData *cb_data = common->cb_data; + BlockDriverState *bs = cb_data->opaque; + DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + + state->aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(state->aio_context); + assert(state->bs == bs); + + backup_action_complete(bs, state->opaque, common->transaction->status); + aio_context_release(state->aio_context); } typedef struct BlockdevBackupState { @@ -2063,6 +2089,7 @@ static const BlkActionOps actions[] = { .prepare = drive_backup_prepare, .abort = drive_backup_abort, .clean = drive_backup_clean, + .cb = drive_backup_cb, }, [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = { .instance_size = sizeof(BlockdevBackupState), diff --git a/include/block/block_int.h b/include/block/block_int.h index 5af712f..5ca5f15 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -630,6 +630,22 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, BlockCompletionFunc *cb, void *opaque, Error **errp); +/* + * backup_action_start: + * @job: A BackupBlockJob we wish to specify is part of a transaction. + * Allows cleanup mechanisms to occur later via backup_action_complete. + */ +void *backup_action_start(BlockJob *job); + +/* + * backup_transaction_complete: + * @bs: The BlockDriverState associated with the backup job. + * @opaque: As returned from backup_action_start. + * @ret: The collective return code for the entire transaction. + * Expects zero for success, non-zero for an error otherwise. + */ +void backup_action_complete(BlockDriverState *bs, void *opaque, int ret); + void blk_dev_change_media_cb(BlockBackend *blk, bool load); bool blk_dev_has_removable_media(BlockBackend *blk); void blk_dev_eject_request(BlockBackend *blk, bool force); -- 2.1.0