From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z303U-0002Ho-H2 for qemu-devel@nongnu.org; Thu, 11 Jun 2015 06:49:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z303O-0007eF-K2 for qemu-devel@nongnu.org; Thu, 11 Jun 2015 06:49:52 -0400 Received: from mx2.parallels.com ([199.115.105.18]:54133) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z303O-0007dn-B3 for qemu-devel@nongnu.org; Thu, 11 Jun 2015 06:49:46 -0400 Message-ID: <557967C1.1010009@virtuozzo.com> Date: Thu, 11 Jun 2015 13:49:37 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1433776886-27239-1-git-send-email-vsementsov@virtuozzo.com> <1433776886-27239-7-git-send-email-vsementsov@virtuozzo.com> <5578CB52.5030504@redhat.com> In-Reply-To: <5578CB52.5030504@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit 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: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, Vladimir Sementsov-Ogievskiy , stefanha@redhat.com, den@openvz.org On 11.06.2015 02:42, John Snow wrote: > > On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: >> From: Vladimir Sementsov-Ogievskiy >> >> 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(-) >> >> 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(BlockDriverState *bs) >> >> s->dirty_bitmaps_offset = dirty_bitmaps_offset; >> s->dirty_bitmaps_size = dirty_bitmaps_size; >> + if (s->nb_dirty_bitmaps > 0) { >> + s->autoclear_features |= QCOW2_AUTOCLEAR_DIRTY_BITMAPS; >> + } else { >> + s->autoclear_features &= ~QCOW2_AUTOCLEAR_DIRTY_BITMAPS; >> + } >> ret = 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; >> } >> >> + if (!(s->autoclear_features & QCOW2_AUTOCLEAR_DIRTY_BITMAPS) && >> + s->nb_dirty_bitmaps > 0) { >> + ret = qcow2_delete_all_dirty_bitmaps(bs, errp); >> + if (ret < 0) { >> + return ret; >> + } >> + } >> + >> #ifdef DEBUG_EXT >> printf("Qcow2: Got dirty bitmaps extension:" >> " offset=%" PRIu64 " nb_bitmaps=%" PRIu32 "\n", >> @@ -928,8 +936,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, >> } >> >> /* Clear unknown autoclear feature bits */ >> - if (!bs->read_only && !(flags & BDRV_O_INCOMING) && s->autoclear_features) { >> - s->autoclear_features = 0; >> + if (!bs->read_only && !(flags & BDRV_O_INCOMING) && >> + (s->autoclear_features & ~QCOW2_AUTOCLEAR_MASK)) { >> + s->autoclear_features |= QCOW2_AUTOCLEAR_MASK; > Like Stefan already mentioned, fixing this |= to &= will fix iotest 036, > which is otherwise broken by this patch. > >> ret = qcow2_update_header(bs); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Could not update qcow2 header"); >> 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 = QCOW2_COMPAT_LAZY_REFCOUNTS, >> }; >> >> +/* Autoclear feature bits */ >> +enum { >> + QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR = 0, >> + QCOW2_AUTOCLEAR_DIRTY_BITMAPS = >> + 1 << QCOW2_AUTOCLEAR_DIRTY_BITMAPS_BITNR, >> + >> + QCOW2_AUTOCLEAR_MASK = QCOW2_AUTOCLEAR_DIRTY_BITMAPS, >> +}; >> + > 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 = 0, > QCOW2_AUTOCLEAR_DIRTY_BITMAPS = 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. This enum is made like enums for QCOW2_INCOMPAT_* and QCOW2_COMPAT_*, which are already in the code... Then, may I make a patch for them too? I agree, it is strange solution to put things of different nature to one enum. > >> enum qcow2_discard_type { >> QCOW2_DISCARD_NEVER = 0, >> QCOW2_DISCARD_ALWAYS, >> -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.