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