block: incremental backup transactions

index 7904098..ca5b1e9 100644
@@ -3306,10 +3306,25 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
     hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
 }
 
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap)
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
 {
     assert(bdrv_dirty_bitmap_enabled(bitmap));
-    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+    if (!out) {
+        hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
+    } else {
+        HBitmap *backup = bitmap->bitmap;
+        bitmap->bitmap = hbitmap_alloc(bitmap->size,
+                                       hbitmap_granularity(backup));
+        *out = backup;
+    }
+}
+
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in)
+{
+    HBitmap *tmp = bitmap->bitmap;
+    assert(bdrv_dirty_bitmap_enabled(bitmap));
+    bitmap->bitmap = in;
+    hbitmap_free(tmp);
 }
 
 void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,

132 lines not on this page · raw message

index 7d1a717..9f0ff3d 100644
@@ -479,7 +479,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t cur_sector, int nr_sectors);
-void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
 void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
 int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);

index db29b74..50355df 100644
@@ -638,4 +638,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
                       int nr_sectors);
 
+void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out);
+void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
+
 #endif /* BLOCK_INT_H */

index a62cc4b..6df575d 100644
@@ -1228,43 +1228,57 @@ static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
 
 /* New and old BlockDriverState structs for atomic group operations */
 
-typedef struct BlkTransactionState BlkTransactionState;
+typedef struct BlkActionState BlkActionState;
 
-/* Only prepare() may fail. In a single transaction, only one of commit() or
-   abort() will be called, clean() will always be called if it present. */
-typedef struct BdrvActionOps {
-    /* Size of state struct, in bytes. */
+/**
+ * BlkActionOps:
+ * Table of operations that define an Action.
+ *
+ * @instance_size: Size of state struct, in bytes.
+ * @prepare: Prepare the work, must NOT be NULL.
+ * @commit: Commit the changes, can be NULL.
+ * @abort: Abort the changes on fail, can be NULL.
+ * @clean: Clean up resources after all transaction actions have called
+ *         commit() or abort(). Can be NULL.
+ *
+ * Only prepare() may fail. In a single transaction, only one of commit() or
+ * abort() will be called. clean() will always be called if it is present.
+ */
+typedef struct BlkActionOps {
     size_t instance_size;
-    /* Prepare the work, must NOT be NULL. */
-    void (*prepare)(BlkTransactionState *common, Error **errp);
-    /* Commit the changes, can be NULL. */
-    void (*commit)(BlkTransactionState *common);
-    /* Abort the changes on fail, can be NULL. */
-    void (*abort)(BlkTransactionState *common);
-    /* Clean up resource in the end, can be NULL. */
-    void (*clean)(BlkTransactionState *common);
-} BdrvActionOps;
+    void (*prepare)(BlkActionState *common, Error **errp);
+    void (*commit)(BlkActionState *common);
+    void (*abort)(BlkActionState *common);
+    void (*clean)(BlkActionState *common);
+} BlkActionOps;
 
-/*
- * This structure must be arranged as first member in child type, assuming
- * that compiler will also arrange it to the same address with parent instance.
- * Later it will be used in free().
+/**
+ * BlkActionState:
+ * Describes one Action's state within a Transaction.
+ *
+ * @action: QAPI-defined enum identifying which Action to perform.
+ * @ops: Table of ActionOps this Action can perform.
+ * @entry: List membership for all Actions in this Transaction.
+ *
+ * This structure must be arranged as first member in a subclassed type,
+ * assuming that the compiler will also arrange it to the same offsets as the
+ * base class.
  */
-struct BlkTransactionState {
+struct BlkActionState {
     TransactionAction *action;
-    const BdrvActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkTransactionState) entry;
+    const BlkActionOps *ops;
+    QSIMPLEQ_ENTRY(BlkActionState) entry;
 };
 
 /* internal snapshot private data */
 typedef struct InternalSnapshotState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *bs;
     AioContext *aio_context;
     QEMUSnapshotInfo sn;
 } InternalSnapshotState;
 
-static void internal_snapshot_prepare(BlkTransactionState *common,
+static void internal_snapshot_prepare(BlkActionState *common,
                                       Error **errp)
 {
     Error *local_err = NULL;

@@ -1359,7 +1373,7 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
     state->bs = bs;
 }
 
-static void internal_snapshot_abort(BlkTransactionState *common)
+static void internal_snapshot_abort(BlkActionState *common)
 {
     InternalSnapshotState *state =
                              DO_UPCAST(InternalSnapshotState, common, common);

@@ -1382,7 +1396,7 @@ static void internal_snapshot_abort(BlkTransactionState *common)
     }
 }
 
