From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54174) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2pdS-0006Va-2F for qemu-devel@nongnu.org; Wed, 10 Jun 2015 19:42:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z2pdN-0000sk-Jo for qemu-devel@nongnu.org; Wed, 10 Jun 2015 19:42:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z2pdN-0000sa-CY for qemu-devel@nongnu.org; Wed, 10 Jun 2015 19:42:13 -0400 Message-ID: <5578CB52.5030504@redhat.com> Date: Wed, 10 Jun 2015 19:42:10 -0400 From: John Snow MIME-Version: 1.0 References: <1433776886-27239-1-git-send-email-vsementsov@virtuozzo.com> <1433776886-27239-7-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1433776886-27239-7-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 6/8] qcow2: add autoclear bit for dirty bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, Vladimir Sementsov-Ogievskiy , stefanha@redhat.com, den@openvz.org On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > block/qcow2-dirty-bitmap.c | 5 +++++ > block/qcow2.c | 13 +++++++++++-- > block/qcow2.h | 9 +++++++++ > 3 files changed, 25 insertions(+), 2 deletions(-) >=20 > diff --git a/block/qcow2-dirty-bitmap.c b/block/qcow2-dirty-bitmap.c > index db83112..686a121 100644 > --- a/block/qcow2-dirty-bitmap.c > +++ b/block/qcow2-dirty-bitmap.c > @@ -188,6 +188,11 @@ static int qcow2_write_dirty_bitmaps(BlockDriverSt= ate *bs) > =20 > s->dirty_bitmaps_offset =3D dirty_bitmaps_offset; > s->dirty_bitmaps_size =3D dirty_bitmaps_size; > + if (s->nb_dirty_bitmaps > 0) { > + s->autoclear_features |=3D QCOW2_AUTOCLEAR_DIRTY_BITMAPS; > + } else { > + s->autoclear_features &=3D ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; > + } > ret =3D qcow2_update_header(bs); > if (ret < 0) { > fprintf(stderr, "Could not update qcow2 header\n"); > diff --git a/block/qcow2.c b/block/qcow2.c > index 406e55d..f85a55a 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -182,6 +182,14 @@ static int qcow2_read_extensions(BlockDriverState = *bs, uint64_t start_offset, > return ret; > } > =20 > + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAP= S) && > + s->nb_dirty_bitmaps > 0) { > + ret =3D qcow2_delete_all_dirty_bitmaps(bs, errp); > + if (ret < 0) { > + return ret; > + } > + } > + > #ifdef DEBUG_EXT > printf("Qcow2: Got dirty bitmaps extension:" > " offset=3D%" PRIu64 " nb_bitmaps=3D%" PRIu32 "\n", > @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *= options, int flags, > } > =20 > /* Clear unknown autoclear feature bits */ > - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_f= eatures) { > - s->autoclear_features =3D 0; > + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && > + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { > + s->autoclear_features |=3D QCOW2_AUTOCLEAR_MASK; Like Stefan already mentioned, fixing this |=3D to &=3D will fix iotest 0= 36, which is otherwise broken by this patch. > ret =3D qcow2_update_header(bs); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not update qcow2 heade= r"); > diff --git a/block/qcow2.h b/block/qcow2.h > index b5e576c..14bd6f9 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -215,6 +215,15 @@ enum { > QCOW2_COMPAT_FEAT_MASK =3D QCOW2_COMPAT_LAZY_REFCOUNTS, > }; > =20 > +/* Autoclear feature bits */ > +enum { > + QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR =3D 0, > + QCOW2_AUTOCLEAR_DIRTY_BITMAPS =3D > + 1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR, > + > + QCOW2_AUTOCLEAR_MASK =3D QCOW2_AUTOCLEAR_DIRTY_BITM= APS, > +}; > + I find it a little awkward to have an enum with three different kinds of data in it, unless I am reading this incorrectly. (bit position, bit masks, and accumulated bit mask.) Just enumerating the indices is probably sufficient: enum { QCOW2_AUTOCLEAR_BEGIN =3D 0, QCOW2_AUTOCLEAR_DIRTY_BITMAPS =3D QCOW2_AUTOCLEAR_BEGIN, ..., QCOW2_AUTOCLEAR_END } and then the QCOW2_AUTOCLEAR_MASK can either be programmatically defined via a function, or just pre-computed as a #define. If you still want the mask definitions, you could do something cheeky like this: #define AUTOCLEAR_MASK(X) (1 << QCOW2_AUTOCLEAR_ ## X) and then you can use things like AUTOCLEAR_MASK(DIRTY_BITMAPS) without having to create and maintain two separate tables if you want both forms easily available. > enum qcow2_discard_type { > QCOW2_DISCARD_NEVER =3D 0, > QCOW2_DISCARD_ALWAYS, >=20