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

[Max Reitz <mreitz@redhat.com>]

On 2014-11-25 at 20:46, John Snow wrote:
> From: Fam Zheng <famz@redhat.com>
>
> 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>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c       | 147 +++++++++++++++++++++++++++++++++++++++++++------------
>   qapi-schema.json |   5 +-
>   2 files changed, 119 insertions(+), 33 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index adf841a..b98249b 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1497,6 +1497,106 @@ 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_lookup_bs(action->device, NULL, NULL);
> +    if (bs) {
> +        bm = bdrv_find_dirty_bitmap(bs, action->name);
> +        if (bm) {
> +            bdrv_release_dirty_bitmap(bs, bm);
> +        }
> +    }
> +}
> +
> +typedef struct BlockDirtyBitmapState {
> +    BlkTransactionState common;
> +    BdrvDirtyBitmap *bitmap;
> +} BlockDirtyBitmapState;
> +
> +static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
> +                                                  const char *name,
> +                                                  Error **errp)
> +{
> +    BlockDriverState *bs;
> +    BdrvDirtyBitmap *bitmap;
> +    Error *local_err = NULL;
> +
> +    if (!device) {
> +        error_setg(errp, "Device cannot be NULL");
> +        return NULL;
> +    }
> +    if (!name) {
> +        error_setg(errp, "Bitmap name cannot be NULL");
> +        return NULL;
> +    }
> +
> +    bs = bdrv_lookup_bs(device, NULL, &local_err);
> +    if (!bs) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +
> +    bitmap = bdrv_find_dirty_bitmap(bs, name);
> +    if (!bitmap) {
> +        error_setg(errp, "Dirty bitmap not found: %s", name);
> +        return NULL;
> +    }
> +
> +    return bitmap;
> +}
> +
> +/**
> + * Enable and Disable re-uses the same preparation.
> + */
> +static void block_dirty_bitmap_en_toggle_prepare(BlkTransactionState *common,
> +                                                 Error **errp)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    BlockDirtyBitmap *action;
> +    Error *local_err = NULL;
> +
> +    action = common->action->block_dirty_bitmap_enable;

common->action is a union, so this works for *disable, too (which has 
the same time as *enable); but I wouldn't object a comment here that 
this is indeed the case.

Or go into pedantic mode and add an assert(action == 
common->action->block_dirty_bitmap_disable).

> +
> +    state->bitmap = block_dirty_bitmap_lookup(action->device,
> +                                              action->name,
> +                                              &local_err);
> +    if (!state->bitmap) {
> +        error_propagate(errp, local_err);

Again, no need for local_err or error_propagate().

Because I feel like I owe you an explanation by now why there is 
local_err at all in so many places: For some functions, it is not 
possible to discern whether they were successful or not just by looking 
at the return value. Therefore, the only way to decide whether they 
failed is by looking whether they set the Error object or not. However, 
errp may be NULL, therefore in those places we cannot just look at 
*errp. In those cases, we have to use a local Error object which is 
passed to the function and which we then can use to determine failure or 
success; on failure, it then needs to be propagated. A good example is 
block_dirty_bitmap_en_toggle_prepare() itself which does not return any 
value.

However, in all the places in this series where I said you could drop 
the error_propagate() and local_err invoked functions which did return 
error values; like here, where !state->bitmap indicates an error. 
Therefore, we don't need local_err here.

> +        return;
> +    }
> +}
> +
> +static void block_dirty_bitmap_enable_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_enable_dirty_bitmap(NULL, state->bitmap);

This does work for now, but then I'm asking myself why 
bdrv_enable_dirty_bitmap() takes a BDS pointer at all. I thought maybe 
in the future the BDS pointer may come in handy, but with this call 
you're basically saying that the function will never need the BDS pointer.

It's better to omit the BDS pointer from bdrv_enable_dirty_bitmap(), 
then. If we do notice a need for it later on, the compiler will tell us 
the places where we don't pass it. Like this, if the function evaluates 
the parameter in the future, it will be a runtime problem.

> +}
> +
> +static void block_dirty_bitmap_disable_commit(BlkTransactionState *common)
> +{
> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
> +                                             common, common);
> +    bdrv_disable_dirty_bitmap(NULL, state->bitmap);

Same here, of course.

> +}
> +
>   static void abort_prepare(BlkTransactionState *common, Error **errp)
>   {
>       error_setg(errp, "Transaction aborted using Abort action");
> @@ -1529,6 +1629,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(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_en_toggle_prepare,
> +        .commit = block_dirty_bitmap_enable_commit,
> +    },
> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
> +        .instance_size = sizeof(BlockDirtyBitmapState),
> +        .prepare = block_dirty_bitmap_en_toggle_prepare,
> +        .commit = block_dirty_bitmap_disable_commit,
> +    },
>   };
>   
>   /*
> @@ -1875,38 +1990,6 @@ void qmp_block_dirty_bitmap_remove(const char *device, const char *name,
>       bdrv_release_dirty_bitmap(bs, bitmap);
>   }
>   
> -static BdrvDirtyBitmap *block_dirty_bitmap_lookup(const char *device,
> -                                                  const char *name,
> -                                                  Error **errp)
> -{
> -    BlockDriverState *bs;
> -    BdrvDirtyBitmap *bitmap;
> -    Error *local_err = NULL;
> -
> -    if (!device) {
> -        error_setg(errp, "Device cannot be NULL");
> -        return NULL;
> -    }
> -    if (!name) {
> -        error_setg(errp, "Bitmap name cannot be NULL");
> -        return NULL;
> -    }
> -
> -    bs = bdrv_lookup_bs(device, NULL, &local_err);
> -    if (!bs) {
> -        error_propagate(errp, local_err);
> -        return NULL;
> -    }
> -
> -    bitmap = bdrv_find_dirty_bitmap(bs, name);
> -    if (!bitmap) {
> -        error_setg(errp, "Dirty bitmap not found: %s", name);
> -        return NULL;
> -    }
> -
> -    return bitmap;
> -}
> -

Is it possible to move this function further up in 6 where it is 
created? If so, it doesn't need to be moved in this patch (and moving 
code blocks always looks a bit ugly in patches...).

Technically, the patch is fine. If it weren't for BDS issue with the 
bdrv_{en,dis}able_dirty_bitmap() calls, I'd give it an R-b.

Max