From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34134) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUaYn-0006Fk-2s for qemu-devel@nongnu.org; Wed, 26 Aug 2015 09:16:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUaYi-0007Z5-N2 for qemu-devel@nongnu.org; Wed, 26 Aug 2015 09:16:12 -0400 Received: from mx2.parallels.com ([199.115.105.18]:59222) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUaYi-0007Uh-E4 for qemu-devel@nongnu.org; Wed, 26 Aug 2015 09:16:08 -0400 Message-ID: <55DDBC06.2080101@virtuozzo.com> Date: Wed, 26 Aug 2015 16:15:50 +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> In-Reply-To: <557B5550.502@redhat.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 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. > >> + 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.