From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55212) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPyRA-0003y7-Fx for qemu-devel@nongnu.org; Mon, 23 Feb 2015 14:13:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YPyR6-0006BD-1g for qemu-devel@nongnu.org; Mon, 23 Feb 2015 14:13:00 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35845) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPyR5-0006B6-Pv for qemu-devel@nongnu.org; Mon, 23 Feb 2015 14:12:55 -0500 Message-ID: <54EB7BB5.2030403@redhat.com> Date: Mon, 23 Feb 2015 14:12:53 -0500 From: John Snow MIME-Version: 1.0 References: <1424473645-29161-1-git-send-email-jsnow@redhat.com> <1424473645-29161-19-git-send-email-jsnow@redhat.com> <54EB5D66.2060304@redhat.com> In-Reply-To: <54EB5D66.2060304@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v14 18/19] block: Resize bitmaps on bdrv_truncate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 02/23/2015 12:03 PM, Max Reitz wrote: > On 2015-02-20 at 18:07, John Snow wrote: >> Signed-off-by: John Snow >> --- >> block.c | 20 ++++++++++++++++++++ >> include/block/block.h | 1 + >> include/qemu/hbitmap.h | 10 ++++++++++ >> util/hbitmap.c | 47 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 78 insertions(+) >> >> diff --git a/block.c b/block.c >> index f3a6dd4..59a8ec9 100644 >> --- a/block.c >> +++ b/block.c >> @@ -3514,6 +3514,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t >> offset) >> ret = drv->bdrv_truncate(bs, offset); >> if (ret == 0) { >> ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS); >> + bdrv_dirty_bitmap_truncate(bs); >> if (bs->blk) { >> blk_dev_resize_cb(bs->blk); >> } >> @@ -5524,6 +5525,25 @@ BdrvDirtyBitmap >> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> return parent; >> } >> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t >> size) >> +{ >> + assert(!bdrv_dirty_bitmap_frozen(bitmap)); > > This assertion is valid because the only way to make a bitmap frozen is > by starting a drive-backup block job on the BDS, and doing so blocks the > HMP block_resize command (and I don't know other ways to invoke > bdrv_truncate() online). > > (That means: It's fine. I just had to think about whether it is.) > >> + hbitmap_truncate(bitmap->bitmap, size); >> +} >> + >> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) >> +{ >> + BdrvDirtyBitmap *bitmap; >> + uint64_t size = bdrv_nb_sectors(bs); >> + >> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> + if (bdrv_dirty_bitmap_frozen(bitmap)) { >> + continue; >> + } >> + dirty_bitmap_truncate(bitmap, size); >> + } >> +} >> + >> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >> *bitmap) >> { >> BdrvDirtyBitmap *bm, *next; >> diff --git a/include/block/block.h b/include/block/block.h >> index f6a50ae..aa6912d 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -442,6 +442,7 @@ BdrvDirtyBitmap >> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >> uint32_t granularity, >> const char *name, >> Error **errp); >> +void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, >> BdrvDirtyBitmap *bitmap, >> Error **errp); >> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h >> index c19c1cb..a75157e 100644 >> --- a/include/qemu/hbitmap.h >> +++ b/include/qemu/hbitmap.h >> @@ -65,6 +65,16 @@ struct HBitmapIter { >> HBitmap *hbitmap_alloc(uint64_t size, int granularity); >> /** >> + * hbitmap_truncate: >> + * @hb: The bitmap to change the size of. >> + * @size: The number of elements to change the bitmap to accommodate. >> + * >> + * truncate or grow an existing bitmap to accommodate a new number of >> elements. >> + * This may invalidate existing HBitmapIterators. >> + */ >> +void hbitmap_truncate(HBitmap *hb, uint64_t size); >> + >> +/** >> * hbitmap_merge: >> * @a: The bitmap to store the result in. >> * @b: The bitmap to merge into @a. >> diff --git a/util/hbitmap.c b/util/hbitmap.c >> index 962ff29..d17b850 100644 >> --- a/util/hbitmap.c >> +++ b/util/hbitmap.c >> @@ -90,6 +90,9 @@ struct HBitmap { >> * bitmap will still allocate HBITMAP_LEVELS arrays. >> */ >> unsigned long *levels[HBITMAP_LEVELS]; >> + >> + /* The length of each levels[] array. */ >> + uint64_t sizes[HBITMAP_LEVELS]; >> }; >> /* Advance hbi to the next nonzero word and return it. hbi->pos >> @@ -384,6 +387,7 @@ HBitmap *hbitmap_alloc(uint64_t size, int >> granularity) >> hb->granularity = granularity; >> for (i = HBITMAP_LEVELS; i-- > 0; ) { >> size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1); >> + hb->sizes[i] = size; >> hb->levels[i] = g_new0(unsigned long, size); >> } >> @@ -396,6 +400,49 @@ HBitmap *hbitmap_alloc(uint64_t size, int >> granularity) >> return hb; >> } >> +void hbitmap_truncate(HBitmap *hb, uint64_t size) >> +{ >> + bool truncate; >> + unsigned i; >> + uint64_t num_elements = size; >> + >> + /* Size comes in as logical elements, adjust for granularity. */ >> + size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity; >> + assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE)); >> + truncate = size < hb->size; >> + >> + if (size == hb->size) { >> + /* A hard day's work */ >> + return; >> + } >> + >> + hb->size = size; >> + for (i = HBITMAP_LEVELS; i-- > 0; ) { > > By now I've heard that this is a relatively wide-spread pattern; but I > still don't like it because I find it pretty hard to figure out that it > does what it's supposed to if you've never seen it before (and with "by > now I've heard" I mean that I read some blog post where someone > mentioned that he didn't like it). > > At least it's not "i --> 0", so I guess I should be fine with it. > > (and with "by now" I mean that I'm the one responsible for it not being > "-->" in hbitmap_merge()) > A copy-paste loop from hbitmap_alloc is why this pattern keeps emerging. It's not necessarily how I'd do it, either. >> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1); >> + if (hb->sizes[i] == size) { >> + continue; >> + } >> + hb->sizes[i] = size; >> + hb->levels[i] = g_realloc_n(hb->levels[i], size, >> sizeof(unsigned long)); > > How about initializing the new memory in case of truncate == false? I'm > not sure whether to initialize it to 0 or 1, though. Probably 0. > Good call; I had actually thought that g_realloc did initialize the memory, but looking at the docs again: nope. >> + } >> + assert(size == 1); >> + >> + /* Clear out any "extra space" we may have that the user didn't >> request: >> + * It may have garbage data in it, now. */ >> + if (truncate) { >> + /* Due to granularity fuzziness, we may accidentally reset >> some of >> + * the last bits that are actually valid. So, record the >> current value, >> + * reset the "dead range," then re-set the one sector we care >> about. */ >> + bool status = hbitmap_get(hb, num_elements - 1); >> + uint64_t fix_count = (hb->size << hb->granularity) - >> num_elements; >> + hbitmap_reset(hb, num_elements, fix_count); > > This can fail an assertion (or at least I believe it's this function, it > may be the hbitmap_set() below, too (but status should be false, so I > don't think that)): > > ./qemu-img create -f raw test.img 64M; echo > "{'execute':'qmp_capabilities'}{'execute':'block-dirty-bitmap-add','arguments':{'node':'drv','name':'foo'}}{'execute':'human-monitor-command','arguments':{'command-line':'block_resize > drv 32M'}}" | x86_64-softmmu/qemu-system-x86_64 -drive > if=none,id=drv,file=test.img,format=raw -qmp stdio -machine accel=qtest > -nodefaults -display none > Formatting 'test.img', fmt=raw size=67108864 > {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, > "package": ""}, "capabilities": []}} > {"return": {}} > {"return": {}} > qemu-system-x86_64: util/hbitmap.c:149: hbitmap_iter_init: Assertion > `pos < hb->size' failed. > > It's probably because fix_count == 0 and therefore num_elements is not > an index into the bitmap (which hbitmap_reset() and subsequently > hb_count_between() don't take into account). > > Max > Ah, indeed. If you hit a perfect even multiple where there are no bits at all to clean, you wind up with a one-past-the-end, even though the operation should be a no-op. >> + if (status) { >> + hbitmap_set(hb, num_elements - 1, 1); >> + } >> + } >> +} >> + >> + >> /** >> * Given HBitmaps A and B, let A := A (BITOR) B. >> * Bitmap B will not be modified. >