From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45406) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMIvx-0001k2-1R for qemu-devel@nongnu.org; Mon, 03 Aug 2015 12:49:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZMIvs-00042K-KW for qemu-devel@nongnu.org; Mon, 03 Aug 2015 12:49:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45419) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMIvs-000425-AS for qemu-devel@nongnu.org; Mon, 03 Aug 2015 12:49:48 -0400 References: <1438238370-16212-1-git-send-email-famz@redhat.com> <1438238370-16212-2-git-send-email-famz@redhat.com> From: Max Reitz Message-ID: <55BF9BA7.7030309@redhat.com> Date: Mon, 3 Aug 2015 18:49:43 +0200 MIME-Version: 1.0 In-Reply-To: <1438238370-16212-2-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 01/14] qapi: Add transaction support to block-dirty-bitmap operations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , Jeff Cody , vsementsov@parallels.com, stefanha@redhat.com, John Snow On 30.07.2015 08:39, Fam Zheng wrote: > From: John Snow > > 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 > Signed-off-by: John Snow > Reviewed-by: Max Reitz > Reviewed-by: Stefan Hajnoczi > Signed-off-by: Stefan Hajnoczi > Signed-off-by: Fam Zheng > --- > block.c | 19 +++++++- > blockdev.c | 114 +++++++++++++++++++++++++++++++++++++++++++++- > docs/bitmaps.md | 6 +-- > include/block/block.h | 1 - > include/block/block_int.h | 3 ++ > qapi-schema.json | 6 ++- > 6 files changed, 139 insertions(+), 10 deletions(-) > > diff --git a/block.c b/block.c > index d088ee0..cee68c2 100644 > --- a/block.c > +++ b/block.c > @@ -3566,10 +3566,25 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, > hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); > } > > -void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap) > +void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) > { > assert(bdrv_dirty_bitmap_enabled(bitmap)); > - hbitmap_reset_all(bitmap->bitmap); > + if (!out) { > + hbitmap_reset_all(bitmap->bitmap); > + } else { > + HBitmap *backup = bitmap->bitmap; > + bitmap->bitmap = hbitmap_alloc(bitmap->size, > + hbitmap_granularity(backup)); > + *out = backup; > + } > +} > + > +void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in) > +{ > + HBitmap *tmp = bitmap->bitmap; > + assert(bdrv_dirty_bitmap_enabled(bitmap)); > + bitmap->bitmap = in; > + hbitmap_free(tmp); > } > > void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > diff --git a/blockdev.c b/blockdev.c > index 62a4586..b80439b 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1708,6 +1708,106 @@ static void blockdev_backup_clean(BlkTransactionState *common) > } > } > > +typedef struct BlockDirtyBitmapState { > + BlkTransactionState common; > + BdrvDirtyBitmap *bitmap; > + BlockDriverState *bs; > + AioContext *aio_context; > + HBitmap *backup; > + 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 and released 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; > + } > + > + bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); > + /* AioContext is released in .clean() */ > +} > + > +static void block_dirty_bitmap_clear_abort(BlkTransactionState *common) > +{ > + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > + common, common); > + > + bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup); > +} > + > +static void block_dirty_bitmap_clear_commit(BlkTransactionState *common) > +{ > + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > + common, common); > + > + hbitmap_free(state->backup); > +} > + > +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"); > @@ -1748,6 +1848,18 @@ 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, > + .abort = block_dirty_bitmap_clear_abort, > + .clean = block_dirty_bitmap_clear_clean, > + } > }; > > /* > @@ -2131,7 +2243,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, > goto out; > } > > - bdrv_clear_dirty_bitmap(bitmap); > + bdrv_clear_dirty_bitmap(bitmap, NULL); > > out: > aio_context_release(aio_context); > diff --git a/docs/bitmaps.md b/docs/bitmaps.md > index fa87f07..9fd8ea6 100644 > --- a/docs/bitmaps.md > +++ b/docs/bitmaps.md > @@ -97,11 +97,7 @@ which is included at the end of this document. > } > ``` > > -## Transactions (Not yet implemented) > - > -* Transactional commands are forthcoming in a future version, > - and are not yet available for use. This section serves as > - documentation of intent for their design and usage. > +## Transactions > > ### Justification > > diff --git a/include/block/block.h b/include/block/block.h > index 37916f7..b1116e9 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -503,7 +503,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors); > void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors); > -void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap); > void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi); > void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset); > int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 14ad4c3..281d790 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -664,4 +664,7 @@ void blk_dev_resize_cb(BlockBackend *blk); > > void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_sectors); > > +void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); > +void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); > + > #endif /* BLOCK_INT_H */ > diff --git a/qapi-schema.json b/qapi-schema.json > index 4342a08..83de942 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1493,6 +1493,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 *2.5, and keep my R-b. Max > ## > { 'union': 'TransactionAction', > 'data': { > @@ -1500,7 +1502,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' > } } > > ## >