include/qemu/hbitmap.h | 2 +- util/hbitmap.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
The last return statement should return true, as we already evaluated that
start == next_dirty
Also, fix hbitmap_status() description in header
Cc: qemu-stable@nongnu.org
Fixes: a6426475a75 ("block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()")
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
include/qemu/hbitmap.h | 2 +-
util/hbitmap.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
index af4e4ab746..8136e33674 100644
--- a/include/qemu/hbitmap.h
+++ b/include/qemu/hbitmap.h
@@ -330,7 +330,7 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
int64_t *dirty_start, int64_t *dirty_count);
/*
- * bdrv_dirty_bitmap_status:
+ * hbitmap_status:
* @hb: The HBitmap to operate on
* @start: The bit to start from
* @count: Number of bits to proceed
diff --git a/util/hbitmap.c b/util/hbitmap.c
index 297db35fb1..6d6e1b595d 100644
--- a/util/hbitmap.c
+++ b/util/hbitmap.c
@@ -331,7 +331,7 @@ bool hbitmap_status(const HBitmap *hb, int64_t start, int64_t count,
assert(next_zero > start);
*pnum = next_zero - start;
- return false;
+ return true;
}
bool hbitmap_empty(const HBitmap *hb)
--
2.36.1Am 02.02.2023 um 19:15 hat Andrey Zhadchenko via geschrieben:
> The last return statement should return true, as we already evaluated that
> start == next_dirty
>
> Also, fix hbitmap_status() description in header
>
> Cc: qemu-stable@nongnu.org
> Fixes: a6426475a75 ("block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()")
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
Thanks, applied to the block branch.
Kevin
On 02.02.23 21:15, Andrey Zhadchenko via wrote:
> The last return statement should return true, as we already evaluated that
> start == next_dirty
>
> Also, fix hbitmap_status() description in header
>
> Cc: qemu-stable@nongnu.org
> Fixes: a6426475a75 ("block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()")
Ohh :/
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> include/qemu/hbitmap.h | 2 +-
> util/hbitmap.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> index af4e4ab746..8136e33674 100644
> --- a/include/qemu/hbitmap.h
> +++ b/include/qemu/hbitmap.h
> @@ -330,7 +330,7 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end,
> int64_t *dirty_start, int64_t *dirty_count);
>
> /*
> - * bdrv_dirty_bitmap_status:
> + * hbitmap_status:
> * @hb: The HBitmap to operate on
> * @start: The bit to start from
> * @count: Number of bits to proceed
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 297db35fb1..6d6e1b595d 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -331,7 +331,7 @@ bool hbitmap_status(const HBitmap *hb, int64_t start, int64_t count,
>
> assert(next_zero > start);
> *pnum = next_zero - start;
> - return false;
> + return true;
> }
>
> bool hbitmap_empty(const HBitmap *hb)
--
Best regards,
Vladimir
On Thu, Feb 02, 2023 at 09:15:23PM +0300, Andrey Zhadchenko via wrote:
> The last return statement should return true, as we already evaluated that
> start == next_dirty
>
> Also, fix hbitmap_status() description in header
>
> Cc: qemu-stable@nongnu.org
> Fixes: a6426475a75 ("block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()")
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
> include/qemu/hbitmap.h | 2 +-
> util/hbitmap.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Eww, nasty bug; looks like copy-before-write is the only curernt
client of this interface.
Reviewed-by: Eric Blake <eblake@redhat.com>
Is there any way to enhance an iotest to add coverage for this code?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 2/2/23 22:39, Eric Blake wrote:
> On Thu, Feb 02, 2023 at 09:15:23PM +0300, Andrey Zhadchenko via wrote:
>> The last return statement should return true, as we already evaluated that
>> start == next_dirty
>>
>> Also, fix hbitmap_status() description in header
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: a6426475a75 ("block/dirty-bitmap: introduce bdrv_dirty_bitmap_status()")
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>> ---
>> include/qemu/hbitmap.h | 2 +-
>> util/hbitmap.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> Eww, nasty bug; looks like copy-before-write is the only curernt
> client of this interface.
Also this only happens when copy-before-write is used in conjunction
with snapshot-access (7.0+), so probably not many people is affected.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Is there any way to enhance an iotest to add coverage for this code?
>
Looks like yes. copy-before-write uses bcs bitmaps, which is 64k.
So changing existing test
diff --git a/tests/qemu-iotests/tests/image-fleecing
b/tests/qemu-iotests/tests/image-fleecing
index f6e449d071..7eda9a2c6b 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -34,23 +34,23 @@ iotests.script_initialize(
unsupported_imgopts=['compat']
)
-patterns = [('0x5d', '0', '64k'),
- ('0xd5', '1M', '64k'),
- ('0xdc', '32M', '64k'),
- ('0xcd', '0x3ff0000', '64k')] # 64M - 64K
+patterns = [('0x5d', '0', '128k'),
+ ('0xd5', '1M', '128k'),
+ ('0xdc', '32M', '128k'),
+ ('0xcd', '0x3fe0000', '128k')] # 64M - 128K
-overwrite = [('0xab', '0', '64k'), # Full overwrite
- ('0xad', '0x00f8000', '64k'), # Partial-left (1M-32K)
- ('0x1d', '0x2008000', '64k'), # Partial-right (32M+32K)
- ('0xea', '0x3fe0000', '64k')] # Adjacent-left (64M - 128K)
+overwrite = [('0xab', '0', '128k'), # Full overwrite
+ ('0xad', '0x00f0000', '128k'), # Partial-left (1M-64K)
+ ('0x1d', '0x2010000', '128k'), # Partial-right (32M+64K)
+ ('0xea', '0x3fc0000', '128k')] # Adjacent-left (64M - 256K)
-zeroes = [('0', '0x00f8000', '32k'), # Left-end of partial-left (1M-32K)
- ('0', '0x2010000', '32k'), # Right-end of partial-right (32M+64K)
- ('0', '0x3fe0000', '64k')] # overwrite[3]
+zeroes = [('0', '0x00f0000', '64k'), # Left-end of partial-left (1M-64K)
+ ('0', '0x2020000', '64k'), # Right-end of partial-right
(32M+128K)
+ ('0', '0x3fc0000', '128k')] # overwrite[3]
-remainder = [('0xd5', '0x108000', '32k'), # Right-end of partial-left [1]
- ('0xdc', '32M', '32k'), # Left-end of partial-right [2]
- ('0xcd', '0x3ff0000', '64k')] # patterns[3]
+remainder = [('0xd5', '0x110000', '64k'), # Right-end of partial-left [1]
+ ('0xdc', '32M', '64k'), # Left-end of partial-right [2]
+ ('0xcd', '0x3fe0000', '128k')] # patterns[3]
def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
fleece_img_path, nbd_sock_path=None,
From 64k chunks to 128k chunks (so at the read moment we have N bit
dirty and N+1 clean) produces the following for one of NBD test cases:
--- Setting up NBD Export ---
{"return": {}}
{"return": {}}
--- Sanity Check ---
read -P0x5d 0 128k
read -P0xd5 1M 128k
read -P0xdc 32M 128k
read -P0xcd 0x3fe0000 128k
read -P0 0x00f0000 64k
read failed: Invalid argument
read -P0 0x2020000 64k
read failed: Invalid argument
read -P0 0x3fc0000 128k
read failed: Invalid argument
I am not good at qemi-io tool, I guess that EINVAL in that case means
qemu-io found out that image data does not follow requested pattern?
Also this does not trigger for non-nbd tests since backup job is
probably copying one cluster at a time.
On 2/3/23 13:55, Andrey Zhadchenko wrote:
>
> On 2/2/23 22:39, Eric Blake wrote:
>> On Thu, Feb 02, 2023 at 09:15:23PM +0300, Andrey Zhadchenko via wrote:
>>> The last return statement should return true, as we already evaluated
>>> that
>>> start == next_dirty
>>>
>>> Also, fix hbitmap_status() description in header
>>>
>>> Cc: qemu-stable@nongnu.org
>>> Fixes: a6426475a75 ("block/dirty-bitmap: introduce
>>> bdrv_dirty_bitmap_status()")
>>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>>> ---
>>> include/qemu/hbitmap.h | 2 +-
>>> util/hbitmap.c | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Eww, nasty bug; looks like copy-before-write is the only curernt
>> client of this interface.
> Also this only happens when copy-before-write is used in conjunction
> with snapshot-access (7.0+), so probably not many people is affected.
>
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Is there any way to enhance an iotest to add coverage for this code?
>>
> Looks like yes. copy-before-write uses bcs bitmaps, which is 64k.
> So changing existing test
>
> diff --git a/tests/qemu-iotests/tests/image-fleecing
> b/tests/qemu-iotests/tests/image-fleecing
> index f6e449d071..7eda9a2c6b 100755
> --- a/tests/qemu-iotests/tests/image-fleecing
> +++ b/tests/qemu-iotests/tests/image-fleecing
> @@ -34,23 +34,23 @@ iotests.script_initialize(
> unsupported_imgopts=['compat']
> )
>
> -patterns = [('0x5d', '0', '64k'),
> - ('0xd5', '1M', '64k'),
> - ('0xdc', '32M', '64k'),
> - ('0xcd', '0x3ff0000', '64k')] # 64M - 64K
> +patterns = [('0x5d', '0', '128k'),
> + ('0xd5', '1M', '128k'),
> + ('0xdc', '32M', '128k'),
> + ('0xcd', '0x3fe0000', '128k')] # 64M - 128K
>
> -overwrite = [('0xab', '0', '64k'), # Full overwrite
> - ('0xad', '0x00f8000', '64k'), # Partial-left (1M-32K)
> - ('0x1d', '0x2008000', '64k'), # Partial-right (32M+32K)
> - ('0xea', '0x3fe0000', '64k')] # Adjacent-left (64M - 128K)
> +overwrite = [('0xab', '0', '128k'), # Full overwrite
> + ('0xad', '0x00f0000', '128k'), # Partial-left (1M-64K)
> + ('0x1d', '0x2010000', '128k'), # Partial-right (32M+64K)
> + ('0xea', '0x3fc0000', '128k')] # Adjacent-left (64M - 256K)
>
> -zeroes = [('0', '0x00f8000', '32k'), # Left-end of partial-left (1M-32K)
> - ('0', '0x2010000', '32k'), # Right-end of partial-right
> (32M+64K)
> - ('0', '0x3fe0000', '64k')] # overwrite[3]
> +zeroes = [('0', '0x00f0000', '64k'), # Left-end of partial-left (1M-64K)
> + ('0', '0x2020000', '64k'), # Right-end of partial-right
> (32M+128K)
> + ('0', '0x3fc0000', '128k')] # overwrite[3]
>
> -remainder = [('0xd5', '0x108000', '32k'), # Right-end of partial-left [1]
> - ('0xdc', '32M', '32k'), # Left-end of partial-right [2]
> - ('0xcd', '0x3ff0000', '64k')] # patterns[3]
> +remainder = [('0xd5', '0x110000', '64k'), # Right-end of partial-left [1]
> + ('0xdc', '32M', '64k'), # Left-end of partial-right [2]
> + ('0xcd', '0x3fe0000', '128k')] # patterns[3]
>
> def do_test(vm, use_cbw, use_snapshot_access_filter, base_img_path,
> fleece_img_path, nbd_sock_path=None,
>
>
> From 64k chunks to 128k chunks (so at the read moment we have N bit
> dirty and N+1 clean) produces the following for one of NBD test cases:
>
> --- Setting up NBD Export ---
>
> {"return": {}}
> {"return": {}}
>
> --- Sanity Check ---
>
> read -P0x5d 0 128k
> read -P0xd5 1M 128k
> read -P0xdc 32M 128k
> read -P0xcd 0x3fe0000 128k
> read -P0 0x00f0000 64k
> read failed: Invalid argument
>
> read -P0 0x2020000 64k
> read failed: Invalid argument
>
> read -P0 0x3fc0000 128k
> read failed: Invalid argument
>
>
> I am not good at qemi-io tool, I guess that EINVAL in that case means
> qemu-io found out that image data does not follow requested pattern?
>
> Also this does not trigger for non-nbd tests since backup job is
> probably copying one cluster at a time.
Please disregard this.
This EINVALs are somewhat anticipated as they are also present in
expected example image-fleecing.out (and applying my patch doesn't get
rid of them)
I should think more about possible test cases
© 2016 - 2023 Red Hat, Inc.