[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate |
Date: |
Wed, 11 Mar 2015 16:18:02 +0000 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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?
> + }
> +}
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().
> /**
> + * 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)
> +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.
> +
> + 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.
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.
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.
pgp30Gy_UJ8gb.pgp
Description: PGP signature
- [Qemu-block] [PATCH v2 05/17] block: Introduce bdrv_dirty_bitmap_granularity(), (continued)
Re: [Qemu-block] [Qemu-devel] [PATCH v2 14/17] block: Resize bitmaps on bdrv_truncate,
Stefan Hajnoczi <=
[Qemu-block] [PATCH v2 09/17] qmp: Add support of "dirty-bitmap" sync mode for drive-backup, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 10/17] qmp: add block-dirty-bitmap-clear, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 15/17] iotests: add invalid input incremental backup tests, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 11/17] qmp: Add dirty bitmap status fields in query-block, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 17/17] iotests: add incremental backup failure recovery test, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 12/17] block: add BdrvDirtyBitmap documentation, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 13/17] block: Ensure consistent bitmap function prototypes, John Snow, 2015/03/02
[Qemu-block] [PATCH v2 16/17] iotests: add simple incremental backup case, John Snow, 2015/03/02