Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 13.06.2015 00:55, John Snow wrote:
>
> On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
>> From: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>>
>> Adds dirty-bitmaps feature to qcow2 format as specified in
>> docs/specs/qcow2.txt
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@parallels.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
...
>> +int qcow2_dirty_bitmap_store(BlockDriverState *bs, uint8_t *buf,
>> +                            const char *name, uint64_t size,
>> +                            int granularity)
>> +{
>> +    BDRVQcowState *s = bs->opaque;
>> +    int cl_size = s->cluster_size;
>> +    int i, dirty_bitmap_index, ret = 0, n;
>> +    uint64_t *l1_table;
>> +    QCowDirtyBitmap *bm;
>> +    uint64_t buf_size;
>> +    uint8_t *p;
>> +    int sector_granularity = granularity >> BDRV_SECTOR_BITS;
>> +
>> +    /* find/create dirty bitmap */
>> +    dirty_bitmap_index = find_dirty_bitmap_by_name(bs, name);
>> +    if (dirty_bitmap_index >= 0) {
>> +        bm = s->dirty_bitmaps + dirty_bitmap_index;
>> +
>> +        if (size != bm->bitmap_size ||
>> +            granularity != bm->bitmap_granularity) {
>> +            qcow2_dirty_bitmap_delete(bs, name, NULL);
> If this fails, we should 'return ret'.
>
>> +            dirty_bitmap_index = -1;
>> +        }
>> +    }
> Oh, find_dirty_bitmap_by_name only looks by name, but then you check to
> make sure the size and granularity matches. If it doesn't, you actually
> create a new bitmap with the *same name* but different attributes, and
> delete the old one.
>
> Is that appropriate? I guess if we're already here in store, it means we
> made it past the add checks... which means for whatever reason we
> definitely want to store *this* bitmap...
>
> I think this code is a little extraneous, it might be best to just issue
> an ultimatum that "You can't have two bitmaps with the same name in a
> file." and let that be that -- finding something with the wrong size
> would just simply be an error.
>
>> +    if (dirty_bitmap_index < 0) {
>> +        qcow2_dirty_bitmap_create(bs, name, size, granularity);
> If this fails, we need to return ret immediately.

Not agree. I think it's ok for qcow2_dirty_bitmap_store to store given 
bitmap if it can. It can in two cases (in next patchset version):

1) found the bitmap with the same name, size and granularity: it is 
assumed to be the previous version and will be rewritten
2) not found the bitmap: it's ok, just save it.. This case works when 
the bitmap was created while qemu runs.



>
>> +        dirty_bitmap_index = s->nb_dirty_bitmaps - 1;
>> +    }
>> +    bm = s->dirty_bitmaps + dirty_bitmap_index;
>> +
>> +    /* read l1 table */
>> +    l1_table = g_malloc(bm->l1_size * sizeof(uint64_t));
>> +    ret = bdrv_pread(bs->file, bm->l1_table_offset, l1_table,
>> +                     bm->l1_size * sizeof(uint64_t));
>> +    if (ret < 0) {
>> +        goto finish;
>> +    }
>> +
>> +    buf_size = (((size - 1) / sector_granularity) >> 3) + 1;
>> +    buf_size = align_offset(buf_size, 4);
>> +    n = buf_size / cl_size;
>> +    p = buf;
>> +    for (i = 0; i < bm->l1_size; ++i) {
>> +        uint64_t addr = be64_to_cpu(l1_table[i]) & ~511;
>> +        int write_size = (i == n ? (buf_size % cl_size) : cl_size);
>> +
>> +        if (buffer_is_zero(p, write_size)) {
>> +            if (addr) {
>> +                qcow2_free_clusters(bs, addr, cl_size,
>> +                                    QCOW2_DISCARD_ALWAYS);
>> +            }
>> +            l1_table[i] = cpu_to_be64(1);
>> +        } else {
>> +            if (!addr) {
>> +                addr = qcow2_alloc_clusters(bs, cl_size);
>> +                l1_table[i] = cpu_to_be64(addr);
>> +            }
>> +
>> +            ret = bdrv_pwrite(bs->file, addr, p, write_size);
>> +            if (ret < 0) {
>> +                goto finish;
>> +            }
>> +        }
>> +
>> +        p += cl_size;
>> +    }
>> +
>> +    ret = bdrv_pwrite(bs->file, bm->l1_table_offset, l1_table,
>> +                      bm->l1_size * sizeof(uint64_t));
>> +    if (ret < 0) {
>> +        goto finish;
>> +    }
>> +
>> +finish:
>> +    g_free(l1_table);
>> +    return ret;
>> +}
>> +/* if no id is provided, a new one is constructed */
>> +int qcow2_dirty_bitmap_create(BlockDriverState *bs, const char *name,

-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.