Re: [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

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.