Re: [Qemu-devel] [PATCH v6 08/10] qapi: Add transaction support to block-dirty-bitmap-{add, enable, disable}

[Max Reitz <mreitz@redhat.com>]

On 2014-11-21 at 23:24, John Snow wrote:
>
>
> On 11/04/2014 06:03 AM, Max Reitz wrote:
>> On 2014-10-30 at 04:22, Fam Zheng wrote:
>>> This adds three 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.
>>>
>>> Signed-off-by: Fam Zheng <famz@redhat.com>
>>> ---
>>>   blockdev.c       | 92
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qapi-schema.json |  5 ++-
>>>   2 files changed, 96 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index fca909b..34fa314 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -1490,6 +1490,83 @@ static void
>>> drive_backup_abort(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->device, action->name,
>>> +                               action->has_granularity,
>>> action->granularity,
>>> +                               errp);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_add_abort(BlkTransactionState *common)
>>> +{
>>> +    BlockDirtyBitmapAdd *action;
>>> +    BdrvDirtyBitmap *bm;
>>> +    BlockDriverState *bs;
>>> +
>>> +    action = common->action->block_dirty_bitmap_add;
>>> +    bs = bdrv_find(action->device);
>>> +    if (bs) {
>>> +        bm = bdrv_find_dirty_bitmap(bs, action->name);
>>> +        if (bm) {
>>> +            bdrv_release_dirty_bitmap(bs, bm);
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +static void block_dirty_bitmap_enable_prepare(BlkTransactionState
>>> *common,
>>> +                                               Error **errp)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +
>>> +    action = common->action->block_dirty_bitmap_enable;
>>> +    qmp_block_dirty_bitmap_enable(action->device, action->name, errp);
>>> +}
>>> +
>>> +static void block_dirty_bitmap_enable_abort(BlkTransactionState 
>>> *common)
>>> +{
>>> +    BlockDirtyBitmap *action;
>>> +    BdrvDirtyBitmap *bitmap;
>>> +    BlockDriverState *bs;
>>> +
>>> +    action = common->action->block_dirty_bitmap_enable;
>>> +    bs = bdrv_find(action->device);
>>> +    if (bs) {
>>> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
>>> +        if (bitmap) {
>>> +            bdrv_disable_dirty_bitmap(bs, bitmap);
>>
>> But what if the dirty bitmap was enabled before so that this enabling
>> transaction was supposed to be a no-op?
>
> Maybe it's not a problem: The only case in which the enable/disable 
> operation will fail is if it cannot find the device or bitmap -- in 
> which case, the abort operation isn't going to be able to affect 
> anything either.

Well, it's part of a transaction. As far as I understand it, one groups 
several operations in one transaction and if any fails, all are aborted. 
Therefore, the "enable the dirty bitmap" operation can trivially succeed 
(because it was enabled before), but some different operation may fail, 
which then results in invocation of this abort function.

Max

> Unless you know something I don't.
>
> Still, it does look goofy. Thoughts?