[Qemu-devel] [PATCH 07/11] block: drive_backup transaction callback support

[John Snow <jsnow@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 <jsnow@redhat.com>
---
 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