diff mbox

[v3,01/10] qapi: Add transaction support to block-dirty-bitmap operations

Message ID 1429747493-24397-2-git-send-email-jsnow@redhat.com
State New
Headers show

Commit Message

John Snow April 23, 2015, 12:04 a.m. UTC
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 <famz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 blockdev.c       | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 docs/bitmaps.md  |   2 +-
 qapi-schema.json |   6 +++-
 3 files changed, 106 insertions(+), 2 deletions(-)

Comments

Eric Blake April 23, 2015, 2:22 a.m. UTC | #1
On 04/22/2015 06:04 PM, John Snow wrote:
> 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 <famz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---

> +++ b/docs/bitmaps.md
> @@ -97,7 +97,7 @@ which is included at the end of this document.
>  }
>  ```
>  
> -## Transactions (Not yet implemented)
> +## Transactions
>  
>  * Transactional commands are forthcoming in a future version,
>    and are not yet available for use. This section serves as

You got the section heading, but missed the next paragraph.

But (as you just proved in this attempt) deleting even more stale docs
doesn't invalidate my R-b on the code :)
Stefan Hajnoczi May 7, 2015, 2:54 p.m. UTC | #2
On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
> +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,
> +                                              &state->aio_context,
> +                                              errp);
> +    if (!state->bitmap) {
> +        return;
> +    }
> +
> +    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
> +        error_setg(errp, "Cannot modify a frozen bitmap");
> +        return;
> +    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
> +        error_setg(errp, "Cannot clear a disabled bitmap");
> +        return;
> +    }
> +
> +    /* AioContext is released in .clean() */
> +}
> +
> +static void block_dirty_bitmap_clear_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_clear_dirty_bitmap(state->bitmap);
> +}

These semantics don't work in this example:

[block-dirty-bitmap-clear,
 drive-backup]

Since drive-backup starts the blockjob in .prepare() but
block-dirty-bitmap-clear only clears the bitmap in .commit() the order
is wrong.

.prepare() has to do something non-destructive, like stashing away the
HBitmap and replacing it with an empty one.  Then .commit() can discard
the old bitmap while .abort() can move the old bitmap back to undo the
operation.

Stefan
John Snow May 7, 2015, 5:22 p.m. UTC | #3
On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
> On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
>> +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, +
>> &state->aio_context, +
>> errp); +    if (!state->bitmap) { +        return; +    } + +
>> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
>> error_setg(errp, "Cannot modify a frozen bitmap"); +
>> return; +    } else if
>> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
>> error_setg(errp, "Cannot clear a disabled bitmap"); +
>> return; +    } + +    /* AioContext is released in .clean() */ 
>> +} + +static void
>> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
>> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
>> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
> 
> These semantics don't work in this example:
> 
> [block-dirty-bitmap-clear, drive-backup]
> 
> Since drive-backup starts the blockjob in .prepare() but 
> block-dirty-bitmap-clear only clears the bitmap in .commit() the
> order is wrong.
> 
> .prepare() has to do something non-destructive, like stashing away
> the HBitmap and replacing it with an empty one.  Then .commit() can
> discard the old bitmap while .abort() can move the old bitmap back
> to undo the operation.
> 
> Stefan
> 

Hmm, that's sort of gross. That means that any transactional command
*ever* destined to be used with drive-backup in any conceivable way
needs to move a lot more of its action forward to .prepare().

That sort of defeats the premise of .prepare() and .commit(), no? And
all because drive-backup jumped the gun.

That's going to get hard to maintain as we add more transactions.

--js
Stefan Hajnoczi May 8, 2015, 1:14 p.m. UTC | #4
On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote:
> 
> 
> On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
> > On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
> >> +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, +
> >> &state->aio_context, +
> >> errp); +    if (!state->bitmap) { +        return; +    } + +
> >> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
> >> error_setg(errp, "Cannot modify a frozen bitmap"); +
> >> return; +    } else if
> >> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
> >> error_setg(errp, "Cannot clear a disabled bitmap"); +
> >> return; +    } + +    /* AioContext is released in .clean() */ 
> >> +} + +static void
> >> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
> >> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
> >> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
> > 
> > These semantics don't work in this example:
> > 
> > [block-dirty-bitmap-clear, drive-backup]
> > 
> > Since drive-backup starts the blockjob in .prepare() but 
> > block-dirty-bitmap-clear only clears the bitmap in .commit() the
> > order is wrong.
> > 
> > .prepare() has to do something non-destructive, like stashing away
> > the HBitmap and replacing it with an empty one.  Then .commit() can
> > discard the old bitmap while .abort() can move the old bitmap back
> > to undo the operation.
> > 
> > Stefan
> > 
> 
> Hmm, that's sort of gross. That means that any transactional command
> *ever* destined to be used with drive-backup in any conceivable way
> needs to move a lot more of its action forward to .prepare().
> 
> That sort of defeats the premise of .prepare() and .commit(), no? And
> all because drive-backup jumped the gun.

No it doesn't.  Actions have to appear atomic to the qmp_transaction
caller.  Both approaches achieve that so they are both correct in
isolation.

The ambiguity is whether "commit the changes" for .commit() means
"changes take effect" or "discard stashed state, making undo
impossible".

I think the "discard stashed state, making undo impossible"
interpretation is good because .commit() is not allowed to fail.  That
function should only do things that never fail.

> That's going to get hard to maintain as we add more transactions.

Yes, we need to be consistent and stick to one of the interpretations in
order to guarantee ordering.

Unfortunately, there is already an inconsistency:

1. internal_snapshot - snapshot taken in .prepare()
2. external_snapshot - BDS node appended in .commit()
3. drive_backup - block job started in .prepare()
4. blockdev_backup - block job started in .prepare()

external_snapshot followed by internal_snapshot acts like the reverse
ordering!

Stefan
Max Reitz May 8, 2015, 1:17 p.m. UTC | #5
On 08.05.2015 15:14, Stefan Hajnoczi wrote:
> On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote:
>>
>> On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
>>> On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
>>>> +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, +
>>>> &state->aio_context, +
>>>> errp); +    if (!state->bitmap) { +        return; +    } + +
>>>> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
>>>> error_setg(errp, "Cannot modify a frozen bitmap"); +
>>>> return; +    } else if
>>>> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
>>>> error_setg(errp, "Cannot clear a disabled bitmap"); +
>>>> return; +    } + +    /* AioContext is released in .clean() */
>>>> +} + +static void
>>>> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
>>>> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
>>>> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
>>> These semantics don't work in this example:
>>>
>>> [block-dirty-bitmap-clear, drive-backup]
>>>
>>> Since drive-backup starts the blockjob in .prepare() but
>>> block-dirty-bitmap-clear only clears the bitmap in .commit() the
>>> order is wrong.

Well, "starts the block job" is technically correct, but the block job 
doesn't run until later. If it were to really start in prepare, that 
would be wrong. Actually, the block job is initialized and yields, 
allowing the code handling the QMP transaction command to continue. I 
think in your example that means that the block job won't actually run 
until after block-dirty-bitmap-clear has been committed.

Max

>>>
>>> .prepare() has to do something non-destructive, like stashing away
>>> the HBitmap and replacing it with an empty one.  Then .commit() can
>>> discard the old bitmap while .abort() can move the old bitmap back
>>> to undo the operation.
>>>
>>> Stefan
>>>
>> Hmm, that's sort of gross. That means that any transactional command
>> *ever* destined to be used with drive-backup in any conceivable way
>> needs to move a lot more of its action forward to .prepare().
>>
>> That sort of defeats the premise of .prepare() and .commit(), no? And
>> all because drive-backup jumped the gun.
> No it doesn't.  Actions have to appear atomic to the qmp_transaction
> caller.  Both approaches achieve that so they are both correct in
> isolation.
>
> The ambiguity is whether "commit the changes" for .commit() means
> "changes take effect" or "discard stashed state, making undo
> impossible".
>
> I think the "discard stashed state, making undo impossible"
> interpretation is good because .commit() is not allowed to fail.  That
> function should only do things that never fail.
>
>> That's going to get hard to maintain as we add more transactions.
> Yes, we need to be consistent and stick to one of the interpretations in
> order to guarantee ordering.
>
> Unfortunately, there is already an inconsistency:
>
> 1. internal_snapshot - snapshot taken in .prepare()
> 2. external_snapshot - BDS node appended in .commit()
> 3. drive_backup - block job started in .prepare()
> 4. blockdev_backup - block job started in .prepare()
>
> external_snapshot followed by internal_snapshot acts like the reverse
> ordering!
>
> Stefan
Eric Blake May 8, 2015, 2:29 p.m. UTC | #6
On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote:

> No it doesn't.  Actions have to appear atomic to the qmp_transaction
> caller.  Both approaches achieve that so they are both correct in
> isolation.
> 
> The ambiguity is whether "commit the changes" for .commit() means
> "changes take effect" or "discard stashed state, making undo
> impossible".
> 
> I think the "discard stashed state, making undo impossible"
> interpretation is good because .commit() is not allowed to fail.  That
> function should only do things that never fail.
> 
>> That's going to get hard to maintain as we add more transactions.
> 
> Yes, we need to be consistent and stick to one of the interpretations in
> order to guarantee ordering.
> 
> Unfortunately, there is already an inconsistency:
> 
> 1. internal_snapshot - snapshot taken in .prepare()
> 2. external_snapshot - BDS node appended in .commit()
> 3. drive_backup - block job started in .prepare()
> 4. blockdev_backup - block job started in .prepare()
> 
> external_snapshot followed by internal_snapshot acts like the reverse
> ordering!

Is that fatal, though?  Let's see if I'm understanding the problem
correctly: if you start with a.qcow2, then
 external_snapshot followed by internal_snapshot
should create b.qcow2 then the internal snapshot inside b.qcow2, while
 internal_snapshot followed by external_snapshot
should create the internal snapshot inside a.qcow2, then create b.qcow2

But since we create the BDS node later than the internal snapshot is
taken, both sequences currently cause the internal snapshot to live in
a.qcow2.
John Snow May 8, 2015, 4:19 p.m. UTC | #7
On 05/08/2015 09:17 AM, Max Reitz wrote:
> On 08.05.2015 15:14, Stefan Hajnoczi wrote:
>> On Thu, May 07, 2015 at 01:22:26PM -0400, John Snow wrote:
>>>
>>> On 05/07/2015 10:54 AM, Stefan Hajnoczi wrote:
>>>> On Wed, Apr 22, 2015 at 08:04:44PM -0400, John Snow wrote:
>>>>> +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, +
>>>>> &state->aio_context, +
>>>>> errp); +    if (!state->bitmap) { +        return; +    } + +
>>>>> if (bdrv_dirty_bitmap_frozen(state->bitmap)) { +
>>>>> error_setg(errp, "Cannot modify a frozen bitmap"); +
>>>>> return; +    } else if
>>>>> (!bdrv_dirty_bitmap_enabled(state->bitmap)) { +
>>>>> error_setg(errp, "Cannot clear a disabled bitmap"); +
>>>>> return; +    } + +    /* AioContext is released in .clean() */
>>>>> +} + +static void
>>>>> block_dirty_bitmap_clear_commit(BlkTransactionState *common) +{ +
>>>>> BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, +
>>>>> common, common); +    bdrv_clear_dirty_bitmap(state->bitmap); +}
>>>> These semantics don't work in this example:
>>>>
>>>> [block-dirty-bitmap-clear, drive-backup]
>>>>
>>>> Since drive-backup starts the blockjob in .prepare() but
>>>> block-dirty-bitmap-clear only clears the bitmap in .commit() the
>>>> order is wrong.
> 
> Well, "starts the block job" is technically correct, but the block job
> doesn't run until later. If it were to really start in prepare, that
> would be wrong. Actually, the block job is initialized and yields,
> allowing the code handling the QMP transaction command to continue. I
> think in your example that means that the block job won't actually run
> until after block-dirty-bitmap-clear has been committed.
> 
> Max
> 

The important thing is that we get the contents of the drive as it was,
according to the bitmap as it was, when we start the job.

So if clear doesn't actually modify the bitmap until the commit() phase,
but drive-backup actually goes until the first yield() in prepare...

We're actually going to see an assertion() failure, likely. drive-backup
will freeze the bitmap with a successor, but then the clear transaction
will try to "commit" the changes and try to modify a frozen bitmap.

Oops.

What we (The royal we...) need to figure out is how we want to solve
this problem.

>>>>
>>>> .prepare() has to do something non-destructive, like stashing away
>>>> the HBitmap and replacing it with an empty one.  Then .commit() can
>>>> discard the old bitmap while .abort() can move the old bitmap back
>>>> to undo the operation.
>>>>
>>>> Stefan
>>>>
>>> Hmm, that's sort of gross. That means that any transactional command
>>> *ever* destined to be used with drive-backup in any conceivable way
>>> needs to move a lot more of its action forward to .prepare().
>>>
>>> That sort of defeats the premise of .prepare() and .commit(), no? And
>>> all because drive-backup jumped the gun.
>> No it doesn't.  Actions have to appear atomic to the qmp_transaction
>> caller.  Both approaches achieve that so they are both correct in
>> isolation.
>>
>> The ambiguity is whether "commit the changes" for .commit() means
>> "changes take effect" or "discard stashed state, making undo
>> impossible".
>>
>> I think the "discard stashed state, making undo impossible"
>> interpretation is good because .commit() is not allowed to fail.  That
>> function should only do things that never fail.
>>

To be clear, you are favoring drive-backup's interpretation of the
prepare and commit phases. I had been operating under the other
interpretation.

I think I like the semantics of my interpretation better, but have to
admit it's a lot harder programmatically to enforce "commit cannot fail"
for all of the transactions we support under mine, so your
interpretation is probably the right way to go for sanity's sake -- we
just need to add a bit of documentation to make it clear.

I suppose in this case clear() isn't too hard to modify -- just rip the
Hbitmap out of it and replace it with a new empty one. Don't free() the
old one until commit(). Should be a fairly inexpensive operation.

I will have to re-audit all the existing transactions to make sure these
semantics are consistent, though.

>>> That's going to get hard to maintain as we add more transactions.
>> Yes, we need to be consistent and stick to one of the interpretations in
>> order to guarantee ordering.
>>
>> Unfortunately, there is already an inconsistency:
>>
>> 1. internal_snapshot - snapshot taken in .prepare()
>> 2. external_snapshot - BDS node appended in .commit()
>> 3. drive_backup - block job started in .prepare()
>> 4. blockdev_backup - block job started in .prepare()
>>
>> external_snapshot followed by internal_snapshot acts like the reverse
>> ordering!
>>
>> Stefan
> 

What a mess!

--js
Stefan Hajnoczi May 11, 2015, 1:10 p.m. UTC | #8
On Fri, May 08, 2015 at 08:29:09AM -0600, Eric Blake wrote:
> On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote:
> 
> > No it doesn't.  Actions have to appear atomic to the qmp_transaction
> > caller.  Both approaches achieve that so they are both correct in
> > isolation.
> > 
> > The ambiguity is whether "commit the changes" for .commit() means
> > "changes take effect" or "discard stashed state, making undo
> > impossible".
> > 
> > I think the "discard stashed state, making undo impossible"
> > interpretation is good because .commit() is not allowed to fail.  That
> > function should only do things that never fail.
> > 
> >> That's going to get hard to maintain as we add more transactions.
> > 
> > Yes, we need to be consistent and stick to one of the interpretations in
> > order to guarantee ordering.
> > 
> > Unfortunately, there is already an inconsistency:
> > 
> > 1. internal_snapshot - snapshot taken in .prepare()
> > 2. external_snapshot - BDS node appended in .commit()
> > 3. drive_backup - block job started in .prepare()
> > 4. blockdev_backup - block job started in .prepare()
> > 
> > external_snapshot followed by internal_snapshot acts like the reverse
> > ordering!
> 
> Is that fatal, though?

Yes, ordering is critical when add-bitmap or clear-bitmap are combined
with drive-backup.  Typically the drive-backup must happen after
add-bitmap or clear-bitmap.

There is probably no one who uses external and internal snapshots
together in a single 'transaction' command, so my example is contrived
but it's the same problem.

> Let's see if I'm understanding the problem
> correctly: if you start with a.qcow2, then
>  external_snapshot followed by internal_snapshot
> should create b.qcow2 then the internal snapshot inside b.qcow2, while
>  internal_snapshot followed by external_snapshot
> should create the internal snapshot inside a.qcow2, then create b.qcow2
> 
> But since we create the BDS node later than the internal snapshot is
> taken, both sequences currently cause the internal snapshot to live in
> a.qcow2.

Right.  Ordering is not honored :(.

Stefan
Kevin Wolf May 18, 2015, 3:03 p.m. UTC | #9
Am 11.05.2015 um 15:10 hat Stefan Hajnoczi geschrieben:
> On Fri, May 08, 2015 at 08:29:09AM -0600, Eric Blake wrote:
> > On 05/08/2015 07:14 AM, Stefan Hajnoczi wrote:
> > 
> > > No it doesn't.  Actions have to appear atomic to the qmp_transaction
> > > caller.  Both approaches achieve that so they are both correct in
> > > isolation.
> > > 
> > > The ambiguity is whether "commit the changes" for .commit() means
> > > "changes take effect" or "discard stashed state, making undo
> > > impossible".
> > > 
> > > I think the "discard stashed state, making undo impossible"
> > > interpretation is good because .commit() is not allowed to fail.  That
> > > function should only do things that never fail.
> > > 
> > >> That's going to get hard to maintain as we add more transactions.
> > > 
> > > Yes, we need to be consistent and stick to one of the interpretations in
> > > order to guarantee ordering.
> > > 
> > > Unfortunately, there is already an inconsistency:
> > > 
> > > 1. internal_snapshot - snapshot taken in .prepare()
> > > 2. external_snapshot - BDS node appended in .commit()
> > > 3. drive_backup - block job started in .prepare()
> > > 4. blockdev_backup - block job started in .prepare()
> > > 
> > > external_snapshot followed by internal_snapshot acts like the reverse
> > > ordering!
> > 
> > Is that fatal, though?
> 
> Yes, ordering is critical when add-bitmap or clear-bitmap are combined
> with drive-backup.  Typically the drive-backup must happen after
> add-bitmap or clear-bitmap.

Is there a reason to include add-bitmap/clear-bitmap in the transaction
rather than preparing everything before the transaction and then only
starting the backup (possibly of multiple disks) in a transaction?

The original assumption was that there are no interdependencies between
different transaction commands and the order is undefined.

Kevin
diff mbox

Patch

diff --git a/blockdev.c b/blockdev.c
index b9c79ed..d162e10 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1693,6 +1693,95 @@  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) {
+        state->prepared = true;
+    } else {
+        error_propagate(errp, local_err);
+    }
+}
+
+static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
+{
+    BlockDirtyBitmapAdd *action;
+    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
+                                             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)
+{
+    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,
+                                              &state->aio_context,
+                                              errp);
+    if (!state->bitmap) {
+        return;
+    }
+
+    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
+        error_setg(errp, "Cannot modify a frozen bitmap");
+        return;
+    } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) {
+        error_setg(errp, "Cannot clear a disabled bitmap");
+        return;
+    }
+
+    /* AioContext is released in .clean() */
+}
+
+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");
@@ -1733,6 +1822,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,
+    }
 };
 
 /*
diff --git a/docs/bitmaps.md b/docs/bitmaps.md
index f066b48..402dd98 100644
--- a/docs/bitmaps.md
+++ b/docs/bitmaps.md
@@ -97,7 +97,7 @@  which is included at the end of this document.
 }
 ```
 
-## Transactions (Not yet implemented)
+## Transactions
 
 * Transactional commands are forthcoming in a future version,
   and are not yet available for use. This section serves as
diff --git a/qapi-schema.json b/qapi-schema.json
index ac9594d..f6fe2b3 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1356,6 +1356,8 @@ 
 # abort since 1.6
 # blockdev-snapshot-internal-sync since 1.7
 # blockdev-backup since 2.3
+# block-dirty-bitmap-add since 2.4
+# block-dirty-bitmap-clear since 2.4
 ##
 { 'union': 'TransactionAction',
   'data': {
@@ -1363,7 +1365,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'
    } }
 
 ##