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

[John Snow <jsnow@redhat.com>]

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.

Unless you know something I don't.

Still, it does look goofy. Thoughts?

>> +        }
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_disable_prepare(BlkTransactionState
>> *common,
>> +                                               Error **errp)
>> +{
>> +    BlockDirtyBitmap *action;
>> +
>> +    action = common->action->block_dirty_bitmap_disable;
>> +    qmp_block_dirty_bitmap_disable(action->device, action->name, errp);
>> +}
>> +
>> +static void block_dirty_bitmap_disable_abort(BlkTransactionState
>> *common)
>> +{
>> +    BlockDirtyBitmap *action;
>> +    BdrvDirtyBitmap *bitmap;
>> +    BlockDriverState *bs;
>> +
>> +    action = common->action->block_dirty_bitmap_disable;
>> +    bs = bdrv_find(action->device);
>> +    if (bs) {
>> +        bitmap = bdrv_find_dirty_bitmap(bs, action->name);
>> +        if (bitmap) {
>> +            bdrv_enable_dirty_bitmap(bs, bitmap);
>
> Same here, the other way round.
>
>> +        }
>> +    }
>> +}
>> +
>>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -1522,6 +1599,21 @@ static const BdrvActionOps actions[] = {
>>           .prepare  = internal_snapshot_prepare,
>>           .abort = internal_snapshot_abort,
>>       },
>> +    [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(BlkTransactionState),
>> +        .prepare = block_dirty_bitmap_enable_prepare,
>> +        .abort = block_dirty_bitmap_enable_abort,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
>> +        .instance_size = sizeof(BlkTransactionState),
>> +        .prepare = block_dirty_bitmap_disable_prepare,
>> +        .abort = block_dirty_bitmap_disable_abort,
>> +    },
>>   };
>>   /*
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 24379ab..ae07677 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1260,7 +1260,10 @@
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>>          'drive-backup': 'DriveBackup',
>>          'abort': 'Abort',
>> -       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
>> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> +       'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>
> Ah, you're reusing that type here. Very well then.
>
> Max
>
>> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap'
>>      } }
>>   ##
>
>

-- 
—js