-static void internal_snapshot_clean(BlkTransactionState *common)
+static void internal_snapshot_clean(BlkActionState *common)
 {
     InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
                                              common, common);

@@ -1394,13 +1408,13 @@ static void internal_snapshot_clean(BlkTransactionState *common)
 
 /* external snapshot private data */
 typedef struct ExternalSnapshotState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
     AioContext *aio_context;
 } ExternalSnapshotState;
 
-static void external_snapshot_prepare(BlkTransactionState *common,
+static void external_snapshot_prepare(BlkActionState *common,
                                       Error **errp)
 {
     BlockDriver *drv;

@@ -1521,7 +1535,7 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     }
 }
 
-static void external_snapshot_commit(BlkTransactionState *common)
+static void external_snapshot_commit(BlkActionState *common)
 {
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);

@@ -1539,7 +1553,7 @@ static void external_snapshot_commit(BlkTransactionState *common)
     aio_context_release(state->aio_context);
 }
 
-static void external_snapshot_abort(BlkTransactionState *common)
+static void external_snapshot_abort(BlkActionState *common)
 {
     ExternalSnapshotState *state =
                              DO_UPCAST(ExternalSnapshotState, common, common);

@@ -1552,13 +1566,13 @@ static void external_snapshot_abort(BlkTransactionState *common)
 }
 
 typedef struct DriveBackupState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *bs;
     AioContext *aio_context;
     BlockJob *job;
 } DriveBackupState;
 
-static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
+static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs;

@@ -1598,7 +1612,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
     state->job = state->bs->job;
 }
 
-static void drive_backup_abort(BlkTransactionState *common)
+static void drive_backup_abort(BlkActionState *common)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
     BlockDriverState *bs = state->bs;

@@ -1609,7 +1623,7 @@ static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
-static void drive_backup_clean(BlkTransactionState *common)
+static void drive_backup_clean(BlkActionState *common)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
 

@@ -1619,13 +1633,13 @@ static void drive_backup_clean(BlkTransactionState *common)
 }
 
 typedef struct BlockdevBackupState {
-    BlkTransactionState common;
+    BlkActionState common;
     BlockDriverState *bs;
     BlockJob *job;
     AioContext *aio_context;
 } BlockdevBackupState;
 
-static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
+static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockdevBackup *backup;

@@ -1674,7 +1688,7 @@ static void blockdev_backup_prepare(BlkTransactionState *common, Error **errp)
     state->job = state->bs->job;
 }
 
-static void blockdev_backup_abort(BlkTransactionState *common)
+static void blockdev_backup_abort(BlkActionState *common)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
     BlockDriverState *bs = state->bs;

@@ -1685,7 +1699,7 @@ static void blockdev_backup_abort(BlkTransactionState *common)
     }
 }
 
-static void blockdev_backup_clean(BlkTransactionState *common)
+static void blockdev_backup_clean(BlkActionState *common)
 {
     BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
 

@@ -1695,7 +1709,7 @@ static void blockdev_backup_clean(BlkTransactionState *common)
 }
 
 typedef struct BlockDirtyBitmapState {
-    BlkTransactionState common;
+    BlkActionState common;
     BdrvDirtyBitmap *bitmap;
     BlockDriverState *bs;
     AioContext *aio_context;

@@ -1703,7 +1717,7 @@ typedef struct BlockDirtyBitmapState {
     bool prepared;
 } BlockDirtyBitmapState;
 
-static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+static void block_dirty_bitmap_add_prepare(BlkActionState *common,
                                            Error **errp)
 {
     Error *local_err = NULL;

@@ -1724,7 +1738,7 @@ static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
     }
 }
 
-static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+static void block_dirty_bitmap_add_abort(BlkActionState *common)
 {
     BlockDirtyBitmapAdd *action;
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,

@@ -1739,7 +1753,7 @@ static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
     }
 }
 
-static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+static void block_dirty_bitmap_clear_prepare(BlkActionState *common,
                                              Error **errp)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,

@@ -1768,7 +1782,7 @@ static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
     /* AioContext is released in .clean() */
 }
 
