From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58933) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXv7E-0001do-KB for qemu-devel@nongnu.org; Tue, 17 Mar 2015 13:17:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXv7A-00026Z-Bx for qemu-devel@nongnu.org; Tue, 17 Mar 2015 13:17:16 -0400 Message-ID: <55086195.1060806@redhat.com> Date: Tue, 17 Mar 2015 13:17:09 -0400 From: Max Reitz MIME-Version: 1.0 References: <1426271419-8277-1-git-send-email-jsnow@redhat.com> <1426271419-8277-16-git-send-email-jsnow@redhat.com> <5508311D.1070301@redhat.com> <550860D1.4050007@redhat.com> In-Reply-To: <550860D1.4050007@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [2.4 PATCH v3 15/19] block: Resize bitmaps on bdrv_truncate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , qemu-block@nongnu.org Cc: kwolf@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com On 2015-03-17 at 13:13, John Snow wrote: > > > On 03/17/2015 09:50 AM, Max Reitz wrote: >> On 2015-03-13 at 14:30, John Snow wrote: >>> Signed-off-by: John Snow >>> --- >>> block.c | 18 +++++++++++++++++ >>> include/qemu/hbitmap.h | 10 ++++++++++ >>> util/hbitmap.c | 52 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index 1eee394..f40b014 100644 >>> --- a/block.c >>> +++ b/block.c >>> @@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs, >>> int64_t cur_sector, >>> int nr_sectors); >>> static void bdrv_reset_dirty(BlockDriverState *bs, int64_t >>> cur_sector, >>> int nr_sectors); >>> +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); >>> /* If non-zero, use only whitelisted block drivers */ >>> static int use_bdrv_whitelist; >>> @@ -3543,6 +3544,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); >>> } >>> @@ -5562,6 +5564,22 @@ BdrvDirtyBitmap >>> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >>> return parent; >>> } >>> +/** >>> + * Truncates _all_ bitmaps attached to a BDS. >>> + */ >>> +static 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; >>> + } >>> + hbitmap_truncate(bitmap->bitmap, size); >>> + } >>> +} >>> + >>> void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap >>> *bitmap) >>> { >>> BdrvDirtyBitmap *bm, *next; >>> 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 ba11fd3..4505ef7 100644 >>> --- a/util/hbitmap.c >>> +++ b/util/hbitmap.c >>> @@ -400,6 +400,58 @@ HBitmap *hbitmap_alloc(uint64_t size, int >>> granularity) >>> return hb; >>> } >>> +void hbitmap_truncate(HBitmap *hb, uint64_t size) >>> +{ >>> + bool shrink; >>> + unsigned i; >>> + uint64_t num_elements = size; >>> + uint64_t old; >>> + >>> + /* 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)); >>> + shrink = size < hb->size; >>> + >>> + /* bit sizes are identical; nothing to do. */ >>> + if (size == hb->size) { >>> + return; >>> + } >>> + >>> + /* If we're losing bits, let's clear those bits before we >>> invalidate all of >>> + * our invariants. This helps keep the bitcount consistent, and >>> will prevent >>> + * us from carrying around garbage bits beyond the end of the map. >>> + * >>> + * Because clearing bits past the end of map might reset bits we >>> care about >>> + * within the array, record the current value of the last bit >>> we're keeping. >>> + */ >>> + if (shrink) { >>> + bool set = hbitmap_get(hb, num_elements - 1); >>> + uint64_t fix_count = (hb->size << hb->granularity) - >>> num_elements; >>> + >>> + assert(fix_count); >>> + hbitmap_reset(hb, num_elements, fix_count); >>> + if (set) { >>> + hbitmap_set(hb, num_elements - 1, 1); >>> + } >>> + } >>> + >>> + hb->size = size; >>> + for (i = HBITMAP_LEVELS; i-- > 0; ) { >>> + size = MAX(BITS_TO_LONGS(size), 1); >> >> Shouldn't this be "size = MAX(BITS_TO_LONGS(size) >> BITS_PER_LEVEL, >> 1);"? >> > > I don't think so; > > BITS_TO_LONGS(X) replaces the original construct: > (size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL > which takes a size, adds <31|63> and then divides by <32|64>. > > BITS_TO_LONGS performs DIV_ROUND_UP(nr, <32|64>), which will do > effectively the same thing. (Actually, a little less efficiently, but > I found this macro was nicer to read.) You're right (it's probably the same, efficiency-wise, as long as you have a compiler which optimizes x / 64 to x >> 6). I don't think it's nicer to read, though, because apparently it confused me. :-) >>> + if (hb->sizes[i] == size) { >>> + break; >>> + } >>> + old = hb->sizes[i]; >>> + hb->sizes[i] = size; >>> + hb->levels[i] = g_realloc(hb->levels[i], size * >>> sizeof(unsigned long)); >> >> Any specific reason you got rid of the g_realloc_n()? >> > > Not available in glib 2.12 (or 2.22.) Urgh... The RHEL 5 curse? :-/ Anyway: Reviewed-by: Max Reitz > >> Apart from these, the changes to v2 look good. >> >> Max >> >>> + if (!shrink) { >>> + memset(&hb->levels[i][old], 0x00, >>> + (size - old) * sizeof(*hb->levels[i])); >>> + } >>> + } >>> +} >>> + >>> + >>> /** >>> * Given HBitmaps A and B, let A := A (BITOR) B. >>> * Bitmap B will not be modified. >> >>