Re: [Qemu-devel] [PATCH v7 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[John Snow]

On 09/14/2017 07:58 AM, Eric Blake wrote:
> On 09/13/2017 07:15 PM, John Snow wrote:
>>
>>
>> On 09/12/2017 04:31 PM, Eric Blake wrote:
>>> Thanks to recent cleanups, most callers were scaling a return value
>>> of sectors into bytes (the exception, in qcow2-bitmap, will be
>>> converted to byte-based iteration later).  Update the interface to
>>> do the scaling internally instead.
>>>
>>> In qcow2-bitmap, the code was specifically checking for an error
>>> to be -1; it is more robust to treat all negative values as an
>>> error, but at the same time it is also easy enough to ensure we
>>> return -1 (and not -512) on error.
>>>
>>> Signed-off-by: Eric Blake 
>>>
>>
>> This patch now smells like a bugfix and a separate incremental feature
>> enhancement.
> 
> There is no bug without this patch; more of a fix to avoid a latent

Ah, you're right, I see. Please take the RB.

> regression from happening in further changes.  In v6, I (accidentally)
> had bdrv_dirty_iter_next() temporarily returning -512 instead of -1 on
> failure; changing the qcow2 code to treat all negatives instead of
> precisely -1 as error is enough to avoid that regression, but so also is
> fixing bdrv_dirty_iter_next() to always return -1 on failure.  This
> patch does both, rather than either fix in isolation, but that means we
> don't need a backport.
> 
>>
>> Do we need to backport the error-checking to a possible 2.10.1?
>>
>> If no:
>>
>> Reviewed-by: John Snow 
>>
> 

Re: [Qemu-devel] [PATCH v7 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[Eric Blake]

On 09/13/2017 07:15 PM, John Snow wrote:
> 
> 
> On 09/12/2017 04:31 PM, Eric Blake wrote:
>> Thanks to recent cleanups, most callers were scaling a return value
>> of sectors into bytes (the exception, in qcow2-bitmap, will be
>> converted to byte-based iteration later).  Update the interface to
>> do the scaling internally instead.
>>
>> In qcow2-bitmap, the code was specifically checking for an error
>> to be -1; it is more robust to treat all negative values as an
>> error, but at the same time it is also easy enough to ensure we
>> return -1 (and not -512) on error.
>>
>> Signed-off-by: Eric Blake 
>>
> 
> This patch now smells like a bugfix and a separate incremental feature
> enhancement.

There is no bug without this patch; more of a fix to avoid a latent
regression from happening in further changes.  In v6, I (accidentally)
had bdrv_dirty_iter_next() temporarily returning -512 instead of -1 on
failure; changing the qcow2 code to treat all negatives instead of
precisely -1 as error is enough to avoid that regression, but so also is
fixing bdrv_dirty_iter_next() to always return -1 on failure.  This
patch does both, rather than either fix in isolation, but that means we
don't need a backport.

> 
> Do we need to backport the error-checking to a possible 2.10.1?
> 
> If no:
> 
> Reviewed-by: John Snow 
> 

-- 
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-devel] [PATCH v7 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[John Snow]

On 09/12/2017 04:31 PM, Eric Blake wrote:
> Thanks to recent cleanups, most callers were scaling a return value
> of sectors into bytes (the exception, in qcow2-bitmap, will be
> converted to byte-based iteration later).  Update the interface to
> do the scaling internally instead.
> 
> In qcow2-bitmap, the code was specifically checking for an error
> to be -1; it is more robust to treat all negative values as an
> error, but at the same time it is also easy enough to ensure we
> return -1 (and not -512) on error.
> 
> Signed-off-by: Eric Blake 
> 

This patch now smells like a bugfix and a separate incremental feature
enhancement.

Do we need to backport the error-checking to a possible 2.10.1?

If no:

Reviewed-by: John Snow 

[Qemu-devel] [PATCH v7 11/20] dirty-bitmap: Change bdrv_dirty_iter_next() to report byte offset

[Eric Blake]

Thanks to recent cleanups, most callers were scaling a return value
of sectors into bytes (the exception, in qcow2-bitmap, will be
converted to byte-based iteration later).  Update the interface to
do the scaling internally instead.

In qcow2-bitmap, the code was specifically checking for an error
to be -1; it is more robust to treat all negative values as an
error, but at the same time it is also easy enough to ensure we
return -1 (and not -512) on error.

Signed-off-by: Eric Blake 

---
v7: return -1, not -512; and fix qcow2-bitmap to check all negatives [Kevin]
v5-v6: no change
v4: rebase to persistent bitmap
v3: no change
v2: no change
---
 block/backup.c   | 2 +-
 block/dirty-bitmap.c | 3 ++-
 block/mirror.c   | 8 
 block/qcow2-bitmap.c | 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index ac9c018717..06ddbfd03d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -375,7 +375,7 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 dbi = bdrv_dirty_iter_new(job->sync_bitmap);

 /* Find the next dirty sector(s) */
-while ((offset = bdrv_dirty_iter_next(dbi) * BDRV_SECTOR_SIZE) >= 0) {
+while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
 cluster = offset / job->cluster_size;

 /* Fake progress updates for any clusters we skipped */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index d50c46621d..49229fd501 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -508,7 +508,8 @@ void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)

 int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
 {
-return hbitmap_iter_next(>hbi);
+int64_t ret = hbitmap_iter_next(>hbi);
+return ret < 0 ? -1 : ret * BDRV_SECTOR_SIZE;
 }

 /* Called within bdrv_dirty_bitmap_lock..unlock */
diff --git a/block/mirror.c b/block/mirror.c
index 0b063b3c20..77bf5aa3a4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -336,10 +336,10 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 int max_io_bytes = MAX(s->buf_size / MAX_IN_FLIGHT, MAX_IO_BYTES);

 bdrv_dirty_bitmap_lock(s->dirty_bitmap);
-offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+offset = bdrv_dirty_iter_next(s->dbi);
 if (offset < 0) {
 bdrv_set_dirty_iter(s->dbi, 0);
-offset = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+offset = bdrv_dirty_iter_next(s->dbi);
 trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap) *
   BDRV_SECTOR_SIZE);
 assert(offset >= 0);
@@ -370,11 +370,11 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 break;
 }

-next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+next_dirty = bdrv_dirty_iter_next(s->dbi);
 if (next_dirty > next_offset || next_dirty < 0) {
 /* The bitmap iterator's cache is stale, refresh it */
 bdrv_set_dirty_iter(s->dbi, next_offset);
-next_dirty = bdrv_dirty_iter_next(s->dbi) * BDRV_SECTOR_SIZE;
+next_dirty = bdrv_dirty_iter_next(s->dbi);
 }
 assert(next_dirty == next_offset);
 nb_chunks++;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 44329fc74f..b09010b1d3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1109,7 +1109,7 @@ static uint64_t *store_bitmap_data(BlockDriverState *bs,
 sbc = limit >> BDRV_SECTOR_BITS;
 assert(DIV_ROUND_UP(bm_size, limit) == tb_size);

-while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
+while ((sector = bdrv_dirty_iter_next(dbi) >> BDRV_SECTOR_BITS) >= 0) {
 uint64_t cluster = sector / sbc;
 uint64_t end, write_size;
 int64_t off;
-- 
2.13.5