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 <vsement...@parallels.com>
Adds dirty-bitmaps feature to qcow2 format as specified in
docs/specs/qcow2.txt
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@parallels.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@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.