[Qemu-devel] [PATCH v5 08/10] block: drive_backup transaction callback support

[John Snow]

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 <address@hidden>
---
 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