Message ID | 1423532117-14490-10-git-send-email-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
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 <famz@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > 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 > } } > > ##
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); +} + +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"); + 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; + } + + /* 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' } } ##