* [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case
@ 2023-02-02 18:15 Andrey Zhadchenko via
2023-02-02 19:39 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Andrey Zhadchenko via @ 2023-02-02 18:15 UTC (permalink / raw)
To: vsementsov; +Cc: hreitz, eblake, jsnow, qemu-block, qemu-devel
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.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case 2023-02-02 18:15 [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case Andrey Zhadchenko via @ 2023-02-02 19:39 ` Eric Blake 2023-02-03 10:55 ` Andrey Zhadchenko 2023-02-14 17:13 ` Vladimir Sementsov-Ogievskiy 2023-02-16 11:01 ` Kevin Wolf 2 siblings, 1 reply; 6+ messages in thread From: Eric Blake @ 2023-02-02 19:39 UTC (permalink / raw) To: Andrey Zhadchenko; +Cc: vsementsov, hreitz, jsnow, qemu-block, qemu-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case 2023-02-02 19:39 ` Eric Blake @ 2023-02-03 10:55 ` Andrey Zhadchenko 2023-02-03 11:22 ` Andrey Zhadchenko 0 siblings, 1 reply; 6+ messages in thread From: Andrey Zhadchenko @ 2023-02-03 10:55 UTC (permalink / raw) To: Eric Blake; +Cc: vsementsov, hreitz, jsnow, qemu-block, qemu-devel 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. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case 2023-02-03 10:55 ` Andrey Zhadchenko @ 2023-02-03 11:22 ` Andrey Zhadchenko 0 siblings, 0 replies; 6+ messages in thread From: Andrey Zhadchenko @ 2023-02-03 11:22 UTC (permalink / raw) To: Eric Blake; +Cc: vsementsov, hreitz, jsnow, qemu-block, qemu-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case 2023-02-02 18:15 [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case Andrey Zhadchenko via 2023-02-02 19:39 ` Eric Blake @ 2023-02-14 17:13 ` Vladimir Sementsov-Ogievskiy 2023-02-16 11:01 ` Kevin Wolf 2 siblings, 0 replies; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2023-02-14 17:13 UTC (permalink / raw) To: Andrey Zhadchenko Cc: hreitz, eblake, jsnow, qemu-block, qemu-devel, Kevin Wolf 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case 2023-02-02 18:15 [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case Andrey Zhadchenko via 2023-02-02 19:39 ` Eric Blake 2023-02-14 17:13 ` Vladimir Sementsov-Ogievskiy @ 2023-02-16 11:01 ` Kevin Wolf 2 siblings, 0 replies; 6+ messages in thread From: Kevin Wolf @ 2023-02-16 11:01 UTC (permalink / raw) To: Andrey Zhadchenko Cc: vsementsov, hreitz, eblake, jsnow, qemu-block, qemu-devel Am 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-16 11:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-02 18:15 [PATCH] hbitmap: fix hbitmap_status() return value for first dirty bit case Andrey Zhadchenko via 2023-02-02 19:39 ` Eric Blake 2023-02-03 10:55 ` Andrey Zhadchenko 2023-02-03 11:22 ` Andrey Zhadchenko 2023-02-14 17:13 ` Vladimir Sementsov-Ogievskiy 2023-02-16 11:01 ` Kevin Wolf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).