Re: [Qemu-devel] [Qemu-block] [PATCH v3 01/10] qapi: Add transaction support to block-dirty-bitmap operations

[John Snow <jsnow@redhat.com>]

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