-static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_abort(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);

@@ -1776,7 +1790,7 @@ static void block_dirty_bitmap_clear_abort(BlkTransactionState *common)
     bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup);
 }
 
-static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_commit(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);

@@ -1784,7 +1798,7 @@ static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
     hbitmap_free(state->backup);
 }
 
-static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+static void block_dirty_bitmap_clear_clean(BlkActionState *common)
 {
     BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
                                              common, common);

@@ -1794,17 +1808,17 @@ static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
     }
 }
 
-static void abort_prepare(BlkTransactionState *common, Error **errp)
+static void abort_prepare(BlkActionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
 }
 
-static void abort_commit(BlkTransactionState *common)
+static void abort_commit(BlkActionState *common)
 {
     g_assert_not_reached(); /* this action never succeeds */
 }
 
-static const BdrvActionOps actions[] = {
+static const BlkActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
         .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,

@@ -1824,7 +1838,7 @@ static const BdrvActionOps actions[] = {
         .clean = blockdev_backup_clean,
     },
     [TRANSACTION_ACTION_KIND_ABORT] = {
-        .instance_size = sizeof(BlkTransactionState),
+        .instance_size = sizeof(BlkActionState),
         .prepare = abort_prepare,
         .commit = abort_commit,
     },

@@ -1855,10 +1869,10 @@ static const BdrvActionOps actions[] = {
 void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
-    BlkTransactionState *state, *next;
+    BlkActionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any operations */

@@ -1867,7 +1881,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     /* We don't do anything in this loop that commits us to the operations */
     while (NULL != dev_entry) {
         TransactionAction *dev_info = NULL;
-        const BdrvActionOps *ops;
+        const BlkActionOps *ops;
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;

index 6df575d..068eccb 100644
@@ -1253,6 +1253,19 @@ typedef struct BlkActionOps {
 } BlkActionOps;
 
 /**
+ * BlkTransactionState:
+ * Object to track the job completion info for jobs launched
+ * by a transaction group.
+ *
+ * @jobs: A reference count that tracks how many jobs still need to complete.
+ * @actions: A list of all Actions in the Transaction.
+ */
+typedef struct BlkTransactionState {
+    int jobs;
+    QTAILQ_HEAD(actions, BlkActionState) actions;
+} BlkTransactionState;
+
+/**
  * BlkActionState:
  * Describes one Action's state within a Transaction.
  *

@@ -1267,9 +1280,45 @@ typedef struct BlkActionOps {
 struct BlkActionState {
     TransactionAction *action;
     const BlkActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkActionState) entry;
+    QTAILQ_ENTRY(BlkActionState) entry;
 };
 
+static BlkTransactionState *new_blk_transaction_state(void)
+{
+    BlkTransactionState *bts = g_new0(BlkTransactionState, 1);
+
+    /* The qmp_transaction function itself can be considered a pending job
+     * that should complete before pending action callbacks are executed,
+     * so increment the jobs remaining refcount to indicate this. */
+    bts->jobs = 1;
+    QTAILQ_INIT(&bts->actions);
+    return bts;
+}
+
+static void destroy_blk_transaction_state(BlkTransactionState *bts)
+{
+    BlkActionState *bas, *bas_next;
+
+    /* The list should in normal cases be empty,
+     * but in case someone really just wants to kibosh the whole deal: */
+    QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
+        QTAILQ_REMOVE(&bts->actions, bas, entry);
+        g_free(bas);
+    }
+
+    g_free(bts);
+}
+
+static BlkTransactionState *transaction_job_complete(BlkTransactionState *bts)
+{
+    bts->jobs--;
+    if (bts->jobs == 0) {
+        destroy_blk_transaction_state(bts);
+        return NULL;
+    }
+    return bts;
+}
+
 /* internal snapshot private data */
 typedef struct InternalSnapshotState {
     BlkActionState common;

@@ -1870,10 +1919,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
     BlkActionState *state, *next;
+    BlkTransactionState *bts;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkActionState) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
+    bts = new_blk_transaction_state();
 
     /* drain all i/o before any operations */
     bdrv_drain_all();

@@ -1894,7 +1943,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         state = g_malloc0(ops->instance_size);
         state->ops = ops;
         state->action = dev_info;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
+        QTAILQ_INSERT_TAIL(&bts->actions, state, entry);
 
         state->ops->prepare(state, &local_err);
         if (local_err) {

@@ -1903,7 +1952,7 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         }
     }
 
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH(state, &bts->actions, entry) {
         if (state->ops->commit) {
             state->ops->commit(state);
         }

@@ -1914,18 +1963,21 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 
 delete_and_fail:
     /* failure, and it is all-or-none; roll back all operations */
-    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+    QTAILQ_FOREACH(state, &bts->actions, entry) {
         if (state->ops->abort) {
             state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+    QTAILQ_FOREACH_SAFE(state, &bts->actions, entry, next) {
         if (state->ops->clean) {
             state->ops->clean(state);
         }
+        QTAILQ_REMOVE(&bts->actions, state, entry);
         g_free(state);
     }
+
+    transaction_job_complete(bts);
 }
 
 

index 068eccb..27db1b4 100644
@@ -1240,6 +1240,8 @@ typedef struct BlkActionState BlkActionState;
  * @abort: Abort the changes on fail, can be NULL.
  * @clean: Clean up resources after all transaction actions have called
  *         commit() or abort(). Can be NULL.
+ * @cb: Executed after all jobs launched by actions in the transaction finish,
+ *      but only if requested by new_action_cb_wrapper() prior to clean().
  *
  * Only prepare() may fail. In a single transaction, only one of commit() or
  * abort() will be called. clean() will always be called if it is present.

@@ -1250,6 +1252,7 @@ typedef struct BlkActionOps {
     void (*commit)(BlkActionState *common);
     void (*abort)(BlkActionState *common);
     void (*clean)(BlkActionState *common);
+    void (*cb)(BlkActionState *common);
 } BlkActionOps;
 
 /**

@@ -1258,19 +1261,46 @@ typedef struct BlkActionOps {
  * by a transaction group.
  *
  * @jobs: A reference count that tracks how many jobs still need to complete.
+ * @status: A cumulative return code for all actions that have reported
+ *          a return code via callback in the transaction.
  * @actions: A list of all Actions in the Transaction.
+ *           However, once the transaction has completed, it will be only a list
+ *           of transactions that have registered a post-transaction callback.
  */
 typedef struct BlkTransactionState {
     int jobs;
+    int status;
     QTAILQ_HEAD(actions, BlkActionState) actions;
 } BlkTransactionState;
 
+typedef void (CallbackFn)(void *opaque, int ret);
+
+/**
+ * BlkActionCallbackData:
+ * Necessary state for intercepting and
+ * re-delivering a callback triggered by an Action.
+ *
+ * @opaque: The data to be given to the encapsulated callback when
+ *          a job launched by an Action completes.
+ * @ret: The status code that was delivered to the encapsulated callback.
+ * @callback: The encapsulated callback to invoke upon completion of
+ *            the Job launched by the Action.
+ */
+typedef struct BlkActionCallbackData {
+    void *opaque;
+    int ret;
+    CallbackFn *callback;
+} BlkActionCallbackData;
+
 /**
  * BlkActionState:
  * Describes one Action's state within a Transaction.
  *
  * @action: QAPI-defined enum identifying which Action to perform.
  * @ops: Table of ActionOps this Action can perform.
+ * @transaction: A pointer back to the Transaction this Action belongs to.
+ * @cb_data: Information on this Action's encapsulated callback, if any.
+ * @refcount: reference count, allowing access to this state beyond clean().
  * @entry: List membership for all Actions in this Transaction.
  *
  * This structure must be arranged as first member in a subclassed type,

@@ -1280,6 +1310,9 @@ typedef struct BlkTransactionState {
 struct BlkActionState {
     TransactionAction *action;
     const BlkActionOps *ops;
+    BlkTransactionState *transaction;
+    BlkActionCallbackData *cb_data;
+    int refcount;
     QTAILQ_ENTRY(BlkActionState) entry;
 };
 

@@ -1295,6 +1328,18 @@ static BlkTransactionState *new_blk_transaction_state(void)
     return bts;
 }
 
+static BlkActionState *put_blk_action_state(BlkActionState *state)
+{
+    state->refcount--;
+    if (state->refcount == 0) {
+        QTAILQ_REMOVE(&state->transaction->actions, state, entry);
+        g_free(state->cb_data);
+        g_free(state);
+        return NULL;
+    }
+    return state;
+}
+
 static void destroy_blk_transaction_state(BlkTransactionState *bts)
 {
     BlkActionState *bas, *bas_next;

@@ -1302,23 +1347,151 @@ static void destroy_blk_transaction_state(BlkTransactionState *bts)
     /* The list should in normal cases be empty,
      * but in case someone really just wants to kibosh the whole deal: */
     QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
-        QTAILQ_REMOVE(&bts->actions, bas, entry);
-        g_free(bas);
+        put_blk_action_state(bas);
     }
 
     g_free(bts);
 }
 
