[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to b
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration |
Date: |
Tue, 19 Sep 2017 14:44:24 +0200 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
Am 18.09.2017 um 20:58 hat Eric Blake geschrieben:
> Now that we have adjusted the majority of the calls this function
> makes to be byte-based, it is easier to read the code if it makes
> passes over the image using bytes rather than sectors.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: John Snow <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>
> ---
> v7: rebase to earlier change, make rounding of offset obvious (no semantic
> change, so R-b kept) [Kevin]
> v5-v6: no change
> v4: new patch
> ---
> block/qcow2-bitmap.c | 26 +++++++++++---------------
> 1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 692ce0de88..302fffd6e1 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1072,10 +1072,9 @@ static uint64_t *store_bitmap_data(BlockDriverState
> *bs,
> {
> int ret;
> BDRVQcow2State *s = bs->opaque;
> - int64_t sector;
> - uint64_t limit, sbc;
> + int64_t offset;
> + uint64_t limit;
> uint64_t bm_size = bdrv_dirty_bitmap_size(bitmap);
> - uint64_t bm_sectors = DIV_ROUND_UP(bm_size, BDRV_SECTOR_SIZE);
> const char *bm_name = bdrv_dirty_bitmap_name(bitmap);
> uint8_t *buf = NULL;
> BdrvDirtyBitmapIter *dbi;
> @@ -1100,18 +1099,17 @@ static uint64_t *store_bitmap_data(BlockDriverState
> *bs,
> dbi = bdrv_dirty_iter_new(bitmap);
> buf = g_malloc(s->cluster_size);
> limit = bytes_covered_by_bitmap_cluster(s, bitmap);
> - 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;
> uint64_t end, write_size;
> int64_t off;
>
> - sector = cluster * sbc;
> - end = MIN(bm_sectors, sector + sbc);
> - write_size = bdrv_dirty_bitmap_serialization_size(bitmap,
> - sector * BDRV_SECTOR_SIZE, (end - sector) * BDRV_SECTOR_SIZE);
> + offset = QEMU_ALIGN_DOWN(offset, limit);
With the question that I asked in v6, apart from changing the spelling
to be more explicit, I actually hoped that you would explain why
aligning down is the right thing to do.
It looks plausible to me that we can do this in correct code because we
don't support granularities < 512 bytes (a qemu restriction that is
written down as a note in the qcow2 spec).
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().)
> + end = MIN(bm_size, offset + limit);
> + write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
> + end - offset);
> assert(write_size <= s->cluster_size);
>
> off = qcow2_alloc_clusters(bs, s->cluster_size);
Kevin
- [Qemu-devel] [PATCH v8 09/20] qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based, (continued)
- [Qemu-devel] [PATCH v8 09/20] qcow2: Switch sectors_covered_by_bitmap_cluster() to byte-based, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 10/20] dirty-bitmap: Set iterator start by offset, not sector, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 12/20] dirty-bitmap: Change bdrv_get_dirty_count() to report bytes, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 13/20] dirty-bitmap: Change bdrv_get_dirty_locked() to take bytes, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 15/20] mirror: Switch mirror_dirty_init() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 17/20] qcow2: Switch load_bitmap_data() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 16/20] qcow2: Switch qcow2_measure() to byte-based iteration, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 14/20] dirty-bitmap: Change bdrv_[re]set_dirty_bitmap() to use bytes, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration, Eric Blake, 2017/09/18
- Re: [Qemu-devel] [PATCH v8 18/20] qcow2: Switch store_bitmap_data() to byte-based iteration,
Kevin Wolf <=
- [Qemu-devel] [PATCH v8 19/20] dirty-bitmap: Switch bdrv_set_dirty() to bytes, Eric Blake, 2017/09/18
- [Qemu-devel] [PATCH v8 20/20] dirty-bitmap: Convert internal hbitmap size/granularity, Eric Blake, 2017/09/18
- Re: [Qemu-devel] [PATCH v8 00/20] make dirty-bitmap byte-based, Kevin Wolf, 2017/09/19
- Re: [Qemu-devel] [PATCH v8 00/20] make dirty-bitmap byte-based, Fam Zheng, 2017/09/19