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

[Andrey Zhadchenko]

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.