Re: [PATCH v2 4/4] block/copy-before-write: report partial block status to snapshot

[Vladimir Sementsov-Ogievskiy]

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.zhadche...@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