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

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

On 26.08.2015 16:15, Vladimir Sementsov-Ogievskiy wrote:
> 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.
>

Oh, sorry. You mean,
ret = qcow2_dirty_bitmap_create ...
if (ret < 0) {
return ret;
}

, ok

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