From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50074) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUbTL-0004bx-2F for qemu-devel@nongnu.org; Wed, 26 Aug 2015 10:14:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUbTH-0003RV-MA for qemu-devel@nongnu.org; Wed, 26 Aug 2015 10:14:38 -0400 Received: from mx2.parallels.com ([199.115.105.18]:34230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUbTH-0003RL-DQ for qemu-devel@nongnu.org; Wed, 26 Aug 2015 10:14:35 -0400 Message-ID: <55DDC9C2.2000706@virtuozzo.com> Date: Wed, 26 Aug 2015 17:14:26 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1433776886-27239-1-git-send-email-vsementsov@virtuozzo.com> <1433776886-27239-3-git-send-email-vsementsov@virtuozzo.com> <557B5550.502@redhat.com> <55DDBC06.2080101@virtuozzo.com> In-Reply-To: <55DDBC06.2080101@virtuozzo.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/8] qcow2: add dirty-bitmaps feature List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, Vladimir Sementsov-Ogievskiy , stefanha@redhat.com, den@openvz.org 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 >>> >>> Adds dirty-bitmaps feature to qcow2 format as specified in >>> docs/specs/qcow2.txt >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> --- > ... >>> +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.