From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40719) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z35Sb-0000uq-7T for qemu-devel@nongnu.org; Thu, 11 Jun 2015 12:36:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z35SW-0004uC-7H for qemu-devel@nongnu.org; Thu, 11 Jun 2015 12:36:08 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59850) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z35SV-0004tD-BQ for qemu-devel@nongnu.org; Thu, 11 Jun 2015 12:36:03 -0400 Message-ID: <5579B8F1.4040606@redhat.com> Date: Thu, 11 Jun 2015 12:36:01 -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> <5578CB52.5030504@redhat.com> <557967C1.1010009@virtuozzo.com> In-Reply-To: <557967C1.1010009@virtuozzo.com> Content-Type: text/plain; charset=windows-1252 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: 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/11/2015 06:49 AM, Vladimir Sementsov-Ogievskiy wrote: > 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. > Follow Kevin's lead, here -- It looked strange to me, but it _is_ best to follow the existing style. I didn't look at the surrounding code too carefully. > >> >>> enum qcow2_discard_type { >>> QCOW2_DISCARD_NEVER = 0, >>> QCOW2_DISCARD_ALWAYS, >>> > >