block: incremental backup transactions

This adds two qmp commands to transactions.

block-dirty-bitmap-add allows you to create a bitmap simultaneously alongside a new full backup to accomplish a clean synchronization point.

block-dirty-bitmap-clear allows you to reset a bitmap back to as-if it were new, which can also be used alongside a full backup to accomplish a clean synchronization point.

Signed-off-by Fam Zheng <[email protected]>

Signed-off-by John Snow <[email protected]>

---
  blockdev.c       | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  qapi-schema.json |   6 +++-
  2 files changed, 106 insertions(+), 1 deletion(-)

blockdev.c +23 -0

index e0671be..5120af1 100644
@@ -1664,6 +1664,96 @@ static void blockdev_backup_clean(BlkTransactionState *common)
      }
  }
 
+typedef struct BlockDirtyBitmapState {
+    BlkTransactionState common;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+    AioContext *aio_context;
+    bool prepared;
+} BlockDirtyBitmapState;
+
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+                                           Error **errp)
+{
+    Error *local_err = NULL;
+    BlockDirtyBitmapAdd *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    action = common->action->block_dirty_bitmap_add;
+    /* AIO context taken within qmp_block_dirty_bitmap_add */
+    qmp_block_dirty_bitmap_add(action->node, action->name,
+                               action->has_granularity, action->granularity,
+                               &local_err);
+
+    if (!local_err) {

blockdev.c +13 -0

+                                             common, common);
+
+    action = common->action->block_dirty_bitmap_add;
+    /* Should not be able to fail: IF the bitmap was added via .prepare(),
+     * then the node reference and bitmap name must have been valid.
+     */
+    if (state->prepared) {
+        qmp_block_dirty_bitmap_remove(action->node, action->name, &error_abort);
+    }
+}
+
+static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+                                             Error **errp)

blockdev.c +5 -0

+    if (!state->bitmap) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {

blockdev.c +30 -0

+}
+
+static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    bdrv_clear_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_clear_clean(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (state->aio_context) {
+        aio_context_release(state->aio_context);
+    }
+}
+
  static void abort_prepare(BlkTransactionState *common, Error **errp)
  {
      error_setg(errp, "Transaction aborted using Abort action");

@@ -1704,6 +1794,17 @@ static const BdrvActionOps actions[] = {
          .abort = internal_snapshot_abort,
          .clean = internal_snapshot_clean,
      },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_add_prepare,
+        .abort = block_dirty_bitmap_add_abort,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_clear_prepare,
+        .commit = block_dirty_bitmap_clear_commit,
+        .clean = block_dirty_bitmap_clear_clean,
+    }
  };
 
  /*

qapi-schema.json +5 -1

index e16f8eb..12c61f3 100644
@@ -1332,6 +1332,8 @@
  # abort since 1.6
  # blockdev-snapshot-internal-sync since 1.7
  # blockdev-backup since 2.3
+# block-dirty-bitmap-add since 2.3
+# block-dirty-bitmap-clear since 2.3
  ##
  { 'union': 'TransactionAction',
    'data': {

@@ -1339,7 +1341,9 @@
         'drive-backup': 'DriveBackup',
         'blockdev-backup': 'BlockdevBackup',
         'abort': 'Abort',
-       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
+       'block-dirty-bitmap-clear': 'BlockDirtyBitmap'
     } }
 
  ##

Reviewed-by Max Reitz <[email protected]>

Signed-off-by John Snow <[email protected]>

---
 tests/qemu-iotests/124     | 49 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/124.out |  4 ++--
 2 files changed, 51 insertions(+), 2 deletions(-)

tests/qemu-iotests/124 +49 -0

index 1c07387..da0bf6d 100644
@@ -184,6 +184,55 @@ class TestIncrementalBackup(iotests.QMPTestCase):
         return True
 
 
+    def test_incremental_transaction(self):
+        '''Test: Verify backups made from transactionally created bitmaps.
+
+        Create a bitmap "before" VM execution begins, then create a second
+        bitmap AFTER writes have already occurred. Use transactions to create
+        a full backup and synchronize both bitmaps to this backup.
+        Create an incremental backup through both bitmaps and verify that
+        both backups match the full backup.
+        '''
+        bitmap0 = self.add_bitmap('bitmap0', 'drive0')
+        self.hmp_io_writes('drive0', (('0xab', 0, 512),
+                                      ('0xfe', '16M', '256k'),
+                                      ('0x64', '32736k', '64k')))
+        bitmap1 = self.add_bitmap('bitmap1', 'drive0')
+
+        result = self.vm.qmp('transaction', actions=[
+            {
+                'type': 'block-dirty-bitmap-clear',
+                'data': { 'node': 'drive0',
+                          'name': 'bitmap0' },
+            },
+            {
+                'type': 'block-dirty-bitmap-clear',
+                'data': { 'node': 'drive0',
+                          'name': 'bitmap1' },
+            },
+            {
+                'type': 'drive-backup',
+                'data': { 'device': 'drive0',
+                          'sync': 'full',
+                          'format': iotests.imgfmt,
+                          'target': self.full_bak },
+            }
+        ])
+        self.assert_qmp(result, 'return', {})
+        self.wait_until_completed()
+        self.files.append(self.full_bak)
+        self.check_full_backup()
+
+        self.hmp_io_writes('drive0', (('0x9a', 0, 512),
+                                      ('0x55', '8M', '352k'),
+                                      ('0x78', '15872k', '1M')))
+        # Both bitmaps should be in sync and create fully valid
+        # incremental backups
+        res1 = self.create_incremental(bitmap0)
+        res2 = self.create_incremental(bitmap1)
+        self.assertTrue(res1 and res2)
+
+
     def test_incremental_failure(self):
         '''Test: Verify backups made after a failure are correct.
 

tests/qemu-iotests/124.out +2 -2

index 89968f3..914e373 100644
@@ -1,5 +1,5 @@
-....
+.....
 ----------------------------------------------------------------------
-Ran 4 tests
+Ran 5 tests
 
 OK

Signature · 1 line

1.9.3

Quoted 6 lines · John Snow

Reviewed-by Max Reitz <[email protected]>

Signed-off-by John Snow <[email protected]>

---
  tests/qemu-iotests/124     | 49 ++++++++++++++++++++++++++++++++++++++++++++++
  tests/qemu-iotests/124.out |  4 ++--
  2 files changed, 51 insertions(+), 2 deletions(-)

This tests seems to fail locally:

. . .
+======================================================================
+FAIL: test_incremental_failure (__main__.TestIncrementalBackup)
+Test: Verify backups made after a failure are correct.
+----------------------------------------------------------------------
+Traceback (most recent call last):
+ File "124", line 331, in test_incremental_failure
+ self.create_incremental()
+ File "124", line 167, in create_incremental
+ return self.wait_incremental(bitmap, validate)
+ File "124", line 179, in wait_incremental
+ self.assert_qmp_absent(event, 'data/error')
+ File "/home/kashyapc/tinker-space/qemu-upstream/tests/qemu-iotests/iotests.py", line 282, in assert_qmp_absent
+ self.fail('path "%s" has value "%s"' % (path, str(result)))
+AssertionError: path "data/error" has value "Input/output error"
+
+======================================================================
+FAIL: test_transaction_failure (__main__.TestIncrementalBackup)
+Test: Verify backups made from a transaction that partially fails.
+----------------------------------------------------------------------
+Traceback (most recent call last):
+ File "124", line 448, in test_transaction_failure
+ self.wait_incremental(dr1bm0, 'drive1')
+ File "124", line 179, in wait_incremental
+ self.assert_qmp_absent(event, 'data/error')
+ File "/home/kashyapc/tinker-space/qemu-upstream/tests/qemu-iotests/iotests.py", line 282, in assert_qmp_absent
+ self.fail('path "%s" has value "%s"' % (path, str(result)))
+AssertionError: path "data/error" has value "Input/output error"
+

    ----------------------------------------------------------------------

. . .

Complete stderr of ./check -qcow2 here:

https://kashyapc.fedorapeople.org/virt/qemu-incremental-backup-tests/stderr-qemu-io-tests-qcow2-11MAR2015.txt

And, as per the other two failures of tests 051 and 061 (that you mentioned on #qemu yesterday), Kevin Wolf on IRC said:

051 was fixed, but the output has changed _again_. That change is from armbru_'s commit 7ee6c1e18. We need to update the reference output.

As for 061, Max sent a patch, but I think we need to fix qemu rather than updating the reference output there
Because the error message has become considerably worse

To test, I applied these two series to yesterday's QEMU git master:

[PATCH 00/11] block: incremental backup transactions [PATCH v2 00/17] block: transactionless incremental backup

So, I'm here (28 commits ahead of commit 3539bbb on master) after applying the patch series:

     $ git describe
     v2.2.0-1190-g41b7f5f

I need to try w/ today's git though, yet.

On 03/11/2015 08:11 AM, Kashyap Chamarthy wrote:

Quoted 11 lines

To test, I applied these two series to yesterday's QEMU git master:

[PATCH 00/11] block: incremental backup transactions [PATCH v2 00/17] block: transactionless incremental backup

So, I'm here (28 commits ahead of commit 3539bbb on master) after applying the patch series:

    $ git describe
    v2.2.0-1190-g41b7f5f

Quoted 2 lines

I need to try w/ today's git though, yet.

OK, I'll check this again after I make my changes to the base series this is based on. I promise I wasn't seeing a failure then ;)

If I can't reproduce I will be in touch to help diagnose what's going wrong.

The goal here is to add a new method to transactions that allows developers to specify a callback that will get invoked only once all jobs spawned by a transaction are completed, allowing developers the chance to perform actions conditionally pending complete success or complete failure.

In order to register the new callback to be invoked, a user must request a callback pointer and closure by calling new_transaction_wrapper, which creates a wrapper around a closure and callback that would originally have been passed to e.g. backup_start().

The function will return a function pointer and a new closure to be passed instead. The transaction system will effectively intercept these callbacks and execute the desired actions upon reception of all intercepted callbacks.

This means that the registered callbacks will be called after all other transaction actions that requested a callback have completed. The feature has no knowledge of jobs spawned without informing the BlkTransactionList.

For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an example for how to hook in drive_backup (or any block job launched by transactions).

Note 1: Defining a callback method alone is not sufficient to have the new
method invoked. You must call new_transaction_wrapper AND ensure the callback it returns to you is used as the callback for the job.

Note 2: You can use this feature for any system that registers completions of
an action via a callback of the form (void *opaque, int ret), not just block job callbacks.

Note 3: new_blk_transaction has no users in this patch, but will in
the next patch where it will become static and local to blockdev.c.

Signed-off-by John Snow <[email protected]>

---
  blockdev.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 223 insertions(+), 2 deletions(-)

blockdev.c +4 -0

index 5120af1..3153ee7 100644
@@ -1207,6 +1207,8 @@ static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *node,
  /* New and old BlockDriverState structs for atomic group operations */
 
  typedef struct BlkTransactionState BlkTransactionState;
