From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59482) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPwQ4-0006lL-KY for qemu-devel@nongnu.org; Mon, 23 Feb 2015 12:03:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YPwQ0-0003oN-D9 for qemu-devel@nongnu.org; Mon, 23 Feb 2015 12:03:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45167) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YPwQ0-0003oB-0x for qemu-devel@nongnu.org; Mon, 23 Feb 2015 12:03:40 -0500 Message-ID: <54EB5D66.2060304@redhat.com> Date: Mon, 23 Feb 2015 12:03:34 -0500 From: Max Reitz MIME-Version: 1.0 References: <1424473645-29161-1-git-send-email-jsnow@redhat.com> <1424473645-29161-19-git-send-email-jsnow@redhat.com> In-Reply-To: <1424473645-29161-19-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=iso-8859-15; 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: John Snow , qemu-devel@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com 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()) > + 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. > + } > + 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 > + 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.