From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVk3h-0004ds-OP for qemu-devel@nongnu.org; Wed, 11 Mar 2015 13:04:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVk3e-00068k-5r for qemu-devel@nongnu.org; Wed, 11 Mar 2015 13:04:37 -0400 Message-ID: <5500759D.7000203@redhat.com> Date: Wed, 11 Mar 2015 13:04:29 -0400 From: John Snow MIME-Version: 1.0 References: <1425338403-16138-1-git-send-email-jsnow@redhat.com> <1425338403-16138-15-git-send-email-jsnow@redhat.com> <20150311161802.GK10493@stefanha-thinkpad.redhat.com> In-Reply-To: <20150311161802.GK10493@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, armbru@redhat.com, vsementsov@parallels.com, stefanha@redhat.com, mreitz@redhat.com On 03/11/2015 12:18 PM, Stefan Hajnoczi wrote: > On Mon, Mar 02, 2015 at 06:20:00PM -0500, John Snow wrote: >> +static void dirty_bitmap_truncate(BdrvDirtyBitmap *bitmap, uint64_t size) >> +{ >> + /* Should only be frozen during a block backup job, which should have >> + * blocked any resize actions. */ >> + assert(!bdrv_dirty_bitmap_frozen(bitmap)); >> + 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); > > If you inline this function here then the discussion about assert() vs > skipping frozen bitmaps goes away. Why is dirty_bitmap_truncate() a > function? > Symmetry with other bitmap functions. >> + } >> +} > > Why is bdrv_dirty_bitmap_truncate() a public API? I expected this code > to be inline or called as a static function by bdrv_truncate(). > OK, fixing that. >> /** >> + * 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); > > Please include a tests/test-hbitmap.c test case. > > Interesting cases: > 1. New size equals old size (odd but possible) > 2. Growing less than sizeof(unsigned long) > 3. Growing more than sizeof(unsigned long) > 4. Shrinking less than sizeof(unsigned long) > 5. Shrinking more than sizeof(unsigned long) > ;_; OK, you're right... >> +void hbitmap_truncate(HBitmap *hb, uint64_t size) >> +{ >> + bool truncate; >> + 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)); >> + truncate = size < hb->size; > > Here "truncate" means "shrink". > > "shrink" is a clearer name since the function name is already "truncate" > but that concept includes both increasing or decreasing size. > Yes, fair enough. Was relying on what I consider the colloquial definition of truncate. >> + >> + if (size == hb->size) { >> + /* A hard day's work */ >> + return; >> + } >> + >> + hb->size = size; >> + for (i = HBITMAP_LEVELS; i-- > 0; ) { >> + size = MAX((size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1); >> + if (hb->sizes[i] == size) { >> + continue; >> + } >> + old = hb->sizes[i]; >> + hb->sizes[i] = size; > > I was wondering what sizes[] is used for. Not a very useful struct > field since it's only needed by this rarely called function. > In future patches, we tend to recalculate the size of each array a lot. I decided I wanted to cache it so we could stop duplicating that code over and over. It comes up in migration and persistence a lot. It's easier to just add it now instead of allow the duplication to sneak in and then patch it out everywhere. > It would be clearer to calculate 'old' alongside 'size' each loop > iteration. The size[] field can be dropped, 'old' becomes 'old_size', > and 'size' becomes 'new_size': > > old_size = hb->size; > for (i = HBITMAP_LEVELS; i-- > 0; ) { > old_size = MAX((old_size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1); > new_size = MAX((new_size + BITS_PER_LONG - 1) >> BITS_PER_LEVEL, 1); > >> + hb->levels[i] = g_realloc_n(hb->levels[i], size, sizeof(unsigned long)); >> + if (!truncate) { >> + memset(&hb->levels[i][old], 0x00, >> + (size - old) * sizeof(*hb->levels[i])); >> + } >> + } >> + 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 element we care about. */ >> + uint64_t fix_count = (hb->size << hb->granularity) - num_elements; >> + if (fix_count) { >> + bool set = hbitmap_get(hb, num_elements - 1); >> + hbitmap_reset(hb, num_elements, fix_count); >> + if (set) { >> + hbitmap_set(hb, num_elements - 1, 1); >> + } >> + } > > Calling hbitmap_reset() with an out-of-bounds index seems hacky to me. > It's the simplest way to re-use the existing code to recursively clear out any bits that are set that shouldn't be. > Why doesn't the for loop's if (!truncate) have an else statement to mask > no longer visible bits? Maybe I'm missing why that's hard to do. > I just didn't see a reason to replicate the logic of what hbitmap_reset already does, so I didn't bother to try.