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

[Max Reitz]

On 2017-06-30 04:18, Eric Blake wrote:
> On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Store persistent dirty bitmaps in qcow2 image.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Reviewed-by: Max Reitz <address@hidden>
>> ---
>>  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...

It's 4:21 am here, so I cannot claim to be right, but if I remember
correctly, it will automatically aligned because sbc is the number of
bits (and thus sectors) a bitmap cluster covers.

> 
>> +        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.

I'm for fixing it later. I would have announced the series "applied" an
hour ago if I hadn't had to bisect iotest 055 breakage...

Max

signature.asc
Description: OpenPGP digital signature