From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55170) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxYUB-0006HF-Jb for qemu-devel@nongnu.org; Fri, 21 Oct 2016 07:59:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxYUA-0002kI-NA for qemu-devel@nongnu.org; Fri, 21 Oct 2016 07:59:43 -0400 Received: from mailhub.sw.ru ([195.214.232.25]:30275 helo=relay.sw.ru) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bxYUA-0002k0-BM for qemu-devel@nongnu.org; Fri, 21 Oct 2016 07:59:42 -0400 References: <1475232808-4852-1-git-send-email-vsementsov@virtuozzo.com> <1475232808-4852-8-git-send-email-vsementsov@virtuozzo.com> <0259023b-a592-9b3a-cb06-09013080a40b@redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <5c32e4bd-a947-8b7b-f55d-f9b84c03d9e6@virtuozzo.com> Date: Fri, 21 Oct 2016 14:59:24 +0300 MIME-Version: 1.0 In-Reply-To: <0259023b-a592-9b3a-cb06-09013080a40b@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org, qemu-devel@nongnu.org Cc: kwolf@redhat.com, armbru@redhat.com, eblake@redhat.com, jsnow@redhat.com, famz@redhat.com, den@openvz.org, stefanha@redhat.com, pbonzini@redhat.com 07.10.2016 22:25, Max Reitz =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote: >> Auto loading bitmaps are bitmaps in Qcow2, with AUTO flag set. They ar= e >> loaded at image open and becomes BdrvDirtyBitmap's for corresponding >> drive. These bitmaps are deleted from Qcow2 image after loading to avo= id >> conflicts. >> >> Extra data in bitmaps is not supported for now. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy >> --- >> block/qcow2-bitmap.c | 518 +++++++++++++++++++++++++++++++++++++++++= +++++++++- >> block/qcow2.c | 5 + >> block/qcow2.h | 3 + >> 3 files changed, 525 insertions(+), 1 deletion(-) >> >> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c >> index cd18b07..760a047 100644 >> --- a/block/qcow2-bitmap.c >> +++ b/block/qcow2-bitmap.c > [...] > >> +static int directory_update(BlockDriverState *bs, uint8_t *new_dir, >> + size_t new_size, uint32_t new_nb_bitmaps) >> +{ >> + BDRVQcow2State *s =3D bs->opaque; >> + int ret; >> + int64_t new_offset =3D 0; >> + uint64_t old_offset =3D s->bitmap_directory_offset; >> + uint64_t old_size =3D s->bitmap_directory_size; >> + uint32_t old_nb_bitmaps =3D s->nb_bitmaps; >> + uint64_t old_autocl =3D s->autoclear_features; >> + >> + if (new_size > QCOW_MAX_DIRTY_BITMAP_DIRECTORY_SIZE) { >> + return -EINVAL; >> + } >> + >> + if ((new_size =3D=3D 0) !=3D (new_nb_bitmaps =3D=3D 0)) { >> + return -EINVAL; >> + } >> + >> + if (new_nb_bitmaps > 0) { >> + new_offset =3D directory_write(bs, new_dir, new_size); >> + if (new_offset < 0) { >> + return new_offset; >> + } >> + >> + ret =3D bdrv_flush(bs); >> + if (ret < 0) { >> + goto fail; >> + } >> + } >> + s->bitmap_directory_offset =3D new_offset; >> + s->bitmap_directory_size =3D new_size; >> + s->nb_bitmaps =3D new_nb_bitmaps; >> + >> + ret =3D update_header_sync(bs); >> + if (ret < 0) { >> + goto fail; >> + } >> + >> + if (old_size) { >> + qcow2_free_clusters(bs, old_offset, old_size, QCOW2_DISCARD_A= LWAYS); >> + } >> + >> + return 0; >> + >> +fail: >> + if (new_offset > 0) { >> + qcow2_free_clusters(bs, new_offset, new_size, QCOW2_DISCARD_A= LWAYS); >> + s->bitmap_directory_offset =3D old_offset; >> + s->bitmap_directory_size =3D old_size; >> + s->nb_bitmaps =3D old_nb_bitmaps; >> + s->autoclear_features =3D old_autocl; > What if bdrv_flush() in update_header_sync() failed? Then you cannot be > sure what bitmap directory the header is actually pointing to. In that > case, it would be wrong to assume it's still pointing to the old one an= d > freeing the new one. > > Max Hmm.. Good point. Any suggestions? I'm trying to achieve some kind of transaction - if function failed file=20 is not changed.. But now I understand that because of flush ability to=20 fail I can't achieve this.. Has write and flush any guaranties in case of fail? Would it be=20 old-or-new state, or some mixed state is possible? > >> + } >> + >> + return ret; >> +} --=20 Best regards, Vladimir