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
