|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature |
Date: | Wed, 26 Aug 2015 17:14:26 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 |
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 <address@hidden> Adds dirty-bitmaps feature to qcow2 format as specified in docs/specs/qcow2.txt Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden> ---...+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.
[Prev in Thread] | Current Thread | [Next in Thread] |