qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v12 09/17] qapi: Add transaction support to bloc


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH v12 09/17] qapi: Add transaction support to block-dirty-bitmap operations
Date: Wed, 11 Feb 2015 14:07:36 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

On 2015-02-09 at 20:35, John Snow wrote:
This adds four qmp commands to transactions.

Users can stop a dirty bitmap, start backup of it, and start another
dirty bitmap atomically, so that the dirty bitmap is tracked
incrementally and we don't miss any write.

For starting a new incremental backup chain, users can also chain
together a bitmap clear and a full block backup.

Signed-off-by: Fam Zheng <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
  blockdev.c       | 155 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
  qapi-schema.json |   6 ++-
  2 files changed, 160 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 83d0608..ed96e72 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1675,6 +1675,138 @@ static void blockdev_backup_clean(BlkTransactionState 
*common)
      }
  }
+static void block_dirty_bitmap_add_prepare(BlkTransactionState *common,
+                                           Error **errp)
+{
+    BlockDirtyBitmapAdd *action;
+
+    action = common->action->block_dirty_bitmap_add;
+    qmp_block_dirty_bitmap_add(action->node, action->name,
+                               action->has_granularity, action->granularity,
+                               errp);
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+    BlockDirtyBitmapAdd *action;
+
+    action = common->action->block_dirty_bitmap_add;
+    /* Should not fail meaningfully: IF the bitmap was added via .prepare(),
+     * then the node reference and bitmap name must have been valid.
+     * THUS: any failure here could only indicate the lack of a bitmap at all.
+     */
+    qmp_block_dirty_bitmap_remove(action->node, action->name, NULL);

What if block_dirty_bitmap_add_prepare() failed because a bitmap with that name already exists? Wouldn't this silently delete that existing bitmap?

I think you should store whether qmp_block_dirty_bitmap_add() was successful and only call qmp_block_dirty_bitmap_remove() if it was, and since you say it cannot fail (which I agree on), with &error_abort as the last argument.

+}
+
+typedef struct BlockDirtyBitmapState {
+    BlkTransactionState common;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+    AioContext *aio_context;
+} BlockDirtyBitmapState;
+
+/**
+ * Enable and Disable re-use the same preparation.
+ */
+static void block_dirty_bitmap_toggle_prepare(BlkTransactionState *common,
+                                              Error **errp)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    BlockDirtyBitmap *action;
+    BlockDriverState *bs;
+
+    /* We may be used by either enable or disable;
+     * We use the "enable" member of the union here,
+     * but "disable" should be functionally equivalent: */
+    action = common->action->block_dirty_bitmap_enable;
+    assert(action == common->action->block_dirty_bitmap_disable);
+
+    state->bitmap = block_dirty_bitmap_lookup(action->node,
+                                              action->name,
+                                              &bs,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+        error_setg(errp, "Cannot modify a frozen bitmap.\n");

I'm sorry.\n

+        return;
+    }
+
+    /* AioContext is released in .clean() */
+    state->aio_context = bdrv_get_aio_context(bs);
+    aio_context_acquire(state->aio_context);
+}
+
+static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    bdrv_enable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    bdrv_disable_dirty_bitmap(state->bitmap);
+}
+
+static void block_dirty_bitmap_toggle_clean(BlkTransactionState *common)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+
+    if (state->aio_context) {
+        aio_context_release(state->aio_context);
+    }
+}
+
+static void block_dirty_bitmap_clear_prepare(BlkTransactionState *common,
+                                             Error **errp)
+{
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             common, common);
+    BlockDirtyBitmap *action;
+
+    action = common->action->block_dirty_bitmap_clear;
+    state->bitmap = block_dirty_bitmap_lookup(action->node,
+                                              action->name,
+                                              &state->bs,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+        error_setg(errp, "Cannot modify a frozen bitmap.\n");
+        return;
+    }

Following your reply regarding patch 8, this needs a check whether the bitmap is disabled, too.\n

+
+    /* AioContext is released in .clean() */
+    state->aio_context = bdrv_get_aio_context(state->bs);
+    aio_context_acquire(state->aio_context);
+}
+
+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");
@@ -1715,6 +1847,29 @@ static const BdrvActionOps actions[] = {
          .abort = internal_snapshot_abort,
          .clean = internal_snapshot_clean,
      },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ADD] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = block_dirty_bitmap_add_prepare,
+        .abort = block_dirty_bitmap_add_abort,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_toggle_prepare,
+        .commit = block_dirty_bitmap_enable_commit,
+        .clean = block_dirty_bitmap_toggle_clean,
+    },
+    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
+        .instance_size = sizeof(BlockDirtyBitmapState),
+        .prepare = block_dirty_bitmap_toggle_prepare,
+        .commit = block_dirty_bitmap_disable_commit,
+        .clean = block_dirty_bitmap_toggle_clean,
+    },
+    [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,
+    }
  };
/*
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..cf4aa12 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1339,7 +1339,11 @@
         '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-enable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
+       'block-dirty-bitmap-clear': 'BlockDirtyBitmap'

Maybe add version information to the comment above this union?

Max

     } }
##




reply via email to

[Prev in Thread] Current Thread [Next in Thread]