From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58097) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXv45-00087M-Uf for qemu-devel@nongnu.org; Tue, 17 Mar 2015 13:14:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXv44-0000df-I2 for qemu-devel@nongnu.org; Tue, 17 Mar 2015 13:14:01 -0400 Message-ID: <550860D1.4050007@redhat.com> Date: Tue, 17 Mar 2015 13:13:53 -0400 From: John Snow 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> In-Reply-To: <5508311D.1070301@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: Max Reitz , 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 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.) >> + 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.) > 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. > >