+typedef struct BlkTransactionList BlkTransactionList;
+typedef struct BlkTransactionData BlkTransactionData;
 
  /* 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. */

@@ -1221,6 +1223,8 @@ typedef struct BdrvActionOps {
      void (*abort)(BlkTransactionState *common);
      /* Clean up resource in the end, can be NULL. */
      void (*clean)(BlkTransactionState *common);
+    /* Execute this after +all+ jobs in the transaction finish */
+    void (*cb)(BlkTransactionState *common);
  } BdrvActionOps;
 
  /*

@@ -1231,9 +1235,220 @@ typedef struct BdrvActionOps {
  struct BlkTransactionState {

blockdev.c +7 -0

+        blk_put_transaction_state(bts);
+    }
+
+    g_free(btl);
+}
+
+static void blk_run_transaction_callbacks(BlkTransactionList *btl)

blockdev.c +19 -0

+        blk_run_transaction_callbacks(btl);
+        del_blk_transaction_list(btl);
+        return NULL;
+    }
+    return btl;
+}
+
+static void blk_transaction_complete(BlkTransactionState *common)
+{
+    BlkTransactionList *btl = common->list;
+
+    /* Add this action into the pile to be completed */
+    QSIMPLEQ_INSERT_TAIL(&btl->actions, common, list_entry);
+
+    /* Inform the list that we have a completion;
+     * possibly run all the pending actions. */
+    put_blk_transaction_list(btl);
+}
+

blockdev.c +21 -0

+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.
+ *
+ * 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 transactional state to set a callback for.
+ * @opaque A closure intended for the encapsulated callback.
+ * @callback The callback we are encapsulating.

blockdev.c +31 -0