+static void transaction_run_cb_action_ops(BlkTransactionState *bts)
+{
+    BlkActionState *bas, *bas_next;
+
+    QTAILQ_FOREACH_SAFE(bas, &bts->actions, entry, bas_next) {
+        if (bas->ops->cb) {
+            bas->ops->cb(bas);
+        }
+
+        g_free(bas->cb_data);
+        bas->cb_data = NULL;
+        put_blk_action_state(bas);
+    }
+}
+
 static BlkTransactionState *transaction_job_complete(BlkTransactionState *bts)
 {
     bts->jobs--;
     if (bts->jobs == 0) {
+        transaction_run_cb_action_ops(bts);
         destroy_blk_transaction_state(bts);
         return NULL;
     }
     return bts;
 }
 
+static void transaction_job_cancel(BlkActionState *bas)
+{
+    assert(bas->transaction->jobs > 1);
+    transaction_job_complete(bas->transaction);
+}
+
+/**
+ * Intercept a callback that was issued due to a transactional action.
+ */
+static void transaction_action_callback(void *opaque, int ret)
+{
+    BlkActionState *common = opaque;
+    BlkActionCallbackData *cb_data = common->cb_data;
+    CallbackFn *callback = cb_data->callback;
+
+    /* Prepare data for ops->cb() */
+    cb_data->callback = NULL;
+    cb_data->ret = ret;
+
+    /* Cumulative return code will track the first error discovered */
+    if (!common->transaction->status) {
+        common->transaction->status = ret;
+    }
+
+    /* Deliver the intercepted callback prior to marking it as completed */
+    callback(cb_data->opaque, cb_data->ret);
+    transaction_job_complete(common->transaction);
+}
+
+/**
+ * Create a new transactional callback wrapper.
+ *
+ * Given a callback and a closure, generate a new
+ * callback and closure that will invoke the
+ * given callback with the given closure.
+ *
+ * After all wrappers in the transactional group have
+ * been processed, each action's .cb() method will be
+ * invoked.
+ *
+ * @common The transaction action state to set a callback for.
+ * @opaque A closure intended for the encapsulated callback.
+ * @callback The callback we are encapsulating.
+ * @new_opaque[out]: The closure to be used instead of @opaque.
+ *
+ * @return The callback to be used instead of @callback.
+ */
+__attribute__((__unused__))
+static CallbackFn *new_action_cb_wrapper(BlkActionState *common,
+                                         void *opaque,
+                                         CallbackFn *callback,
+                                         void **new_opaque)
+{
+    BlkActionCallbackData *cb_data = g_new0(BlkActionCallbackData, 1);
+    assert(new_opaque);
+
+    /* Stash the original callback information */
+    cb_data->opaque = opaque;
+    cb_data->callback = callback;
+    common->cb_data = cb_data;
+
+    /* The BAS itself will serve as our new closure */
+    *new_opaque = common;
+    common->refcount++;
+
+    /* Inform the transaction to expect one more return */
+    common->transaction->jobs++;
+
+    /* Lastly, the actual callback function to handle the interception. */
+    return transaction_action_callback;
+}
+
+/**
+ * Undo any actions performed by the above call.
+ */
+__attribute__((__unused__))
+static void cancel_action_cb_wrapper(BlkActionState *common)
+{
+    /* Stage 0: Wrapper was never created: */
+    if (common->cb_data == NULL) {
+        assert(common->refcount == 1);
+        return;
+    }
+
+    /* Stage 1: Callback was created, but the job never launched.
+     * (We assume that the job cannot possibly be running, because
+     *  we assume it has been synchronously canceled if it was.)
+     *
+     * We also assume that any cleanup normally handled by cb()
+     * is not needed, because it should have been prepared after
+     * the job launched.
+     */
+    if (common->cb_data && common->cb_data->callback) {
+        transaction_job_cancel(common);
+        goto cleanup;
+    }
+
+    /* Stage 2: Job already completed, or was canceled.
+     *
+     * Force an error in the callback data and just invoke the completion
+     * handler to perform appropriate cleanup for us.
+     */
+    if (common->cb_data && !common->cb_data->callback) {
+        common->cb_data->ret = -ECANCELED;
+        common->ops->cb(common);
+    }
+
+ cleanup:
+    g_free(common->cb_data);
+    common->cb_data = NULL;
+    put_blk_action_state(common);
+}
+
 /* internal snapshot private data */
 typedef struct InternalSnapshotState {
     BlkActionState common;

@@ -1941,8 +2114,10 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         assert(ops->instance_size > 0);
 
         state = g_malloc0(ops->instance_size);
+        state->refcount = 1;
         state->ops = ops;
         state->action = dev_info;
+        state->transaction = bts;
         QTAILQ_INSERT_TAIL(&bts->actions, state, entry);
 
         state->ops->prepare(state, &local_err);

@@ -1973,10 +2148,10 @@ exit:
         if (state->ops->clean) {
             state->ops->clean(state);
         }
-        QTAILQ_REMOVE(&bts->actions, state, entry);
-        g_free(state);
+        put_blk_action_state(state);
     }
 
