Re: [Qemu-devel] [PATCH v9 02/10] qmp: Add block-dirty-bitmap-add and block-dirty-bitmap-remove

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2075 bytes --]

On Mon, Dec 01, 2014 at 03:30:08PM -0500, John Snow wrote:
> diff --git a/block.c b/block.c
> index e5c6ccf..3f27519 100644
> --- a/block.c
> +++ b/block.c
> @@ -5420,6 +5420,25 @@ int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t sector
>      }
>  }
>  
> +#define BDB_MIN_DEF_GRANULARITY 4096
> +#define BDB_MAX_DEF_GRANULARITY 65536
> +#define BDB_DEFAULT_GRANULARITY BDB_MAX_DEF_GRANULARITY
> +
> +uint64_t bdrv_dbm_calc_def_granularity(BlockDriverState *bs)

Long names are unwieldy but introducing multiple abbreviations is not a
good solution, it makes the code more confusing (BDB vs dbm).

I would call the function bdrv_get_default_bitmap_granularity().

The constants weren't necessary since the point of this function is to
capture the default value.  No one else should use the constants -
otherwise there is a high probability that they are reimplementing this
logic.  I would just leave them as literals in the code.

> diff --git a/blockdev.c b/blockdev.c
> index 5651a8e..4d30b09 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1894,6 +1894,60 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
>      aio_context_release(aio_context);
>  }
>  
> +void qmp_block_dirty_bitmap_add(const char *device, const char *name,
> +                                bool has_granularity, int64_t granularity,
> +                                Error **errp)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_lookup_bs(device, NULL, errp);

Markus: I think we need to support node-name here so dirty bitmaps can
be applied at any node in the graph?

> +    if (!bs) {
> +        return;
> +    }

These new monitor commands need to acquire/release the
BlockDriverState's AioContext like the other monitor commands.  This
ensures that BDS accesses in other threads are synchronized (stopped
while we run).

AioContext *aio_context;
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
...
out:
aio_context_release(aio_context);

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]