Re: [Qemu-block] [PATCH v6 16/18] qcow2: Switch store_bitmap_data() to byte-based iteration
Am 08.09.2017 um 16:09 hat Eric Blake geschrieben:
> On 09/08/2017 08:27 AM, Kevin Wolf wrote:
> > Am 30.08.2017 um 23:05 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
> >> Reviewed-by: John Snow
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy
> >>
>
> >>
> >> -while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) !=
> >> -1) {
> >> -uint64_t cluster = sector / sbc;
> >> +while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
> >
> > Don't you have to multiply both sides of the equation? This would be
> > offset != -512, which points out that the previous patch to convert
> > bdrv_dirty_iter_next() to byte-based gave it a really awkward interface.
>
> I think what I really need to do is change '!= -1' to '< 0', as that's
> much easier to reason about when scaling is present.
Hm, I think I would prefer a special case for -1 in bdrv_dirty_iter_next()
so that it returns -1 after the last entry. Even if you check for < 0,
-512 is still an odd return value to signal the end.
Though I think after the final patch, we're back to -1 anyway, so it's
not that important.
Kevin
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v6 16/18] qcow2: Switch store_bitmap_data() to byte-based iteration
On 09/08/2017 08:27 AM, Kevin Wolf wrote:
> Am 30.08.2017 um 23:05 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
>> Reviewed-by: John Snow
>> Reviewed-by: Vladimir Sementsov-Ogievskiy
>>
>>
>> -while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) != -1) {
>> -uint64_t cluster = sector / sbc;
>> +while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
>
> Don't you have to multiply both sides of the equation? This would be
> offset != -512, which points out that the previous patch to convert
> bdrv_dirty_iter_next() to byte-based gave it a really awkward interface.
I think what I really need to do is change '!= -1' to '< 0', as that's
much easier to reason about when scaling is present.
>
>> +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 = cluster * limit;
>
> You just had cluster = offset / limit, so in other words, align down
> offset? If so, this is how it should be written.
Thanks for the close reviews; looks like I have enough things to do a
respin.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v6 16/18] qcow2: Switch store_bitmap_data() to byte-based iteration
Am 30.08.2017 um 23:05 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
> Reviewed-by: John Snow
> Reviewed-by: Vladimir Sementsov-Ogievskiy
>
> ---
> v5: 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 b807298484..63d845e35f 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) != -1) {
> -uint64_t cluster = sector / sbc;
> +while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
Don't you have to multiply both sides of the equation? This would be
offset != -512, which points out that the previous patch to convert
bdrv_dirty_iter_next() to byte-based gave it a really awkward interface.
> +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 = cluster * limit;
You just had cluster = offset / limit, so in other words, align down
offset? If so, this is how it should be written.
Kevin
[Qemu-block] [PATCH v6 16/18] qcow2: Switch store_bitmap_data() to byte-based iteration
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
Reviewed-by: John Snow
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
v5: 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 b807298484..63d845e35f 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) != -1) {
-uint64_t cluster = sector / sbc;
+while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
+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 = cluster * limit;
+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);
@@ -1123,9 +1121,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
}
tb[cluster] = off;
-bdrv_dirty_bitmap_serialize_part(bitmap, buf,
- sector * BDRV_SECTOR_SIZE,
- (end - sector) * BDRV_SECTOR_SIZE);
+bdrv_dirty_bitmap_serialize_part(bitmap, buf, offset, end - offset);
if (write_size < s->cluster_size) {
memset(buf + write_size, 0, s->cluster_size - write_size);
}
@@ -1143,11 +1139,11 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
goto fail;
}
-if (end >= bm_sectors) {
+if (end >= bm_size) {
break;
}
-bdrv_set_dirty_iter(dbi, end * BDRV_SECTOR_SIZE);
+bdrv_set_dirty_iter(dbi, end);
}
*bitmap_table_size = tb_size;
--
2.13.5