+    /* The implicit 'job' of the transaction itself is complete: */
     transaction_job_complete(bts);
 }
 

index 2755465..9b3456f 100644
@@ -35,6 +35,19 @@
 #include "qemu/timer.h"
 #include "qapi-event.h"
 
+void block_job_incref(BlockJob *job)
+{
+    job->refcount++;
+}
+
+void block_job_decref(BlockJob *job)
+{
+    job->refcount--;
+    if (job->refcount == 0) {
+        g_free(job);
+    }
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)

@@ -57,6 +70,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     job->cb            = cb;
     job->opaque        = opaque;
     job->busy          = true;
+    job->refcount      = 1;
     bs->job = job;
 
     /* Only set speed when necessary to avoid NotSupported error */

@@ -68,7 +82,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
             bs->job = NULL;
             bdrv_op_unblock_all(bs, job->blocker);
             error_free(job->blocker);
-            g_free(job);
+            block_job_decref(job);
             error_propagate(errp, local_err);
             return NULL;
         }

@@ -85,7 +99,7 @@ void block_job_completed(BlockJob *job, int ret)
     bs->job = NULL;
     bdrv_op_unblock_all(bs, job->blocker);
     error_free(job->blocker);
-    g_free(job);
+    block_job_decref(job);
 }
 
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)

