until the non-accessible area

Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 block/copy-before-write.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 7e074ad569..c5e6e1c112 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -207,10 +207,11 @@ static int coroutine_fn GRAPH_RDLOCK cbw_co_flush(BlockDriverState *bs)
  */
 static BlockReq * coroutine_fn GRAPH_RDLOCK
 cbw_snapshot_read_lock(BlockDriverState *bs, int64_t offset, int64_t bytes,
-                       int64_t *pnum, BdrvChild **file)
+                       int64_t *pnum, BdrvChild **file, bool query)
 {
     BDRVCopyBeforeWriteState *s = bs->opaque;
     BlockReq *req = g_new(BlockReq, 1);
+    int64_t next_dirty;
     bool done;
 
     QEMU_LOCK_GUARD(&s->lock);
@@ -220,9 +221,13 @@ cbw_snapshot_read_lock(BlockDriverState *bs, int64_t offset, int64_t bytes,
         return NULL;
     }
 
-    if (bdrv_dirty_bitmap_next_dirty(s->access_bitmap, offset, bytes) != -1) {
-        g_free(req);
-        return NULL;
+    next_dirty = bdrv_dirty_bitmap_next_dirty(s->access_bitmap, offset, bytes);
+    if (next_dirty != -1) {
+        if (!query || next_dirty == offset) {
+            g_free(req);
+            return NULL;
+        }
+        bytes = offset + bytes - next_dirty;
     }
 
     done = bdrv_dirty_bitmap_status(s->done_bitmap, offset, bytes, pnum);
@@ -270,7 +275,8 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
     while (bytes) {
         int64_t cur_bytes;
 
-        req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &file);
+        req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &file,
+                                     false);
         if (!req) {
             return -EACCES;
         }
@@ -302,7 +308,7 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
     int64_t cur_bytes;
     BdrvChild *child;
 
-    req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &child);
+    req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &child, true);
     if (!req) {
         return -EACCES;
     }
-- 
2.43.0

On 28.05.25 15:07, Andrey Zhadchenko wrote:
> until the non-accessible area

Please write a bit more: what, why and how. What is the scenario when this change helps? I think the client of snapshot-access, should not try to read/get-block-status from "denied" areas?

Also, it's a lot more comfortable to read messages, starting from a full sentence, not just continuing a sentence of subject. It's OK to repeat the subject, may be in more detailed form.

> 
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
> ---
>   block/copy-before-write.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 7e074ad569..c5e6e1c112 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -207,10 +207,11 @@ static int coroutine_fn GRAPH_RDLOCK cbw_co_flush(BlockDriverState *bs)
>    */

the comment above the function should be updated to cover new logic

>   static BlockReq * coroutine_fn GRAPH_RDLOCK
>   cbw_snapshot_read_lock(BlockDriverState *bs, int64_t offset, int64_t bytes,
> -                       int64_t *pnum, BdrvChild **file)
> +                       int64_t *pnum, BdrvChild **file, bool query)
>   {
>       BDRVCopyBeforeWriteState *s = bs->opaque;
>       BlockReq *req = g_new(BlockReq, 1);
> +    int64_t next_dirty;
>       bool done;
>   
>       QEMU_LOCK_GUARD(&s->lock);
> @@ -220,9 +221,13 @@ cbw_snapshot_read_lock(BlockDriverState *bs, int64_t offset, int64_t bytes,
>           return NULL;
>       }
>   
> -    if (bdrv_dirty_bitmap_next_dirty(s->access_bitmap, offset, bytes) != -1) {
> -        g_free(req);
> -        return NULL;
> +    next_dirty = bdrv_dirty_bitmap_next_dirty(s->access_bitmap, offset, bytes);
> +    if (next_dirty != -1) {
> +        if (!query || next_dirty == offset) {
> +            g_free(req);
> +            return NULL;
> +        }
> +        bytes = offset + bytes - next_dirty;

I don't follow. Shouldn't this be

     bytes = next_dirty - offset

I.e., number of bytes from offset up to first "denied" byte.

>       }
>   
>       done = bdrv_dirty_bitmap_status(s->done_bitmap, offset, bytes, pnum);
> @@ -270,7 +275,8 @@ cbw_co_preadv_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes,
>       while (bytes) {
>           int64_t cur_bytes;
>   
> -        req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &file);
> +        req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &file,
> +                                     false);
>           if (!req) {
>               return -EACCES;
>           }
> @@ -302,7 +308,7 @@ cbw_co_snapshot_block_status(BlockDriverState *bs,
>       int64_t cur_bytes;
>       BdrvChild *child;
>   
> -    req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &child);
> +    req = cbw_snapshot_read_lock(bs, offset, bytes, &cur_bytes, &child, true);
>       if (!req) {
>           return -EACCES;
>       }

-- 
Best regards,
Vladimir