From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTNCD-0004fr-8U for qemu-devel@nongnu.org; Wed, 04 Mar 2015 23:15:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTNC8-0005eO-BZ for qemu-devel@nongnu.org; Wed, 04 Mar 2015 23:15:37 -0500 From: John Snow Date: Wed, 4 Mar 2015 23:15:07 -0500 Message-Id: <1425528911-10300-8-git-send-email-jsnow@redhat.com> In-Reply-To: <1425528911-10300-1-git-send-email-jsnow@redhat.com> References: <1425528911-10300-1-git-send-email-jsnow@redhat.com> Subject: [Qemu-devel] [PATCH 07/11] block: drive_backup transaction callback support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, John Snow , qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, mreitz@redhat.com This patch actually implements the transactional callback system for the drive_backup transaction. (1) We manually pick up a reference to the bitmap if present to allow its cleanup to be delayed until after all drive_backup jobs launched by the transaction have fully completed. (2) 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. (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_transaction_complete will perform the final cleanup on the backup job. (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 --- block/backup.c | 9 +++++++ blockdev.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/block/block_int.h | 8 ++++++ 3 files changed, 69 insertions(+), 12 deletions(-) diff --git a/block/backup.c b/block/backup.c index 4332df4..3673fb0 100644 --- a/block/backup.c +++ b/block/backup.c @@ -233,6 +233,15 @@ typedef struct { int ret; } BackupCompleteData; +void backup_transaction_complete(BlockJob *job, int ret) +{ + BackupBlockJob *s = container_of(job, BackupBlockJob, common); + + if (s->sync_bitmap) { + bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, ret, NULL); + } +} + static void backup_complete(BlockJob *job, void *opaque) { BackupBlockJob *s = container_of(job, BackupBlockJob, common); diff --git a/blockdev.c b/blockdev.c index 9e3b9e9..3ff14a7 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1363,14 +1363,6 @@ static void transaction_callback(void *opaque, int ret) typedef void (CallbackFn)(void *opaque, int ret); -/* Temporary. Removed in the next patch. */ -CallbackFn *new_transaction_wrapper(BlkTransactionState *common, - void *opaque, - void (*callback)(void *, int), - void **new_opaque); - -void undo_transaction_wrapper(BlkTransactionState *common); - /** * Create a new transactional callback wrapper. * @@ -1389,7 +1381,7 @@ void undo_transaction_wrapper(BlkTransactionState *common); * * @return The callback to be used instead of @callback. */ -CallbackFn *new_transaction_wrapper(BlkTransactionState *common, +static CallbackFn *new_transaction_wrapper(BlkTransactionState *common, void *opaque, CallbackFn *callback, void **new_opaque) @@ -1416,7 +1408,7 @@ CallbackFn *new_transaction_wrapper(BlkTransactionState *common, /** * Undo any actions performed by the above call. */ -void undo_transaction_wrapper(BlkTransactionState *common) +static void undo_transaction_wrapper(BlkTransactionState *common) { BlkTransactionList *btl = common->list; BlkTransactionState *bts; @@ -1449,6 +1441,7 @@ void undo_transaction_wrapper(BlkTransactionState *common) blk_put_transaction_state(common); } +static void block_job_cb(void *opaque, int ret); static void _drive_backup(const char *device, const char *target, bool has_format, const char *format, enum MirrorSyncMode sync, @@ -1767,6 +1760,9 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) BlockDriverState *bs; DriveBackup *backup; Error *local_err = NULL; + CallbackFn *cb; + void *opaque; + BdrvDirtyBitmap *bmap = NULL; assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); backup = common->action->drive_backup; @@ -1777,6 +1773,19 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) return; } + /* BackupBlockJob is opaque to us, so look up the bitmap ourselves */ + if (backup->has_bitmap) { + bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); + if (!bmap) { + error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); + return; + } + } + + /* Create our transactional callback wrapper, + and register that we'd like to call .cb() later. */ + cb = new_transaction_wrapper(common, bs, block_job_cb, &opaque); + /* AioContext is released in .clean() */ state->aio_context = bdrv_get_aio_context(bs); aio_context_acquire(state->aio_context); @@ -1789,7 +1798,7 @@ static void drive_backup_prepare(BlkTransactionState *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); @@ -1798,6 +1807,11 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp) state->bs = bs; state->job = state->bs->job; + /* Keep the job alive until .cb(), too. */ + block_job_incref(state->job); + if (bmap) { + bdrv_dirty_bitmap_incref(bmap); + } } static void drive_backup_abort(BlkTransactionState *common) @@ -1809,6 +1823,10 @@ static void drive_backup_abort(BlkTransactionState *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(). */ + undo_transaction_wrapper(common); } static void drive_backup_clean(BlkTransactionState *common) @@ -1820,6 +1838,27 @@ static void drive_backup_clean(BlkTransactionState *common) } } +static void drive_backup_cb(BlkTransactionState *common) +{ + BlkTransactionData *btd = common->opaque; + BlockDriverState *bs = btd->opaque; + DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + + assert(state->bs == bs); + if (bs->job) { + assert(state->job == bs->job); + } + + state->aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(state->aio_context); + + /* Note: We also have the individual job's return code in btd->ret */ + backup_transaction_complete(state->job, common->list->status); + block_job_decref(state->job); + + aio_context_release(state->aio_context); +} + typedef struct BlockdevBackupState { BlkTransactionState common; BlockDriverState *bs; @@ -2004,7 +2043,8 @@ static const BdrvActionOps actions[] = { .instance_size = sizeof(DriveBackupState), .prepare = drive_backup_prepare, .abort = drive_backup_abort, - .clean = drive_backup_clean + .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 e0d5561..731684d 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -619,6 +619,14 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target, BlockCompletionFunc *cb, void *opaque, Error **errp); +/* + * backup_transaction_complete + * @job The BackupJob that was associated with a transaction + * @ret Amalgamated return code for the entire transaction + */ +void backup_transaction_complete(BlockJob *job, 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); -- 1.9.3