index 57d8ef1..86d770a 100644
@@ -122,6 +122,9 @@ struct BlockJob {
 
     /** The opaque value that is passed to the completion function.  */
     void *opaque;
+
+    /** A reference count, allowing for post-job actions in e.g. transactions */
+    int refcount;
 };
 
 /**

@@ -147,6 +150,24 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        void *opaque, Error **errp);
 
 /**
+ * block_job_incref:
+ * @job: The job to pick up a handle to
+ *
+ * Increment the refcount on @job, to be able to use it asynchronously
+ * from the job it is being used for. Put down the reference when done
+ * with @block_job_unref.
+ */
+void block_job_incref(BlockJob *job);
+
+/**
+ * block_job_decref:
+ * @job: The job to unreference and delete.
+ *
+ * Decrement the refcount on @job and delete it if there are no more references.
+ */
+void block_job_decref(BlockJob *job);
+
+/**
  * block_job_sleep_ns:
  * @job: The job that calls the function.
  * @clock: The clock to sleep on.

index ca5b1e9..d964564 100644
@@ -51,6 +51,12 @@
 #include <windows.h>
 #endif
 
+typedef enum BitmapSuccessorAction {
+    SUCCESSOR_ACTION_UNDEFINED = 0,
+    SUCCESSOR_ACTION_ABDICATE,
+    SUCCESSOR_ACTION_RECLAIM
+} BitmapSuccessorAction;
+
 /**
  * A BdrvDirtyBitmap can be in three possible states:
  * (1) successor is NULL and disabled is false: full r/w mode

@@ -65,6 +71,8 @@ struct BdrvDirtyBitmap {
     char *name;                 /* Optional non-empty unique ID */
     int64_t size;               /* Size of the bitmap (Number of sectors) */
     bool disabled;              /* Bitmap is read-only */
