Re: [Qemu-devel] [PATCH v22 19/30] qcow2: add persistent dirty bitmaps support

[Eric Blake <eblake@redhat.com>]

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

On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> Store persistent dirty bitmaps in qcow2 image.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-bitmap.c | 475 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c        |   9 +
>  block/qcow2.h        |   1 +
>  3 files changed, 485 insertions(+)
> 

> +
> +/* store_bitmap_data()
> + * Store bitmap to image, filling bitmap table accordingly.
> + */
> +static uint64_t *store_bitmap_data(BlockDriverState *bs,
> +                                   BdrvDirtyBitmap *bitmap,
> +                                   uint32_t *bitmap_table_size, Error **errp)
> +{
> +    int ret;
> +    BDRVQcow2State *s = bs->opaque;
> +    int64_t sector;
> +    uint64_t sbc;
> +    uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);

This grabs the size (currently in sectors, although I plan to fix it to
be in bytes)...

> +    const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
> +    uint8_t *buf = NULL;
> +    BdrvDirtyBitmapIter *dbi;
> +    uint64_t *tb;
> +    uint64_t tb_size =
> +            size_to_clusters(s,
> +                bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size));

...then finds out how many bytes are required to serialize the entire
image, where bm_size should be the same as before...

> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> +        uint64_t cluster = sector / sbc;
> +        uint64_t end, write_size;
> +        int64_t off;
> +
> +        sector = cluster * sbc;
> +        end = MIN(bm_size, sector + sbc);
> +        write_size =
> +            bdrv_dirty_bitmap_serialization_size(bitmap, sector, end - sector);

But here, rather than tackling the entire image at once, you are
subdividing your queries along arbitrary lines. But nowhere do I see a
call to bdrv_dirty_bitmap_serialization_align() to make sure your
end-sector value is properly aligned; if it is not aligned, you will
trigger an assertion failure here...

> +        assert(write_size <= s->cluster_size);
> +
> +        off = qcow2_alloc_clusters(bs, s->cluster_size);
> +        if (off < 0) {
> +            error_setg_errno(errp, -off,
> +                             "Failed to allocate clusters for bitmap '%s'",
> +                             bm_name);
> +            goto fail;
> +        }
> +        tb[cluster] = off;
> +
> +        bdrv_dirty_bitmap_serialize_part(bitmap, buf, sector, end - sector);

...and again here, during serialization_chunk().

I don't know if that means you need a v23 series, or if we can just
patch it in as a followup (perhaps by having me add the usage during my
byte-based dirty-bitmap series).  I guess it depends on whether we can
come up with any bitmap granularity (between 512 bytes and 2M is all the
more we are currently supporting, right?) that differs from the qcow2
cluster granularity in a manner that iteration by qcow2 clusters is no
longer guaranteed to be bitmap-granularity aligned, and thus trigger an
assertion failure on your code as-is.

I also think it's pretty gross to be calculating the iteration bounds by
sectors rather than bytes, when we are really worried about clusters
(it's easier to track just bytes/clusters than it is to track
bytes/sectors/clusters) - but that one is more along the lines of the
sector-to-byte conversions I've been tackling, so I won't insist on you
changing it if there is no other reason for a v23.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]