diff mbox

[v4,9/9] qapi: Add transaction support to dirty-bitmap-{add, disable}

Message ID 1395911388-31027-10-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng March 27, 2014, 9:09 a.m. UTC 
This adds dirty-bitmap-add and dirty-bitmap-disable to transactions.
With this, user 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.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  4 +++-
 2 files changed, 71 insertions(+), 1 deletion(-)

Comments

Eric Blake March 27, 2014, 4:49 p.m. UTC | #1 
On 03/27/2014 03:09 AM, Fam Zheng wrote:
> This adds dirty-bitmap-add and dirty-bitmap-disable to transactions.
> With this, user 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.

Ah, this addresses my question in 7/9 about atomicity of creating a map
in disabled state.  Nice idea to use 'transaction' to merge the two
commands, instead of having to bloat the creation with ever more options.

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockdev.c       | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |  4 +++-
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> +++ b/qapi-schema.json
> @@ -1994,7 +1994,9 @@
>         'blockdev-snapshot-sync': 'BlockdevSnapshot',
>         'drive-backup': 'DriveBackup',
>         'abort': 'Abort',
> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
> +       'dirty-bitmap-add': 'DirtyBitmap',
> +       'dirty-bitmap-disable': 'DirtyBitmap'
>     } }

However, shouldn't it ALSO be possible to enable a bitmap in tandem with
other transaction operations?  I can imagine a transaction that both
kicks off a snapshot and enables a bitmap that was previously added but
left disabled.
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index 4120dee..38dabe6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1411,6 +1411,64 @@  static void drive_backup_abort(BlkTransactionState *common)
     }
 }
 
+static void dirty_bitmap_add_prepare(BlkTransactionState *common, Error **errp)
+{
+    DirtyBitmap *action;
+    Error *local_err = NULL;
+
+    action = common->action->dirty_bitmap_add;
+    qmp_dirty_bitmap_add(action->device, action->name, false, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+    DirtyBitmap *action;
+    BdrvDirtyBitmap *bm;
+    BlockDriverState *bs;
+
+    action = common->action->dirty_bitmap_add;
+    bs = bdrv_find(action->device);
+    if (bs) {
+        bm = bdrv_find_dirty_bitmap(bs, action->name);
+        if (bm) {
+            bdrv_release_dirty_bitmap(bs, bm);
+        }
+    }
+}
+
+static void dirty_bitmap_disable_prepare(BlkTransactionState *common,
+                                         Error **errp)
+{
+    DirtyBitmap *action;
+    Error *local_err = NULL;
+
+    action = common->action->dirty_bitmap_disable;
+    qmp_dirty_bitmap_disable(action->device, action->name,
+                             false, 0, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void dirty_bitmap_disable_abort(BlkTransactionState *common)
+{
+    DirtyBitmap *action;
+    BdrvDirtyBitmap *bitmap;
+    BlockDriverState *bs;
+
+    action = common->action->dirty_bitmap_disable;
+    bs = bdrv_find(action->device);
+    if (bs) {
+        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
+        if (bitmap) {
+            bdrv_enable_dirty_bitmap(bs, bitmap);
+        }
+    }
+}
+
 static void abort_prepare(BlkTransactionState *common, Error **errp)
 {
     error_setg(errp, "Transaction aborted using Abort action");
@@ -1443,6 +1501,16 @@  static const BdrvActionOps actions[] = {
         .prepare  = internal_snapshot_prepare,
         .abort = internal_snapshot_abort,
     },
+    [TRANSACTION_ACTION_KIND_DIRTY_BITMAP_ADD] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = dirty_bitmap_add_prepare,
+        .abort = dirty_bitmap_add_abort,
+    },
+    [TRANSACTION_ACTION_KIND_DIRTY_BITMAP_DISABLE] = {
+        .instance_size = sizeof(BlkTransactionState),
+        .prepare = dirty_bitmap_disable_prepare,
+        .abort = dirty_bitmap_disable_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index d99041c..48f7c15 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1994,7 +1994,9 @@ 
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
        'drive-backup': 'DriveBackup',
        'abort': 'Abort',
-       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
+       'dirty-bitmap-add': 'DirtyBitmap',
+       'dirty-bitmap-disable': 'DirtyBitmap'
    } }
 
 ##