+    int successor_refcount;     /* Number of active handles to the successor */
+    BitmapSuccessorAction act;  /* Action to take on successor upon release */
     QLIST_ENTRY(BdrvDirtyBitmap) list;
 };
 

@@ -3133,6 +3141,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
 
     /* Install the successor and freeze the parent */
     bitmap->successor = child;
+    bitmap->successor_refcount = 1;
     return 0;
 }
 

@@ -3140,9 +3149,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
  * For a bitmap with a successor, yield our name to the successor,
  * delete the old bitmap, and return a handle to the new bitmap.
  */
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
-                                            Error **errp)
+static BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *bitmap,
+                                                   Error **errp)
 {
     char *name;
     BdrvDirtyBitmap *successor = bitmap->successor;

@@ -3167,9 +3176,9 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
  * we may wish to re-join the parent and child/successor.
  * The merged parent will be un-frozen, but not explicitly re-enabled.
  */
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *parent,
-                                           Error **errp)
+static BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
+                                                  BdrvDirtyBitmap *parent,
+                                                  Error **errp)
 {
     BdrvDirtyBitmap *successor = parent->successor;
 

@@ -3188,6 +3197,50 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
     return parent;
 }
 
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap *parent)
+{
+    assert(!parent->successor_refcount);
+
+    switch (parent->act) {
+    case SUCCESSOR_ACTION_RECLAIM:
+        return bdrv_reclaim_dirty_bitmap(bs, parent, NULL);
+    case SUCCESSOR_ACTION_ABDICATE:
+        return bdrv_dirty_bitmap_abdicate(bs, parent, NULL);
+    case SUCCESSOR_ACTION_UNDEFINED:
+    default:
+        g_assert_not_reached();
+    }
+}
+
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+                                           BdrvDirtyBitmap *parent,
+                                           int ret)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    if (ret) {
+        parent->act = SUCCESSOR_ACTION_RECLAIM;
+    } else if (parent->act != SUCCESSOR_ACTION_RECLAIM) {
+        parent->act = SUCCESSOR_ACTION_ABDICATE;
+    }
+
+    parent->successor_refcount--;
+    if (parent->successor_refcount == 0) {
+        return bdrv_free_bitmap_successor(bs, parent);
+    }
+    return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
 /**
  * Truncates _all_ bitmaps attached to a BDS.
  */

index d3f648d..4ac0be8 100644
@@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void *opaque)
 
     bdrv_unref(s->target);
 
+    if (s->sync_bitmap) {
+        BdrvDirtyBitmap *bm;
+        bm = bdrv_frozen_bitmap_decref(job->bs, s->sync_bitmap, data->ret);
+        assert(bm);
+    }
+
     block_job_completed(job, data->ret);
     g_free(data);
 }

@@ -428,18 +434,6 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_wrlock(&job->flush_rwlock);
     qemu_co_rwlock_unlock(&job->flush_rwlock);
 
-    if (job->sync_bitmap) {
-        BdrvDirtyBitmap *bm;
-        if (ret < 0) {
-            /* Merge the successor back into the parent, delete nothing. */
-            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        } else {
-            /* Everything is fine, delete this bitmap and install the backup. */
-            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        }
-    }
     hbitmap_free(job->bitmap);
 
     bdrv_iostatus_disable(target);

@@ -543,6 +537,6 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
 
  error:
     if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        bdrv_frozen_bitmap_decref(bs, sync_bitmap, -ECANCELED);
     }
 }

index 9f0ff3d..7c09903 100644
@@ -457,12 +457,10 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
                                        Error **errp);
-BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
-                                            BdrvDirtyBitmap *bitmap,
-                                            Error **errp);
-BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
-                                           BdrvDirtyBitmap *bitmap,
-                                           Error **errp);
+BdrvDirtyBitmap *bdrv_frozen_bitmap_decref(BlockDriverState *bs,
+                                           BdrvDirtyBitmap *parent,
+                                           int ret);
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                         const char *name);
 void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);

index 27db1b4..f391e18 100644
@@ -1794,6 +1794,19 @@ typedef struct DriveBackupState {
     BlockJob *job;
 } DriveBackupState;
 
