Re: [Qemu-devel] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]