Re: [Qemu-devel] [PATCH v14 18/19] block: Resize bitmaps on bdrv_truncate

[John Snow <jsnow@redhat.com>]

On 02/23/2015 12:03 PM, Max Reitz wrote:
> On 2015-02-20 at 18:07, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   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())
>

A copy-paste loop from hbitmap_alloc is why this pattern keeps emerging. 
It's not necessarily how I'd do it, either.

>> +        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.
>

Good call; I had actually thought that g_realloc did initialize the 
memory, but looking at the docs again: nope.

>> +    }
>> +    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
>

Ah, indeed. If you hit a perfect even multiple where there are no bits 
at all to clean, you wind up with a one-past-the-end, even though the 
operation should be a no-op.

>> +        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.
>