+static void do_drive_backup(const char *device, const char *target,
+                            bool has_format, const char *format,
+                            enum MirrorSyncMode sync,
+                            bool has_mode, enum NewImageMode mode,
+                            bool has_speed, int64_t speed,
+                            bool has_bitmap, const char *bitmap,
+                            bool has_on_source_error,
+                            BlockdevOnError on_source_error,
+                            bool has_on_target_error,
+                            BlockdevOnError on_target_error,
+                            BlockCompletionFunc *cb, void *opaque,
+                            Error **errp);
+
 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
     DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);

@@ -1816,15 +1829,16 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     state->aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(state->aio_context);
 
-    qmp_drive_backup(backup->device, backup->target,
-                     backup->has_format, backup->format,
-                     backup->sync,
-                     backup->has_mode, backup->mode,
-                     backup->has_speed, backup->speed,
-                     backup->has_bitmap, backup->bitmap,
-                     backup->has_on_source_error, backup->on_source_error,
-                     backup->has_on_target_error, backup->on_target_error,
-                     &local_err);
+    do_drive_backup(backup->device, backup->target,
+                    backup->has_format, backup->format,
+                    backup->sync,
+                    backup->has_mode, backup->mode,
+                    backup->has_speed, backup->speed,
+                    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,
+                    &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;

@@ -2778,15 +2792,18 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_drive_backup(const char *device, const char *target,
-                      bool has_format, const char *format,
-                      enum MirrorSyncMode sync,
-                      bool has_mode, enum NewImageMode mode,
-                      bool has_speed, int64_t speed,
-                      bool has_bitmap, const char *bitmap,
-                      bool has_on_source_error, BlockdevOnError on_source_error,
-                      bool has_on_target_error, BlockdevOnError on_target_error,
-                      Error **errp)
+static void do_drive_backup(const char *device, const char *target,
+                            bool has_format, const char *format,
+                            enum MirrorSyncMode sync,
+                            bool has_mode, enum NewImageMode mode,
+                            bool has_speed, int64_t speed,
+                            bool has_bitmap, const char *bitmap,
+                            bool has_on_source_error,
+                            BlockdevOnError on_source_error,
+                            bool has_on_target_error,
+                            BlockdevOnError on_target_error,
+                            BlockCompletionFunc *cb, void *opaque,
+                            Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;

@@ -2900,9 +2917,16 @@ void qmp_drive_backup(const char *device, const char *target,
         }
     }
 
+    /* If we are not supplied with callback override info, use our defaults */
+    if (cb == NULL) {
+        cb = block_job_cb;
+    }
+    if (opaque == NULL) {
+        opaque = bs;
+    }
     backup_start(bs, target_bs, speed, sync, bmap,
                  on_source_error, on_target_error,
-                 block_job_cb, bs, &local_err);
+                 cb, opaque, &local_err);
     if (local_err != NULL) {
         bdrv_unref(target_bs);
         error_propagate(errp, local_err);

@@ -2913,6 +2937,22 @@ out:
     aio_context_release(aio_context);
 }
 
+void qmp_drive_backup(const char *device, const char *target,
+                      bool has_format, const char *format,
+                      enum MirrorSyncMode sync,
+                      bool has_mode, enum NewImageMode mode,
+                      bool has_speed, int64_t speed,
+                      bool has_bitmap, const char *bitmap,
+                      bool has_on_source_error, BlockdevOnError on_source_error,
+                      bool has_on_target_error, BlockdevOnError on_target_error,
+                      Error **errp)
+{
+    do_drive_backup(device, target, has_format, format, sync, has_mode, mode,
+                    has_speed, speed, has_bitmap, bitmap, has_on_source_error,
+                    on_source_error, has_on_target_error, on_target_error,
+                    NULL, NULL, errp);
+}
+
 BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 {
     return bdrv_named_nodes_list(errp);

index 4ac0be8..1634c88 100644
@@ -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_frozen_bitmap_decref(job->bs, s->sync_bitmap, ret);
+    }
+}
+
 static void backup_complete(BlockJob *job, void *opaque)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common);

113 lines not on this page · raw message

index 50355df..e345d62 100644
@@ -627,6 +627,14 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   BlockCompletionFunc *cb, void *opaque,
                   Error **errp);
 
+/*
+ * backup_transaction_complete:
+ * @job: The BackupBlockJob that was associated with a transaction.
+ * @ret: The collective return code for the entire transaction.
+ *       Expects zero for success, non-zero for an error otherwise.
+ */
+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);