[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to b
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration |
Date: |
Wed, 20 Sep 2017 08:22:44 +0200 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
Am 19.09.2017 um 21:42 hat Eric Blake geschrieben:
> However...
>
> >> - sbc = limit >> BDRV_SECTOR_BITS;
> >> assert(DIV_ROUND_UP(bm_size, limit) == tb_size);
> >>
> >> - while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) >= 0)
> >> {
> >> - uint64_t cluster = sector / sbc;
> >> + while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
> >> + uint64_t cluster = offset / limit;
>
> bdrv_dirty_iter_next() returns the next dirty bit (which is not
> necessarily the first bit in the cluster). For the purposes of
> serialization, we want to serialize the entire cluster in one go, even
> though we will be serializing 0 bits up until the first dirty bit. So
> offset at this point may be unaligned,
Ok, this is the part that I was missing. It makes a lot more sense now.
Also, I think 'cluster' meaning bitmap clusters and not qcow2 clusters
here confused me a bit.
> > The part that I'm missing yet is why we need to do it. The bitmap
> > granularity is also the granularity of bdrv_dirty_iter_next(), so isn't
> > offset already aligned and we could even assert that instead of aligning
> > down? (As long we enforce our restriction, which we seem to do in
> > bitmap_list_load().)
>
> Sadly, a quick:
> [...]
> does NOT fail iotests 165, which appears to be the only test that
> actually hammers on qcow2 bitmaps (changing it to an 'assert(false)'
> only shows an effect on 165) - which means our test is NOT exercising
> all possible alignments. And it's python-based, with lame output, which
> makes debugging it painful. But never fear, for v9 I will improve the
> test to actually affect the bitmap at a point that would fail with my
> temporary assertion in place, and thus proving that we DO need to align
> down. Note that test 165 is testing only a 1G image, but I just showed
> that 64k clusters with 64k granularity covers up to 32G of image space
> in one cluster of the bitmap, so the test is only covering one cluster
> of serialization in the first place, and as written, the test is
> dirtying byte 0, which explains why it happens to get an offset aligned
> to limit, even though that is not a valid assertion.
More tests are always welcome and a good argument for getting a series
merged. :-)
Kevin
signature.asc
Description: PGP signature
- [Qemu-block] [PATCH v8 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset, (continued)
- [Qemu-block] [PATCH v8 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 12/20] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 13/20] dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 15/20] mirror: Switch mirror_dirty_init() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 16/20] qcow2: Switch qcow2_measure() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 14/20] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 17/20] qcow2: Switch load_bitmap_data() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 19/20] dirty-bitmap: Switch bdrv_set_dirty() to bytes, Eric Blake, 2017/09/18
- [Qemu-block] [PATCH v8 20/20] dirty-bitmap: Convert internal hbitmap size/granularity, Eric Blake, 2017/09/18
- Re: [Qemu-block] [PATCH v8 00/20] make dirty-bitmap byte-based, Kevin Wolf, 2017/09/19
- Re: [Qemu-block] [Qemu-devel] [PATCH v8 00/20] make dirty-bitmap byte-based, Fam Zheng, 2017/09/19