+    assert(new_opaque);
+
+    /* Stash the original callback information */
+    btw->opaque = opaque;
+    btw->callback = callback;
+    common->opaque = btw;
+
+    /* The BTS will serve as our new closure */
+    *new_opaque = common;
+    common->refcount++;
+
+    /* Inform the transaction BTL to expect one more return */
+    common->list->jobs++;
+
+    /* Lastly, the actual callback function to handle the interception. */
+    return transaction_callback;
+}
+
+/**
+ * Undo any actions performed by the above call.
+ */
+void undo_transaction_wrapper(BlkTransactionState *common)
+{
+    BlkTransactionList *btl = common->list;
+    BlkTransactionState *bts;
+    BlkTransactionData *btd;
+
+    /* Stage 0: Wrapper was never created: */
+    if (common->opaque == NULL && common->refcount == 1) {
+        return;
+    }

blockdev.c +4 -0

      },
      [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
          .instance_size = sizeof(BlockdevBackupState),

@@ -1815,10 +2030,12 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
  {
      TransactionActionList *dev_entry = dev_list;
      BlkTransactionState *state, *next;
+    BlkTransactionList *btl;
      Error *local_err = NULL;
 
      QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
      QSIMPLEQ_INIT(&snap_bdrv_states);
+    btl = new_blk_transaction_list();
 
      /* drain all i/o before any operations */
      bdrv_drain_all();

@@ -1837,8 +2054,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->list = btl;
          QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
 
          state->ops->prepare(state, &local_err);

@@ -1869,8 +2088,10 @@ exit:
          if (state->ops->clean) {

Quoted 83 lines · John Snow

The goal here is to add a new method to transactions that allows developers to specify a callback that will get invoked only once all jobs spawned by a transaction are completed, allowing developers the chance to perform actions conditionally pending complete success or complete failure.

In order to register the new callback to be invoked, a user must request a callback pointer and closure by calling new_transaction_wrapper, which creates a wrapper around a closure and callback that would originally have
been passed to e.g. backup_start().

The function will return a function pointer and a new closure to be passed
instead. The transaction system will effectively intercept these callbacks
and execute the desired actions upon reception of all intercepted callbacks.

This means that the registered callbacks will be called after all other transaction actions that requested a callback have completed. The feature has no knowledge of jobs spawned without informing the BlkTransactionList.

For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an example
for how to hook in drive_backup (or any block job launched by transactions).

Note 1: Defining a callback method alone is not sufficient to have the new
method invoked. You must call new_transaction_wrapper AND
ensure the
callback it returns to you is used as the callback for the job.

Note 2: You can use this feature for any system that registers completions of
an action via a callback of the form (void *opaque, int ret),
not just
block job callbacks.

Note 3: new_blk_transaction has no users in this patch, but will in
the next patch where it will become static and local to
blockdev.c.

Signed-off-by John Snow <[email protected]>

---
  blockdev.c | 225
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 223 insertions(+), 2 deletions(-)

blockdev.c +2 -0

index 5120af1..3153ee7 100644
@@ -1207,6 +1207,8 @@ static BdrvDirtyBitmap
 *block_dirty_bitmap_lookup(const char *node,
  /* New and old BlockDriverState structs for atomic group operations */
  typedef struct BlkTransactionState BlkTransactionState;
+typedef struct BlkTransactionList BlkTransactionList;
+typedef struct BlkTransactionData BlkTransactionData;
  /* 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. */

blockdev.c +5 -0

@@ -1221,6 +1223,8 @@ typedef struct BdrvActionOps {
      void (*abort)(BlkTransactionState *common);
      /* Clean up resource in the end, can be NULL. */
      void (*clean)(BlkTransactionState *common);
+    /* Execute this after +all+ jobs in the transaction finish */
+    void (*cb)(BlkTransactionState *common);
  } BdrvActionOps;
  /*

@@ -1231,9 +1235,220 @@ typedef struct BdrvActionOps {
  struct BlkTransactionState {
      TransactionAction *action;
      const BdrvActionOps *ops;
+    BlkTransactionList *list;
+    void *opaque;
+    /* Allow external users (callbacks) to reference this obj past

.clean() */

blockdev.c typedef struct BdrvActionOps { · line 1245

Quoted 5 lines

blockdev.c +4 -0

+    int refcount;
+    /* All transactions in the current group */
      QSIMPLEQ_ENTRY(BlkTransactionState) entry;
+    /* Transactions in the current group with callbacks */
+    QSIMPLEQ_ENTRY(BlkTransactionState) list_entry;

"list_entry" seems very generic and it's hard for me to see a fundamental difference to just "entry". How about "cb_entry", or "cb_list_entry", or "cb_group_entry" or something like that?

blockdev.c typedef struct BdrvActionOps { · line 1249

Quoted 3 lines

  };

blockdev.c +2 -0

+struct BlkTransactionList {
+    int jobs;     /* Effectively: A refcount */

Good to know, but I'd like the reason why it's called like this to be here anyway ("Number of jobs remaining" or I don't know).

Quoted 1 line

+ int status; /* Cumulative retcode */

The only places I currently find "retcode" in are linux-user/signal.c and tests/image-fuzzer/runner.py. How about the more common "return value" (or simply "cumulative status")?

("retcode" reads a bit like "retcon" to me, that's why I had to think about it for a second)

Quoted 7 lines

blockdev.c +7 -0

+    QSIMPLEQ_HEAD(actions, BlkTransactionState) actions;
+};
+
+typedef struct BlkTransactionData {
+    void *opaque; /* Data given to encapsulated callback */
+    int ret;      /* Return code given to encapsulated callback */
+} BlkTransactionData;

blockdev.c typedef struct BdrvActionOps { · line 1262

Quoted 5 lines

blockdev.c +5 -0

+
+typedef struct BlkTransactionWrapper {
+    void *opaque; /* Data to be given to encapsulated callback */
+    void (*callback)(void *opaque, int ret); /* Encapsulated callback */
+} BlkTransactionWrapper;

I find it pretty difficult to figure out what these objects are each for. Care to add comments for that, too?

blockdev.c typedef struct BdrvActionOps { · line 1266

Quoted 4 lines

blockdev.c +4 -0

+
+static BlkTransactionList *new_blk_transaction_list(void)
+{
+    BlkTransactionList *btl = g_malloc0(sizeof(*btl));

Maybe use g_new0(BlkTransactionList, 1); (It's typesafe! And who doesn't love typesafety?).

(Optional, though, the foo = malloc(sizeof(*foo)) pattern is pretty wide-spread, and as far as I remember, Markus purposely did not replace it by foo = g_new(Foo, 1))

blockdev.c typedef struct BdrvActionOps { · line 1269

Quoted 3 lines

blockdev.c +3 -0

+
+    /* Implicit 'job' for qmp_transaction itself */
+    btl->jobs = 1;

Well, if it is a refcount, just call it that way...

(Update: Okay, I see why you didn't call it that way. Maybe the comment needs improvement, like "The transaction itself is a job, too, that needs to be completed before the callbacks are called")

blockdev.c typedef struct BdrvActionOps { · line 1278

Quoted 10 lines

blockdev.c +5 -0

+    QSIMPLEQ_INIT(&btl->actions);
+    return btl;
+}
+
+static BlkTransactionState

*blk_put_transaction_state(BlkTransactionState *bts)

blockdev.c +4 -0

+{
+    bts->refcount--;
+    if (bts->refcount == 0) {
+        g_free(bts);

How about removing it from the lists it's in? Doesn't appear to be necessary, but I'd find it cleaner.

Quoted 11 lines

blockdev.c +11 -0

+        return NULL;
+    }
+    return bts;
+}
+
+static void del_blk_transaction_list(BlkTransactionList *btl)
+{
+    BlkTransactionState *bts, *bts_next;
+
+    /* The list should in normal cases be empty,
+     * but in case someone really just wants to kibosh the whole

Quoted 1 line

deal: */

Thank you for teaching me a new word.

blockdev.c

Quoted 2 lines

blockdev.c +2 -0

+    QSIMPLEQ_FOREACH_SAFE(bts, &btl->actions, list_entry, bts_next) {
+        g_free(bts->opaque);

Urp... Are you sure? :-/

I'd rather have some callback for destroying the object... Apparently this will (for now) be always useless overhead, because that callback is only calling g_free(), but it's an opaque pointer, so it's not really up to you to do anything with it other than passing it around.

Quoted 13 lines

blockdev.c +13 -0

+        blk_put_transaction_state(bts);
+    }
+
+    g_free(btl);
+}
+
+static void blk_run_transaction_callbacks(BlkTransactionList *btl)
+{
+    BlkTransactionState *bts, *bts_next;
+
+    QSIMPLEQ_FOREACH_SAFE(bts, &btl->actions, list_entry, bts_next) {
+        if (bts->ops->cb) {
+            bts->ops->cb(bts);

blockdev.c typedef struct BdrvActionOps { · line 1308

Quoted 4 lines

blockdev.c +4 -0

+        }
+
+        /* Free the BlkTransactionData */
+        g_free(bts->opaque);

Again... Urp. If you know it's a BlkTransactionData object, please make it a BlkTransactionData pointer and don't call it "opaque" if it's not so opaque in the end.

blockdev.c typedef struct BdrvActionOps { · line 1319

Quoted 12 lines

blockdev.c +8 -0

+        bts->opaque = NULL;
+        blk_put_transaction_state(bts);
+    }
+
+    QSIMPLEQ_INIT(&btl->actions);
+}
+
+static BlkTransactionList

*put_blk_transaction_list(BlkTransactionList *btl)

blockdev.c +3 -0

+{
+    btl->jobs--;
+    if (btl->jobs == 0) {

Okay, this is interesting, because on the one hand it makes it clear why you did not call it "refcount", while on the other hand this is clearly a pattern which looks very much like handling a refcount.

Maybe you could call the function differently, like "blk_transaction_list_job_completed()" or something. It's longer, but I think it reduces the change of people staring at this thinking "Why is 'jobs' actually a refcount?".

Quoted 19 lines

blockdev.c +19 -0

+        blk_run_transaction_callbacks(btl);
+        del_blk_transaction_list(btl);
+        return NULL;
+    }
+    return btl;
+}
+
+static void blk_transaction_complete(BlkTransactionState *common)
+{
+    BlkTransactionList *btl = common->list;
+
+    /* Add this action into the pile to be completed */
+    QSIMPLEQ_INSERT_TAIL(&btl->actions, common, list_entry);
+
+    /* Inform the list that we have a completion;
+     * possibly run all the pending actions. */
+    put_blk_transaction_list(btl);
+}
+

blockdev.c typedef struct BdrvActionOps { · line 1348

Quoted 10 lines

blockdev.c +10 -0

+/**
+ * Intercept a callback that was issued due to a transactional action.
+ */
+static void transaction_callback(void *opaque, int ret)
+{
+    BlkTransactionState *common = opaque;
+    BlkTransactionWrapper *btw = common->opaque;
+
+    /* Prepare data for ops->cb() */
+    BlkTransactionData *btd = g_malloc0(sizeof(*btd));

g_new0(BlkTransactionData, 1);

blockdev.c typedef struct BdrvActionOps { · line 1353

Quoted 5 lines

blockdev.c +5 -0

+    btd->opaque = btw->opaque;
+    btd->ret = ret;
+
+    /* Transaction state now tracks OUR data */
+    common->opaque = btd;

Sorry, but I really have a hard time following this opaqueness... Again, if you can, please clarify what the objects are for, and it would be very nice to separate truly opaque objects from these internally used objects (which are managed by you and thus are to be managed by you (g_free()), because reusing void * pointers for different kinds of objects like this makes my brain go strawberry.

blockdev.c typedef struct BdrvActionOps { · line 1356

Quoted 3 lines

blockdev.c +3 -0

+
+    /* Keep track of the amalgamated return code */
+    common->list->status |= ret;

Hm, I guess you're expecting ret to be -errno. In that case, you'd probably rather want "if (ret && (!common->list->status || common->list->status == -ENOSPC)) { common->list->status = ret; }" or something like that, because bit-OR-ing different -errno values will probably not turn out so great.

blockdev.c typedef struct BdrvActionOps { · line 1366

Quoted 10 lines

blockdev.c +10 -0

+
+    /* Deliver the intercepted callback FIRST */
+    btw->callback(btw->opaque, ret);
+    blk_transaction_complete(common);
+    g_free(btw);
+}
+
+typedef void (CallbackFn)(void *opaque, int ret);
+
+/* Temporary. Removed in the next patch. */

Actually, no. :-)

(remove in patch 7)

Why are you making them non-static in the first place? I see both functions mentioned in patch 3 and patch 7 only (except for context in patch 6).

Quoted 21 lines

blockdev.c +21 -0

+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.
+ *
+ * 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 transactional state to set a callback for.
+ * @opaque A closure intended for the encapsulated callback.
+ * @callback The callback we are encapsulating.

blockdev.c typedef struct BdrvActionOps { · line 1397

Quoted 10 lines

blockdev.c +10 -0

+ * @new_opaque The closure to be used instead of @opaque.
+ *
+ * @return The callback to be used instead of @callback.
+ */
+CallbackFn *new_transaction_wrapper(BlkTransactionState *common,
+                                           void *opaque,
+                                           CallbackFn *callback,
+                                           void **new_opaque)
+{
+    BlkTransactionWrapper *btw = g_malloc0(sizeof(*btw));

g_new0(BlkTransactionWrapper, 1);

Quoted 37 lines

blockdev.c +15 -0

+    assert(new_opaque);
+
+    /* Stash the original callback information */
+    btw->opaque = opaque;
+    btw->callback = callback;
+    common->opaque = btw;
+
+    /* The BTS will serve as our new closure */
+    *new_opaque = common;
+    common->refcount++;
+
+    /* Inform the transaction BTL to expect one more return */
+    common->list->jobs++;
+
+    /* Lastly, the actual callback function to handle the

interception. */

blockdev.c +19 -0

+    return transaction_callback;
+}
+
+/**
+ * Undo any actions performed by the above call.
+ */
+void undo_transaction_wrapper(BlkTransactionState *common)
+{
+    BlkTransactionList *btl = common->list;
+    BlkTransactionState *bts;
+    BlkTransactionData *btd;
+
+    /* Stage 0: Wrapper was never created: */
+    if (common->opaque == NULL && common->refcount == 1) {
+        return;
+    }
+
+    /* 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.

blockdev.c typedef struct BdrvActionOps { · line 1438

Quoted 6 lines

blockdev.c +6 -0

+     */
+    QSIMPLEQ_FOREACH(bts, &btl->actions, list_entry) {
+        if (bts == common) {
+            btd = common->opaque;
+            /* Force error for callback */
+            btd->ret = -1;

No -errno?

blockdev.c typedef struct BdrvActionOps { · line 1448

Quoted 10 lines

blockdev.c +10 -0

+            common->ops->cb(common);
+            QSIMPLEQ_REMOVE(&btl->actions, common,
+                            BlkTransactionState, list_entry);
+            goto cleanup;
+        }
+    }
+    /* Stage 1: Callback created, but job never launched */
+    put_blk_transaction_list(common->list);
+ cleanup:
+    g_free(common->opaque);

urp

Quoted 6 lines

blockdev.c +3 -0

+    blk_put_transaction_state(common);
+}
+
  /* internal snapshot private data */
  typedef struct InternalSnapshotState {
      BlkTransactionState common;

blockdev.c static const BdrvActionOps actions[] = { · line 1993

Quoted 6 lines

blockdev.c +1 -1

@@ -1775,7 +1990,7 @@ static const BdrvActionOps actions[] = {
          .instance_size = sizeof(DriveBackupState),
          .prepare = drive_backup_prepare,
          .abort = drive_backup_abort,
-        .clean = drive_backup_clean,
+        .clean = drive_backup_clean

Probably not intended, I guess.

Quoted 27 lines

blockdev.c +4 -0

      },
      [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
          .instance_size = sizeof(BlockdevBackupState),

@@ -1815,10 +2030,12 @@ void qmp_transaction(TransactionActionList
 *dev_list, Error **errp)
  {
      TransactionActionList *dev_entry = dev_list;
      BlkTransactionState *state, *next;
+    BlkTransactionList *btl;
      Error *local_err = NULL;
      QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState)
 snap_bdrv_states;
      QSIMPLEQ_INIT(&snap_bdrv_states);
+    btl = new_blk_transaction_list();
      /* drain all i/o before any operations */
      bdrv_drain_all();

@@ -1837,8 +2054,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->list = btl;
          QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
          state->ops->prepare(state, &local_err);

blockdev.c exit: · line 2095

Quoted 9 lines

blockdev.c +3 -1

@@ -1869,8 +2088,10 @@ exit:
          if (state->ops->clean) {
              state->ops->clean(state);
          }
-        g_free(state);
+        blk_put_transaction_state(state);
      }
+
+    put_blk_transaction_list(btl);
  }

Sorry, as I said, I need more context on the objects and very much would like a separation between truly opaque pointers and not-so-opaque pointers, before I can really understand what's going on (or I put more effort into it, but since it's always more probable to belong to the majority, I guess I won't be the only one getting confused).

Maybe the next patches will clear it up, I don't know.

Max

Quoted 8 lines

blockdev.c typedef struct BdrvActionOps { · line 1366

Quoted 3 lines

blockdev.c +3 -0

+typedef void (CallbackFn)(void *opaque, int ret);
+
+/* Temporary. Removed in the next patch. */

Actually, no. :-)

(remove in patch 7)

Quoted 4 lines

Why are you making them non-static in the first place? I see both functions mentioned in patch 3 and patch 7 only (except for context in patch 6).

Won't compile otherwise, because they have no users as of this patch. I am trying to keep the size of these patches down and in a sane order.

Quoted 13 lines · John Snow

Quoted 12 lines

blockdev.c typedef struct BdrvActionOps { · line 1366

Quoted 3 lines

blockdev.c +3 -0

+typedef void (CallbackFn)(void *opaque, int ret);
+
+/* Temporary. Removed in the next patch. */

Actually, no. :-)

(remove in patch 7)

Why are you making them non-static in the first place? I see both functions mentioned in patch 3 and patch 7 only (except for context in patch 6).

Quoted 2 lines

Won't compile otherwise, because they have no users as of this patch. I am trying to keep the size of these patches down and in a sane order.

Another solution is to mark it static, but also add __attribute__((unused)) to shut the compiler up, so that you don't cause build failures, but also don't cause reviewers to wonder who is using it. Then when it finally gets used, a patch to remove the attribute is easier to see than a patch adding static.

Quoted 96 lines · Max Reitz

Quoted 85 lines · John Snow

The goal here is to add a new method to transactions that allows developers to specify a callback that will get invoked only once all jobs spawned by a transaction are completed, allowing developers the chance to perform actions conditionally pending complete success or complete failure.

In order to register the new callback to be invoked, a user must request a callback pointer and closure by calling new_transaction_wrapper, which creates a wrapper around a closure and callback that would originally have
been passed to e.g. backup_start().

The function will return a function pointer and a new closure to be passed
instead. The transaction system will effectively intercept these callbacks
and execute the desired actions upon reception of all intercepted callbacks.

This means that the registered callbacks will be called after all other transaction actions that requested a callback have completed. The feature has no knowledge of jobs spawned without informing the BlkTransactionList.

For an example of how to use the feature, please skip ahead to:
'block: drive_backup transaction callback support' which serves as an example
for how to hook in drive_backup (or any block job launched by transactions).

Note 1: Defining a callback method alone is not sufficient to have the new
method invoked. You must call new_transaction_wrapper AND
ensure the
callback it returns to you is used as the callback for the
job.

Note 2: You can use this feature for any system that registers completions of
an action via a callback of the form (void *opaque, int ret),
not just
block job callbacks.

Note 3: new_blk_transaction has no users in this patch, but will in
the next patch where it will become static and local to
blockdev.c.

Signed-off-by John Snow <[email protected]>

---
  blockdev.c | 225
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 223 insertions(+), 2 deletions(-)

blockdev.c +0 -0

index 5120af1..3153ee7 100644
@@ -1207,6 +1207,8 @@ static BdrvDirtyBitmap
 *block_dirty_bitmap_lookup(const char *node,
  /* New and old BlockDriverState structs for atomic group 

operations */
typedef struct BlkTransactionState BlkTransactionState;

blockdev.c +2 -0

+typedef struct BlkTransactionList BlkTransactionList;
+typedef struct BlkTransactionData BlkTransactionData;
  /* 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. */

blockdev.c +5 -0

@@ -1221,6 +1223,8 @@ typedef struct BdrvActionOps {
      void (*abort)(BlkTransactionState *common);
      /* Clean up resource in the end, can be NULL. */
      void (*clean)(BlkTransactionState *common);
+    /* Execute this after +all+ jobs in the transaction finish */
+    void (*cb)(BlkTransactionState *common);
  } BdrvActionOps;
  /*

@@ -1231,9 +1235,220 @@ typedef struct BdrvActionOps {
  struct BlkTransactionState {
      TransactionAction *action;
      const BdrvActionOps *ops;
+    BlkTransactionList *list;
+    void *opaque;
+    /* Allow external users (callbacks) to reference this obj past

.clean() */

blockdev.c typedef struct BdrvActionOps { · line 1245

Quoted 5 lines

blockdev.c +4 -0

+    int refcount;
+    /* All transactions in the current group */
      QSIMPLEQ_ENTRY(BlkTransactionState) entry;
+    /* Transactions in the current group with callbacks */
+    QSIMPLEQ_ENTRY(BlkTransactionState) list_entry;

"list_entry" seems very generic and it's hard for me to see a fundamental difference to just "entry". How about "cb_entry", or "cb_list_entry", or "cb_group_entry" or something like that?

Quoted 7 lines

blockdev.c typedef struct BdrvActionOps { · line 1249

Quoted 3 lines

  };

blockdev.c +2 -0

+struct BlkTransactionList {
+    int jobs;     /* Effectively: A refcount */

Good to know, but I'd like the reason why it's called like this to be here anyway ("Number of jobs remaining" or I don't know).

Yes, it's basically a refcount where the references are held by jobs.

Quoted 9 lines

Quoted 1 line

+ int status; /* Cumulative retcode */

The only places I currently find "retcode" in are linux-user/signal.c and tests/image-fuzzer/runner.py. How about the more common "return value" (or simply "cumulative status")?

("retcode" reads a bit like "retcon" to me, that's why I had to think about it for a second)

Sure.

Quoted 8 lines

Quoted 8 lines

blockdev.c +8 -0

+    QSIMPLEQ_HEAD(actions, BlkTransactionState) actions;
+};
+
+typedef struct BlkTransactionData {
+    void *opaque; /* Data given to encapsulated callback */
+    int ret;      /* Return code given to encapsulated callback */
+} BlkTransactionData;
+

Quoted 9 lines

Quoted 5 lines

blockdev.c +3 -0

+typedef struct BlkTransactionWrapper {
+    void *opaque; /* Data to be given to encapsulated callback */
+    void (*callback)(void *opaque, int ret); /* Encapsulated 

callback */
+} BlkTransactionWrapper;

I find it pretty difficult to figure out what these objects are each for. Care to add comments for that, too?

Will do.

Quoted 16 lines

blockdev.c typedef struct BdrvActionOps { · line 1266

Quoted 4 lines

blockdev.c +4 -0

+
+static BlkTransactionList *new_blk_transaction_list(void)
+{
+    BlkTransactionList *btl = g_malloc0(sizeof(*btl));

Maybe use g_new0(BlkTransactionList, 1); (It's typesafe! And who doesn't love typesafety?).

(Optional, though, the foo = malloc(sizeof(*foo)) pattern is pretty wide-spread, and as far as I remember, Markus purposely did not replace it by foo = g_new(Foo, 1))

blockdev.c typedef struct BdrvActionOps { · line 1269

Quoted 3 lines

blockdev.c +3 -0

+
+    /* Implicit 'job' for qmp_transaction itself */
+    btl->jobs = 1;

Quoted 6 lines

Well, if it is a refcount, just call it that way...

(Update: Okay, I see why you didn't call it that way. Maybe the comment needs improvement, like "The transaction itself is a job, too, that needs to be completed before the callbacks are called")

That's exactly the trick in play, here.

Quoted 52 lines

blockdev.c typedef struct BdrvActionOps { · line 1278

Quoted 10 lines

+ QSIMPLEQ_INIT(&btl->actions);

blockdev.c +4 -0

+    return btl;
+}
+
+static BlkTransactionState

*blk_put_transaction_state(BlkTransactionState *bts)

blockdev.c +4 -0

+{
+    bts->refcount--;
+    if (bts->refcount == 0) {
+        g_free(bts);

How about removing it from the lists it's in? Doesn't appear to be necessary, but I'd find it cleaner.

Quoted 11 lines

blockdev.c +11 -0

+        return NULL;
+    }
+    return bts;
+}
+
+static void del_blk_transaction_list(BlkTransactionList *btl)
+{
+    BlkTransactionState *bts, *bts_next;
+
+    /* The list should in normal cases be empty,
+     * but in case someone really just wants to kibosh the whole

Quoted 1 line

deal: */

Thank you for teaching me a new word.

blockdev.c

Quoted 2 lines

blockdev.c +2 -0

+    QSIMPLEQ_FOREACH_SAFE(bts, &btl->actions, list_entry, bts_next) {
+        g_free(bts->opaque);

Urp... Are you sure? :-/

I'd rather have some callback for destroying the object... Apparently this will (for now) be always useless overhead, because that callback is only calling g_free(), but it's an opaque pointer, so it's not really up to you to do anything with it other than passing it around.

Quoted 13 lines

blockdev.c +13 -0

+        blk_put_transaction_state(bts);
+    }
+
+    g_free(btl);
+}
+
+static void blk_run_transaction_callbacks(BlkTransactionList *btl)
+{
+    BlkTransactionState *bts, *bts_next;
+
+    QSIMPLEQ_FOREACH_SAFE(bts, &btl->actions, list_entry, bts_next) {
+        if (bts->ops->cb) {
+            bts->ops->cb(bts);

Quoted 9 lines

blockdev.c typedef struct BdrvActionOps { · line 1308

Quoted 4 lines

blockdev.c +4 -0

+        }
+
+        /* Free the BlkTransactionData */
+        g_free(bts->opaque);

Again... Urp. If you know it's a BlkTransactionData object, please make it a BlkTransactionData pointer and don't call it "opaque" if it's not so opaque in the end.

A good point. I use this "opaque" pointer to actually hold two very specific and knowable things; it's just that those things change during the lifetime of the object.

For whatever reason, I decided it was nicer to not have two pointers that are never used simultaneously.

I can either make two dedicated fields, or just introduce it as an union. The state of the object otherwise dictates which union field to access.

Because I am a complicated individual, I am thinking fondly of the union right now.

Quoted 18 lines

blockdev.c typedef struct BdrvActionOps { · line 1319

Quoted 13 lines

blockdev.c +8 -0

+        bts->opaque = NULL;
+        blk_put_transaction_state(bts);
+    }
+
+    QSIMPLEQ_INIT(&btl->actions);
+}
+
+static BlkTransactionList

*put_blk_transaction_list(BlkTransactionList *btl)

blockdev.c +3 -0

+{
+    btl->jobs--;
+    if (btl->jobs == 0) {

Okay, this is interesting, because on the one hand it makes it clear why you did not call it "refcount", while on the other hand this is clearly a pattern which looks very much like handling a refcount.

Quoted 5 lines

Maybe you could call the function differently, like "blk_transaction_list_job_completed()" or something. It's longer, but I think it reduces the change of people staring at this thinking "Why is 'jobs' actually a refcount?".

OK, I'm up for suggestions on naming. I didn't really (deep in my heart) expect v1 to be perfect.

I think your main problem here is "jobs" as an integral refcount. Maybe "num_jobs" or "jobs_remaining" etc would make it clearer.

And then, sure, "blk_transaction_list_job_completed" would be a fine alternative to "put."

Quoted 38 lines

Quoted 19 lines

+ blk_run_transaction_callbacks(btl);

blockdev.c +18 -0

+        del_blk_transaction_list(btl);
+        return NULL;
+    }
+    return btl;
+}
+
+static void blk_transaction_complete(BlkTransactionState *common)
+{
+    BlkTransactionList *btl = common->list;
+
+    /* Add this action into the pile to be completed */
+    QSIMPLEQ_INSERT_TAIL(&btl->actions, common, list_entry);
+
+    /* Inform the list that we have a completion;
+     * possibly run all the pending actions. */
+    put_blk_transaction_list(btl);
+}
+

blockdev.c typedef struct BdrvActionOps { · line 1348

Quoted 10 lines

blockdev.c +10 -0

+/**
+ * Intercept a callback that was issued due to a transactional action.
+ */
+static void transaction_callback(void *opaque, int ret)
+{
+    BlkTransactionState *common = opaque;
+    BlkTransactionWrapper *btw = common->opaque;
+
+    /* Prepare data for ops->cb() */
+    BlkTransactionData *btd = g_malloc0(sizeof(*btd));

g_new0(BlkTransactionData, 1);

blockdev.c typedef struct BdrvActionOps { · line 1353

Quoted 5 lines

blockdev.c +5 -0

+    btd->opaque = btw->opaque;
+    btd->ret = ret;
+
+    /* Transaction state now tracks OUR data */
+    common->opaque = btd;

Quoted 7 lines

Sorry, but I really have a hard time following this opaqueness... Again, if you can, please clarify what the objects are for, and it would be very nice to separate truly opaque objects from these internally used objects (which are managed by you and thus are to be managed by you (g_free()), because reusing void * pointers for different kinds of objects like this makes my brain go strawberry.

I'll shoot for Raspberry in v2.

Quoted 4 lines

Quoted 3 lines

blockdev.c +3 -0

+
+    /* Keep track of the amalgamated return code */
+    common->list->status |= ret;

Quoted 6 lines

Hm, I guess you're expecting ret to be -errno. In that case, you'd probably rather want "if (ret && (!common->list->status || common->list->status == -ENOSPC)) { common->list->status = ret; }" or something like that, because bit-OR-ing different -errno values will probably not turn out so great.

I am only super interested in keeping a zero-or-nonzero cumulative status -- the actual error code winds up not being so important.

The mechanisms here allow the delivery of both the "cumulative" and the individual return code to the transactional callback, so error messages etc. can fill in the blanks if they need to.

Quoted 13 lines

blockdev.c typedef struct BdrvActionOps { · line 1366

Quoted 10 lines

blockdev.c +10 -0

+
+    /* Deliver the intercepted callback FIRST */
+    btw->callback(btw->opaque, ret);
+    blk_transaction_complete(common);
+    g_free(btw);
+}
+
+typedef void (CallbackFn)(void *opaque, int ret);
+
+/* Temporary. Removed in the next patch. */

Actually, no. :-)

Quoted 6 lines

(remove in patch 7)

Why are you making them non-static in the first place? I see both functions mentioned in patch 3 and patch 7 only (except for context in patch 6).

Won't compile otherwise, because they have no users as of this patch. I am trying to keep the size of these patches down and in a sane order.

Once a user is added, we can re-add the static attribute.

Quoted 71 lines

Quoted 21 lines

blockdev.c +21 -0

+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.
+ *
+ * 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 transactional state to set a callback for.
+ * @opaque A closure intended for the encapsulated callback.
+ * @callback The callback we are encapsulating.

blockdev.c typedef struct BdrvActionOps { · line 1397

Quoted 10 lines

blockdev.c +10 -0

+ * @new_opaque The closure to be used instead of @opaque.
+ *
+ * @return The callback to be used instead of @callback.
+ */
+CallbackFn *new_transaction_wrapper(BlkTransactionState *common,
+                                           void *opaque,
+                                           CallbackFn *callback,
+                                           void **new_opaque)
+{
+    BlkTransactionWrapper *btw = g_malloc0(sizeof(*btw));

g_new0(BlkTransactionWrapper, 1);

Quoted 37 lines

blockdev.c +15 -0

+    assert(new_opaque);
+
+    /* Stash the original callback information */
+    btw->opaque = opaque;
+    btw->callback = callback;
+    common->opaque = btw;
+
+    /* The BTS will serve as our new closure */
+    *new_opaque = common;
+    common->refcount++;
+
+    /* Inform the transaction BTL to expect one more return */
+    common->list->jobs++;
+
+    /* Lastly, the actual callback function to handle the

interception. */

blockdev.c +19 -0

+    return transaction_callback;
+}
+
+/**
+ * Undo any actions performed by the above call.
+ */
+void undo_transaction_wrapper(BlkTransactionState *common)
+{
+    BlkTransactionList *btl = common->list;
+    BlkTransactionState *bts;
+    BlkTransactionData *btd;
+
+    /* Stage 0: Wrapper was never created: */
+    if (common->opaque == NULL && common->refcount == 1) {
+        return;
+    }
+
+    /* 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.

Quoted 9 lines

blockdev.c typedef struct BdrvActionOps { · line 1438

Quoted 6 lines

blockdev.c +6 -0

+     */
+    QSIMPLEQ_FOREACH(bts, &btl->actions, list_entry) {
+        if (bts == common) {
+            btd = common->opaque;
+            /* Force error for callback */
+            btd->ret = -1;

No -errno?

Hmm. I suppose I should emulate one. What errno should I emulate for "One of your siblings died, and now you also need to take a dirtnap?"

Quoted 12 lines

blockdev.c typedef struct BdrvActionOps { · line 1448

Quoted 11 lines

+ common->ops->cb(common);

blockdev.c +9 -0

+            QSIMPLEQ_REMOVE(&btl->actions, common,
+                            BlkTransactionState, list_entry);
+            goto cleanup;
+        }
+    }
+    /* Stage 1: Callback created, but job never launched */
+    put_blk_transaction_list(common->list);
+ cleanup:
+    g_free(common->opaque);

Quoted 2 lines

urp

ok, ok! I'm sorry! ... kind of ...

Quoted 6 lines

Quoted 6 lines

blockdev.c +3 -0

+    blk_put_transaction_state(common);
+}
+
  /* internal snapshot private data */
  typedef struct InternalSnapshotState {
      BlkTransactionState common;

Quoted 9 lines

blockdev.c static const BdrvActionOps actions[] = { · line 1993

Quoted 6 lines

blockdev.c +1 -1

@@ -1775,7 +1990,7 @@ static const BdrvActionOps actions[] = {
          .instance_size = sizeof(DriveBackupState),
          .prepare = drive_backup_prepare,
          .abort = drive_backup_abort,
-        .clean = drive_backup_clean,
+        .clean = drive_backup_clean

Probably not intended, I guess.

Did an earlier patch of mine goof this up? The way this patch leaves it looks correct.

Quoted 44 lines

Quoted 28 lines

blockdev.c +4 -0

      },
      [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
          .instance_size = sizeof(BlockdevBackupState),

@@ -1815,10 +2030,12 @@ void qmp_transaction(TransactionActionList
 *dev_list, Error **errp)
  {
      TransactionActionList *dev_entry = dev_list;
      BlkTransactionState *state, *next;
+    BlkTransactionList *btl;
      Error *local_err = NULL;
      QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState)
 snap_bdrv_states;
      QSIMPLEQ_INIT(&snap_bdrv_states);
+    btl = new_blk_transaction_list();
      /* drain all i/o before any operations */
      bdrv_drain_all();

@@ -1837,8 +2054,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->list = btl;
          QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
          state->ops->prepare(state, &local_err);

blockdev.c exit: · line 2095

Quoted 9 lines

blockdev.c +3 -1

@@ -1869,8 +2088,10 @@ exit:
          if (state->ops->clean) {
              state->ops->clean(state);
          }
-        g_free(state);
+        blk_put_transaction_state(state);
      }
+
+    put_blk_transaction_list(btl);
  }

Sorry, as I said, I need more context on the objects and very much would like a separation between truly opaque pointers and not-so-opaque pointers, before I can really understand what's going on (or I put more effort into it, but since it's always more probable to belong to the majority, I guess I won't be the only one getting confused).

Quoted 3 lines

Maybe the next patches will clear it up, I don't know.

Max

I'll put some more elbow grease into explaining the flow.

If we want to get at the job after the life of the job, we'll need a refcount for this object.

This may occur for example if we wish to inspect the actions taken by a particular job after a transactional group of jobs runs, and further actions are required.

Signed-off-by John Snow <[email protected]>

---
  blockjob.c               | 18 ++++++++++++++++--
  include/block/blockjob.h | 21 +++++++++++++++++++++
  2 files changed, 37 insertions(+), 2 deletions(-)

blockjob.c +16 -2

index ba2255d..d620082 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)

include/block/blockjob.h +3 -0

index b6d4ebb..8fe1c95 100644
@@ -116,6 +116,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;
  };
 
  /**

include/block/blockjob.h +4 -0

+ */
+void block_job_decref(BlockJob *job);
+
+/**
   * block_job_sleep_ns:
   * @job: The job that calls the function.
   * @clock: The clock to sleep on.

Allow bitmap successors to carry reference counts.

We can in a later patch use this ability to clean up the dirty bitmap according to both the individual job's success and the success of all jobs in the transaction group.

The code for cleaning up a bitmap is also moved from backup_run to backup_complete.

Signed-off-by John Snow <[email protected]>

---
  block.c               | 79 +++++++++++++++++++++++++++++++++++++++++++++++----
  block/backup.c        | 23 ++++++---------
  include/block/block.h | 11 ++++---
  3 files changed, 87 insertions(+), 26 deletions(-)

block.c +0 -0

index 5eaa874..a0036af 100644

block.c +10 -6

+
  /**
   * A BdrvDirtyBitmap can be in three possible states:
   * (1) successor is false 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;
  };
 

@@ -5508,6 +5516,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;
  }
 

@@ -5515,9 +5524,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;

@@ -5542,9 +5551,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;
 

block.c +6 -0

+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);

block.c +12 -0

+    }
+    return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
  static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size)
  {
      /* Should only be frozen during a block backup job, which should have

block/backup.c +0 -0

index 41bd9af..4332df4 100644

block/backup.c +2 -0

+    }
+
      block_job_completed(job, data->ret);
      g_free(data);
  }

block/backup.c +3 -7

-        } 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);

@@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
 
   error:
      if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        BdrvDirtyBitmap *ret;
+        ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL);
+        assert(ret);
      }
  }

include/block/block.h +0 -4

index 3a85690..d7859a7 100644
@@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(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,

Quoted 22 lines · John Snow

Allow bitmap successors to carry reference counts.

We can in a later patch use this ability to clean up the dirty bitmap according to both the individual job's success and the success of all jobs in the transaction group.

The code for cleaning up a bitmap is also moved from backup_run to backup_complete.

Signed-off-by John Snow <[email protected]>

---
  block.c               | 79
+++++++++++++++++++++++++++++++++++++++++++++++----
  block/backup.c        | 23 ++++++---------
  include/block/block.h | 11 ++++---
  3 files changed, 87 insertions(+), 26 deletions(-)

block.c +0 -0

index 5eaa874..a0036af 100644

block.c line 57

Quoted 7 lines

block.c +5 -0

@@ -51,6 +51,12 @@
  #include <windows.h>
  #endif
+typedef enum BitmapSuccessorAction {
+    SUCCESSOR_ACTION_UNDEFINED = 0,
+    SUCCESSOR_ACTION_ABDICATE,
+    SUCCESSOR_ACTION_RECLAIM
+} BitmapSuccessorAction;

Maybe making this a QAPI enum can come in handy later. I don't know in which QMP commands one would use it, but if I could predict the future, I'd make trillions (or even billions) at the stock market.

(It's just that it already looks so much alike to a QAPI enum that I can't help but to think of making it one)

Quoted 57 lines

block.c +1 -0

+
  /**
   * A BdrvDirtyBitmap can be in three possible states:
   * (1) successor is false 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;

};

block.c +4 -3

@@ -5508,6 +5516,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;
  }

@@ -5515,9 +5524,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;

block.c +3 -3

@@ -5542,9 +5551,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;

block.c +2 -0

@@ -5563,6 +5572,64 @@ BdrvDirtyBitmap
 *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
      return parent;
  }
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5579

Quoted 4 lines

block.c +3 -0

 *parent,
+                                                   Error **errp)
+{
+    if (parent->successor_refcount) {

I don't know whether you're intending to call this function from anywhere outside of bdrv_dirty_bitmap_decref(), but if you don't, please just make this an assert(!parent->successor_refcount).

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5581

Quoted 2 lines

block.c +2 -0

+        error_setg(errp, "Cannot free the successor for bitmap '%s', "
+                   "because the refcount is non-zero.", parent->name);

Hm, again with the full stops? *g*

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5588

Quoted 7 lines

block.c +7 -0

+        return NULL;
+    }
+
+    switch (parent->act) {
+    case SUCCESSOR_ACTION_UNDEFINED:
+        error_setg(errp, "Cannot free the successor for bitmap '%s', "
+                   "because the successor action has not yet been set.",

Indeed, again with the full stops. ;-)

block.c BdrvDirtyBitmap · line 5591

Quoted 2 lines

block.c +2 -0

+                   parent->name);
+        return NULL;

So you're (for now) only calling this function from bdrv_dirty_bitmap_decref(), and that function always makes sure that parent->act is set to SUCCESSOR_ACTION_{RECLAIM,ABDICATE}. Why not add an assert(parent->act != SUCCESSOR_ACTION_UNDEFINED) before the switch? (If that causes problems in regards to compiler warnings or something (not having all enum values covered in the switch), just add an assert(0); under "case SUCCESSOR_ACTION_UNDEFINED:".)

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5600

Quoted 10 lines

block.c +10 -0

+    case SUCCESSOR_ACTION_RECLAIM:
+        return bdrv_reclaim_dirty_bitmap(bs, parent, errp);
+    case SUCCESSOR_ACTION_ABDICATE:
+        return bdrv_dirty_bitmap_abdicate(bs, parent, errp);
+    default:
+        error_setg(errp,
+                   "Unrecognized successor action (%d), "
+                   "cannot free successor for bitmap '%s'",
+                   parent->act, parent->name);
+        return NULL;

This can be made an assert(0), a g_assert_not_reached(), or simply an abort() (with the latter probably being the preferred way).

So I think that all the error_setg() calls are actually cases that should never happen. Better make them abort() and drop the Error parameter (because *_decref() and *_free() functions normally (for good reason) don't have Error parameters...).

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5604

Quoted 4 lines

block.c +4 -0

+    }
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs,

I don't know whether I'm that content with the name chosen, because

you're actually decrementing the refcount of the successor; but since the successor is basically a clone of the original bitmap (and I mean in the Star Trek sense, that it's a teleported clone and the original is intended to be destroyed so the successor can replace it), decrementing the refcount of the successor basically is equal to decrementing the refcount of the bitmap itself (as long as there is a successor, which you are asserting; maybe you want to add a comment about that to include/block/block.h, that one can only use this on frozen bitmaps?).

Quoted 11 lines

block.c +11 -0

+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp)
+{
+    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;

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5620

Quoted 5 lines

block.c +5 -0

+    }
+
+    parent->successor_refcount--;
+    if (parent->successor_refcount == 0) {
+        return bdrv_free_bitmap_successor(bs, parent, errp);

If you drop the Error parameter from bdrv_free_bitmap_successor(), you can drop it from this function, too.

Quoted 22 lines

block.c +12 -0

+    }
+    return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
  static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
 size)

  {
      /* Should only be frozen during a block backup job, which should

have

block/backup.c +0 -0

index 41bd9af..4332df4 100644

block/backup.c static void backup_complete(BlockJob *job, void · line 246

Quoted 7 lines

block/backup.c +4 -0

@@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void
 *opaque)
      bdrv_unref(s->target);
+    if (s->sync_bitmap) {
+        BdrvDirtyBitmap *bm;
+        bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap,
 data->ret, NULL);
+        assert(bm);

You can use &error_abort as the Error object and drop the assert(); or, if you are dropping the Error parameter, there is no need to check the return value at all, because it will always be non-NULL (there won't be any code path in the function returning NULL at all). Maybe you can even drop the return value, too.

I just looked through the series: Actually, you're never using the Error parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you really should drop it (and maybe the return value along with it).

Quoted 12 lines

block/backup.c +2 -4

+    }
+
      block_job_completed(job, data->ret);
      g_free(data);
  }

@@ -419,19 +425,6 @@ leave:
      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

block/backup.c leave: · line 428

Quoted 4 lines

nothing. */

block/backup.c +0 -3

-            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
-            assert(bm);
-            bdrv_enable_dirty_bitmap(job->sync_bitmap);

Hm, what is that function call doing here? It feels like it shouldn't have been part of your transactionless series (because other than this, there is no caller of bdrv_{en,dis}able_dirty_bitmap() at all).

(You're silently removing it here; removing it is fine, but I guess it shouldn't have been there in the first place)

Quoted 41 lines

block/backup.c +0 -2

-        } else {
-            /* Everything is fine, delete this bitmap and install the

backup. */

block/backup.c +3 -5

-            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        }
-    }
      hbitmap_free(job->bitmap);
      bdrv_iostatus_disable(target);

@@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs,
 BlockDriverState *target,
   error:
      if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        BdrvDirtyBitmap *ret;
+        ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL);
+        assert(ret);
      }
  }

include/block/block.h +5 -6

index 3a85690..d7859a7 100644
@@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(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_dirty_bitmap_decref(BlockDriverState *bs,
+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp);
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
  BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                          const char *name);

Quoted 1 line

  void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);

I'm happy with this patch if you drop the Error parameter from bdrv_dirty_bitmap_decref() (possibly dropping the return value, too) and turn all the error cases into assertions.

Max

Quoted 22 lines · John Snow

Allow bitmap successors to carry reference counts.

We can in a later patch use this ability to clean up the dirty bitmap according to both the individual job's success and the success of all jobs in the transaction group.

The code for cleaning up a bitmap is also moved from backup_run to backup_complete.

Signed-off-by John Snow <[email protected]>

---
  block.c               | 79
+++++++++++++++++++++++++++++++++++++++++++++++----
  block/backup.c        | 23 ++++++---------
  include/block/block.h | 11 ++++---
  3 files changed, 87 insertions(+), 26 deletions(-)

block.c +0 -0

index 5eaa874..a0036af 100644

block.c line 57

Quoted 7 lines

block.c +5 -0

@@ -51,6 +51,12 @@
  #include <windows.h>
  #endif
+typedef enum BitmapSuccessorAction {
+    SUCCESSOR_ACTION_UNDEFINED = 0,
+    SUCCESSOR_ACTION_ABDICATE,
+    SUCCESSOR_ACTION_RECLAIM
+} BitmapSuccessorAction;

Maybe making this a QAPI enum can come in handy later. I don't know in which QMP commands one would use it, but if I could predict the future, I'd make trillions (or even billions) at the stock market.

(It's just that it already looks so much alike to a QAPI enum that I can't help but to think of making it one)

Quoted 57 lines

block.c +1 -0

+
  /**
   * A BdrvDirtyBitmap can be in three possible states:
   * (1) successor is false 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;

};

block.c +4 -3

@@ -5508,6 +5516,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;
  }

@@ -5515,9 +5524,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;

block.c +3 -3

@@ -5542,9 +5551,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;

block.c +2 -0

@@ -5563,6 +5572,64 @@ BdrvDirtyBitmap
 *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
      return parent;
  }
+static BdrvDirtyBitmap *bdrv_free_bitmap_successor(BlockDriverState *bs,
+                                                   BdrvDirtyBitmap

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5579

Quoted 4 lines

block.c +3 -0

 *parent,
+                                                   Error **errp)
+{
+    if (parent->successor_refcount) {

I don't know whether you're intending to call this function from anywhere outside of bdrv_dirty_bitmap_decref(), but if you don't, please just make this an assert(!parent->successor_refcount).

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5581

Quoted 2 lines

block.c +2 -0

+        error_setg(errp, "Cannot free the successor for bitmap '%s', "
+                   "because the refcount is non-zero.", parent->name);

Hm, again with the full stops? *g*

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5588

Quoted 7 lines

block.c +7 -0

+        return NULL;
+    }
+
+    switch (parent->act) {
+    case SUCCESSOR_ACTION_UNDEFINED:
+        error_setg(errp, "Cannot free the successor for bitmap '%s', "
+                   "because the successor action has not yet been set.",

Indeed, again with the full stops. ;-)

block.c BdrvDirtyBitmap · line 5591

Quoted 2 lines

block.c +2 -0

+                   parent->name);
+        return NULL;

So you're (for now) only calling this function from bdrv_dirty_bitmap_decref(), and that function always makes sure that parent->act is set to SUCCESSOR_ACTION_{RECLAIM,ABDICATE}. Why not add an assert(parent->act != SUCCESSOR_ACTION_UNDEFINED) before the switch? (If that causes problems in regards to compiler warnings or something (not having all enum values covered in the switch), just add an assert(0); under "case SUCCESSOR_ACTION_UNDEFINED:".)

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5600

Quoted 10 lines

block.c +10 -0

+    case SUCCESSOR_ACTION_RECLAIM:
+        return bdrv_reclaim_dirty_bitmap(bs, parent, errp);
+    case SUCCESSOR_ACTION_ABDICATE:
+        return bdrv_dirty_bitmap_abdicate(bs, parent, errp);
+    default:
+        error_setg(errp,
+                   "Unrecognized successor action (%d), "
+                   "cannot free successor for bitmap '%s'",
+                   parent->act, parent->name);
+        return NULL;

This can be made an assert(0), a g_assert_not_reached(), or simply an abort() (with the latter probably being the preferred way).

So I think that all the error_setg() calls are actually cases that should never happen. Better make them abort() and drop the Error parameter (because *_decref() and *_free() functions normally (for good reason) don't have Error parameters...).

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5604

Quoted 4 lines

block.c +4 -0

+    }
+}
+
+BdrvDirtyBitmap *bdrv_dirty_bitmap_decref(BlockDriverState *bs,

I don't know whether I'm that content with the name chosen, because

you're actually decrementing the refcount of the successor; but since the successor is basically a clone of the original bitmap (and I mean in the Star Trek sense, that it's a teleported clone and the original is intended to be destroyed so the successor can replace it), decrementing the refcount of the successor basically is equal to decrementing the refcount of the bitmap itself (as long as there is a successor, which you are asserting; maybe you want to add a comment about that to include/block/block.h, that one can only use this on frozen bitmaps?).

Quoted 11 lines

block.c +11 -0

+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp)
+{
+    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;

block.c BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, · line 5620

Quoted 5 lines

block.c +5 -0

+    }
+
+    parent->successor_refcount--;
+    if (parent->successor_refcount == 0) {
+        return bdrv_free_bitmap_successor(bs, parent, errp);

If you drop the Error parameter from bdrv_free_bitmap_successor(), you can drop it from this function, too.

Quoted 22 lines

block.c +12 -0

+    }
+    return parent;
+}
+
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent)
+{
+    assert(bdrv_dirty_bitmap_frozen(parent));
+    assert(parent->successor);
+
+    parent->successor_refcount++;
+}
+
  static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t
 size)

  {
      /* Should only be frozen during a block backup job, which should

have

block/backup.c +0 -0

index 41bd9af..4332df4 100644

block/backup.c static void backup_complete(BlockJob *job, void · line 246

Quoted 7 lines

block/backup.c +4 -0

@@ -240,6 +240,12 @@ static void backup_complete(BlockJob *job, void
 *opaque)
      bdrv_unref(s->target);
+    if (s->sync_bitmap) {
+        BdrvDirtyBitmap *bm;
+        bm = bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap,
 data->ret, NULL);
+        assert(bm);

You can use &error_abort as the Error object and drop the assert(); or, if you are dropping the Error parameter, there is no need to check the return value at all, because it will always be non-NULL (there won't be any code path in the function returning NULL at all). Maybe you can even drop the return value, too.

I just looked through the series: Actually, you're never using the Error parameter for bdrv_dirty_bitmap_decref() at all. Seems to me like you really should drop it (and maybe the return value along with it).

Quoted 13 lines

block/backup.c +2 -4

+    }
+
      block_job_completed(job, data->ret);
      g_free(data);
  }

@@ -419,19 +425,6 @@ leave:
      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. */

block/backup.c leave: · line 428

Quoted 3 lines

block/backup.c +0 -3

-            bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
-            assert(bm);
-            bdrv_enable_dirty_bitmap(job->sync_bitmap);

Hm, what is that function call doing here? It feels like it shouldn't have been part of your transactionless series (because other than this, there is no caller of bdrv_{en,dis}able_dirty_bitmap() at all).

(You're silently removing it here; removing it is fine, but I guess it shouldn't have been there in the first place)

Quoted 41 lines

block/backup.c +0 -2

-        } else {
-            /* Everything is fine, delete this bitmap and install the

backup. */

block/backup.c +3 -5

-            bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
-            assert(bm);
-        }
-    }
      hbitmap_free(job->bitmap);
      bdrv_iostatus_disable(target);

@@ -535,6 +528,8 @@ void backup_start(BlockDriverState *bs,
 BlockDriverState *target,
   error:
      if (sync_bitmap) {
-        bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
+        BdrvDirtyBitmap *ret;
+        ret = bdrv_dirty_bitmap_decref(bs, sync_bitmap, -1, NULL);
+        assert(ret);
      }
  }

include/block/block.h +5 -6

index 3a85690..d7859a7 100644
@@ -458,12 +458,11 @@ void bdrv_dirty_bitmap_truncate(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_dirty_bitmap_decref(BlockDriverState *bs,
+                                          BdrvDirtyBitmap *parent,
+                                          int ret,
+                                          Error **errp);
+void bdrv_dirty_bitmap_incref(BdrvDirtyBitmap *parent);
  BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
                                          const char *name);

Quoted 1 line

  void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap);

I'm happy with this patch if you drop the Error parameter from bdrv_dirty_bitmap_decref() (possibly dropping the return value, too) and turn all the error cases into assertions.

Max