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 <vsement...@virtuozzo.com>
>> Reviewed-by: Max Reitz <mre...@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...
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
