From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58450) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxfvJ-0008OX-4F for qemu-devel@nongnu.org; Fri, 21 Oct 2016 15:56:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxfvI-0004bt-5Y for qemu-devel@nongnu.org; Fri, 21 Oct 2016 15:56:13 -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> <5c32e4bd-a947-8b7b-f55d-f9b84c03d9e6@virtuozzo.com> From: Max Reitz Message-ID: <524e8350-025f-6ab0-763c-447fdaf72277@redhat.com> Date: Fri, 21 Oct 2016 21:56:01 +0200 MIME-Version: 1.0 In-Reply-To: <5c32e4bd-a947-8b7b-f55d-f9b84c03d9e6@virtuozzo.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="oARn0j3f3ai50Lb4M37LM5EWxUQ3ObQ1O" 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: Vladimir Sementsov-Ogievskiy , 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 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --oARn0j3f3ai50Lb4M37LM5EWxUQ3ObQ1O From: Max Reitz To: Vladimir Sementsov-Ogievskiy , 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 Message-ID: <524e8350-025f-6ab0-763c-447fdaf72277@redhat.com> Subject: Re: [PATCH 07/22] qcow2-bitmap: introduce auto-loading bitmaps 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> <5c32e4bd-a947-8b7b-f55d-f9b84c03d9e6@virtuozzo.com> In-Reply-To: <5c32e4bd-a947-8b7b-f55d-f9b84c03d9e6@virtuozzo.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 21.10.2016 13:59, Vladimir Sementsov-Ogievskiy wrote: > 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 a= re >>> loaded at image open and becomes BdrvDirtyBitmap's for corresponding >>> drive. These bitmaps are deleted from Qcow2 image after loading to av= oid >>> 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_ALWAYS); >>> + } >>> + >>> + return 0; >>> + >>> +fail: >>> + if (new_offset > 0) { >>> + qcow2_free_clusters(bs, new_offset, new_size, >>> QCOW2_DISCARD_ALWAYS); >>> + 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 b= e >> 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 a= nd >> freeing the new one. >> >> Max >=20 > Hmm.. Good point. Any suggestions? >=20 > I'm trying to achieve some kind of transaction - if function failed fil= e > is not changed.. But now I understand that because of flush ability to > fail I can't achieve this.. >=20 > Has write and flush any guaranties in case of fail? Would it be > old-or-new state, or some mixed state is possible? Anything is possible, I think. However, if flushing the data fails, it still means writing it was successful -- you just don't know whether it has been written to the disk successfully. The most correct way out would be to signal image corruption, but that isn't what we should do, I think. If the write operation was successful but flushing was not, I'd always assume the data hopefully gets flushed later and continue as if it had been written to disk successfully. Therefore, I think it would be best for update_header_sync() to just ignore bdrv_flush()'s return value. Max --oARn0j3f3ai50Lb4M37LM5EWxUQ3ObQ1O Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEvBAEBCAAZBQJYCnLREhxtcmVpdHpAcmVkaGF0LmNvbQAKCRD0B9sAYdXPQIxM B/9g8rE4Wxb+fcdYDR6pSZal5Jftnyz7Igksf0Q+gPdMF2uyJ4QOf0H12AuHV1OQ jRnmqFrSFmPUe/UcXpL24FXtcxPWG01UOn8cmK94ALb9kjddzIwfzwWetxaYDQHR m6N4b1kfB3jCMmUGnPiAtZW8lJp3mimf86BCMt2yI7X5R7Gs9MuObLX0rTb8Y9Ke IaetktBGwA0czHLv7LsWFD7Rx6VvX//GwMrc6Jh04Fx3w707Yy4NhCjFcEAdoJPp a53oYhIWkpYQfYcFXxpVBhHGbx/dAJH1TPBIHwyhI4F5nTTzzZDL5mnvngKUms6P OM0tzb9+kYGl5SZrc3MdsQZN =Z4Zs -----END PGP SIGNATURE----- --oARn0j3f3ai50Lb4M37LM5EWxUQ3ObQ1O--