[PATCH v2 0/3] block: align CoR requests to subclusters

[PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev via]

v1 --> v2:
 * Fixed line indentation;
 * Fixed wording in a comment;
 * Added R-b.

v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html

Andrey Drobyshev (3):
  block: add subcluster_size field to BlockDriverInfo
  block/io: align requests to subcluster_size
  tests/qemu-iotests/197: add testcase for CoR with subclusters

 block.c                      |  7 +++++
 block/io.c                   | 50 ++++++++++++++++++------------------
 block/mirror.c               |  8 +++---
 block/qcow2.c                |  1 +
 include/block/block-common.h |  5 ++++
 include/block/block-io.h     |  8 +++---
 tests/qemu-iotests/197       | 29 +++++++++++++++++++++
 tests/qemu-iotests/197.out   | 24 +++++++++++++++++
 8 files changed, 99 insertions(+), 33 deletions(-)

-- 
2.39.3

[PATCH v2 1/3] block: add subcluster_size field to BlockDriverInfo

[Andrey Drobyshev via]

This is going to be used in the subsequent commit as requests alignment
(in particular, during copy-on-read).  This value only makes sense for
the formats which support subclusters (currently QCOW2 only).  If this
field isn't set by driver's own bdrv_get_info() implementation, we
simply set it equal to the cluster size thus treating each cluster as
having a single subcluster.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block.c                      | 7 +++++++
 block/qcow2.c                | 1 +
 include/block/block-common.h | 5 +++++
 3 files changed, 13 insertions(+)

diff --git a/block.c b/block.c
index a307c151a8..0af890f647 100644
--- a/block.c
+++ b/block.c
@@ -6480,6 +6480,13 @@ int coroutine_fn bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     }
     memset(bdi, 0, sizeof(*bdi));
     ret = drv->bdrv_co_get_info(bs, bdi);
+    if (bdi->subcluster_size == 0) {
+        /*
+         * If the driver left this unset, subclusters are not supported.
+         * Then it is safe to treat each cluster as having only one subcluster.
+         */
+        bdi->subcluster_size = bdi->cluster_size;
+    }
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index c51388e99d..b48cd9ce63 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5197,6 +5197,7 @@ qcow2_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
 {
     BDRVQcow2State *s = bs->opaque;
     bdi->cluster_size = s->cluster_size;
+    bdi->subcluster_size = s->subcluster_size;
     bdi->vm_state_offset = qcow2_vm_state_offset(s);
     bdi->is_dirty = s->incompatible_features & QCOW2_INCOMPAT_DIRTY;
     return 0;
diff --git a/include/block/block-common.h b/include/block/block-common.h
index e15395f2cb..df5ffc8d09 100644
--- a/include/block/block-common.h
+++ b/include/block/block-common.h
@@ -132,6 +132,11 @@ typedef struct BlockZoneWps {
 typedef struct BlockDriverInfo {
     /* in bytes, 0 if irrelevant */
     int cluster_size;
+    /*
+     * A fraction of cluster_size, if supported (currently QCOW2 only); if
+     * disabled or unsupported, set equal to cluster_size.
+     */
+    int subcluster_size;
     /* offset at which the VM state can be saved (0 if not possible) */
     int64_t vm_state_offset;
     bool is_dirty;
-- 
2.39.3

Re: [PATCH v2 1/3] block: add subcluster_size field to BlockDriverInfo

[Vladimir Sementsov-Ogievskiy]

On 11.07.23 20:25, Andrey Drobyshev wrote:
> This is going to be used in the subsequent commit as requests alignment
> (in particular, during copy-on-read).  This value only makes sense for
> the formats which support subclusters (currently QCOW2 only).  If this
> field isn't set by driver's own bdrv_get_info() implementation, we
> simply set it equal to the cluster size thus treating each cluster as
> having a single subcluster.
> 
> Reviewed-by: Eric Blake<eblake@redhat.com>
> Reviewed-by: Denis V. Lunev<den@openvz.org>
> Signed-off-by: Andrey Drobyshev<andrey.drobyshev@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v2 2/3] block/io: align requests to subcluster_size

[Andrey Drobyshev via]

When target image is using subclusters, and we align the request during
copy-on-read, it makes sense to align to subcluster_size rather than
cluster_size.  Otherwise we end up with unnecessary allocations.

This commit renames bdrv_round_to_clusters() to bdrv_round_to_subclusters()
and utilizes subcluster_size field of BlockDriverInfo to make necessary
alignments.  It affects copy-on-read as well as mirror job (which is
using bdrv_round_to_clusters()).

This change also fixes the following bug with failing assert (covered by
the test in the subsequent commit):

qemu-img create -f qcow2 base.qcow2 64K
qemu-img create -f qcow2 -o extended_l2=on,backing_file=base.qcow2,backing_fmt=qcow2 img.qcow2 64K
qemu-io -c "write -P 0xaa 0 2K" img.qcow2
qemu-io -C -c "read -P 0x00 2K 62K" img.qcow2

qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/io.c               | 50 ++++++++++++++++++++--------------------
 block/mirror.c           |  8 +++----
 include/block/block-io.h |  8 +++----
 3 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/block/io.c b/block/io.c
index e8293d6b26..4a2fef6a77 100644
--- a/block/io.c
+++ b/block/io.c
@@ -728,21 +728,21 @@ BdrvTrackedRequest *coroutine_fn bdrv_co_get_self_request(BlockDriverState *bs)
 }
 
 /**
- * Round a region to cluster boundaries
+ * Round a region to subcluster (if supported) or cluster boundaries
  */
 void coroutine_fn GRAPH_RDLOCK
-bdrv_round_to_clusters(BlockDriverState *bs, int64_t offset, int64_t bytes,
-                       int64_t *cluster_offset, int64_t *cluster_bytes)
+bdrv_round_to_subclusters(BlockDriverState *bs, int64_t offset, int64_t bytes,
+                          int64_t *align_offset, int64_t *align_bytes)
 {
     BlockDriverInfo bdi;
     IO_CODE();
-    if (bdrv_co_get_info(bs, &bdi) < 0 || bdi.cluster_size == 0) {
-        *cluster_offset = offset;
-        *cluster_bytes = bytes;
+    if (bdrv_co_get_info(bs, &bdi) < 0 || bdi.subcluster_size == 0) {
+        *align_offset = offset;
+        *align_bytes = bytes;
     } else {
-        int64_t c = bdi.cluster_size;
-        *cluster_offset = QEMU_ALIGN_DOWN(offset, c);
-        *cluster_bytes = QEMU_ALIGN_UP(offset - *cluster_offset + bytes, c);
+        int64_t c = bdi.subcluster_size;
+        *align_offset = QEMU_ALIGN_DOWN(offset, c);
+        *align_bytes = QEMU_ALIGN_UP(offset - *align_offset + bytes, c);
     }
 }
 
@@ -1168,8 +1168,8 @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,
     void *bounce_buffer = NULL;
 
     BlockDriver *drv = bs->drv;
-    int64_t cluster_offset;
-    int64_t cluster_bytes;
+    int64_t align_offset;
+    int64_t align_bytes;
     int64_t skip_bytes;
     int ret;
     int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer,
@@ -1203,28 +1203,28 @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,
      * BDRV_REQUEST_MAX_BYTES (even when the original read did not), which
      * is one reason we loop rather than doing it all at once.
      */
-    bdrv_round_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);
-    skip_bytes = offset - cluster_offset;
+    bdrv_round_to_subclusters(bs, offset, bytes, &align_offset, &align_bytes);
+    skip_bytes = offset - align_offset;
 
     trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
-                                   cluster_offset, cluster_bytes);
+                                   align_offset, align_bytes);
 
-    while (cluster_bytes) {
+    while (align_bytes) {
         int64_t pnum;
 
         if (skip_write) {
             ret = 1; /* "already allocated", so nothing will be copied */
-            pnum = MIN(cluster_bytes, max_transfer);
+            pnum = MIN(align_bytes, max_transfer);
         } else {
-            ret = bdrv_is_allocated(bs, cluster_offset,
-                                    MIN(cluster_bytes, max_transfer), &pnum);
+            ret = bdrv_is_allocated(bs, align_offset,
+                                    MIN(align_bytes, max_transfer), &pnum);
             if (ret < 0) {
                 /*
                  * Safe to treat errors in querying allocation as if
                  * unallocated; we'll probably fail again soon on the
                  * read, but at least that will set a decent errno.
                  */
-                pnum = MIN(cluster_bytes, max_transfer);
+                pnum = MIN(align_bytes, max_transfer);
             }
 
             /* Stop at EOF if the image ends in the middle of the cluster */
@@ -1242,7 +1242,7 @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,
             /* Must copy-on-read; use the bounce buffer */
             pnum = MIN(pnum, MAX_BOUNCE_BUFFER);
             if (!bounce_buffer) {
-                int64_t max_we_need = MAX(pnum, cluster_bytes - pnum);
+                int64_t max_we_need = MAX(pnum, align_bytes - pnum);
                 int64_t max_allowed = MIN(max_transfer, MAX_BOUNCE_BUFFER);
                 int64_t bounce_buffer_len = MIN(max_we_need, max_allowed);
 
@@ -1254,7 +1254,7 @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,
             }
             qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
 
-            ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
+            ret = bdrv_driver_preadv(bs, align_offset, pnum,
                                      &local_qiov, 0, 0);
             if (ret < 0) {
                 goto err;
@@ -1266,13 +1266,13 @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,
                 /* FIXME: Should we (perhaps conditionally) be setting
                  * BDRV_REQ_MAY_UNMAP, if it will allow for a sparser copy
                  * that still correctly reads as zero? */
-                ret = bdrv_co_do_pwrite_zeroes(bs, cluster_offset, pnum,
+                ret = bdrv_co_do_pwrite_zeroes(bs, align_offset, pnum,
                                                BDRV_REQ_WRITE_UNCHANGED);
             } else {
                 /* This does not change the data on the disk, it is not
                  * necessary to flush even in cache=writethrough mode.
                  */
-                ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+                ret = bdrv_driver_pwritev(bs, align_offset, pnum,
                                           &local_qiov, 0,
                                           BDRV_REQ_WRITE_UNCHANGED);
             }
@@ -1301,8 +1301,8 @@ bdrv_co_do_copy_on_readv(BdrvChild *child, int64_t offset, int64_t bytes,
             }
         }
 
-        cluster_offset += pnum;
-        cluster_bytes -= pnum;
+        align_offset += pnum;
+        align_bytes -= pnum;
         progress += pnum - skip_bytes;
         skip_bytes = 0;
     }
diff --git a/block/mirror.c b/block/mirror.c
index d3cacd1708..e213a892db 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -283,8 +283,8 @@ static int coroutine_fn mirror_cow_align(MirrorBlockJob *s, int64_t *offset,
     need_cow |= !test_bit((*offset + *bytes - 1) / s->granularity,
                           s->cow_bitmap);
     if (need_cow) {
-        bdrv_round_to_clusters(blk_bs(s->target), *offset, *bytes,
-                               &align_offset, &align_bytes);
+        bdrv_round_to_subclusters(blk_bs(s->target), *offset, *bytes,
+                                  &align_offset, &align_bytes);
     }
 
     if (align_bytes > max_bytes) {
@@ -576,8 +576,8 @@ static void coroutine_fn mirror_iteration(MirrorBlockJob *s)
             int64_t target_offset;
             int64_t target_bytes;
             WITH_GRAPH_RDLOCK_GUARD() {
-                bdrv_round_to_clusters(blk_bs(s->target), offset, io_bytes,
-                                       &target_offset, &target_bytes);
+                bdrv_round_to_subclusters(blk_bs(s->target), offset, io_bytes,
+                                          &target_offset, &target_bytes);
             }
             if (target_offset == offset &&
                 target_bytes == io_bytes) {
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 4415506e40..6db48f2d35 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -189,10 +189,10 @@ bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs,
                                           Error **errp);
 BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs);
-void bdrv_round_to_clusters(BlockDriverState *bs,
-                            int64_t offset, int64_t bytes,
-                            int64_t *cluster_offset,
-                            int64_t *cluster_bytes);
+void bdrv_round_to_subclusters(BlockDriverState *bs,
+                               int64_t offset, int64_t bytes,
+                               int64_t *cluster_offset,
+                               int64_t *cluster_bytes);
 
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
-- 
2.39.3

Re: [PATCH v2 2/3] block/io: align requests to subcluster_size

[Vladimir Sementsov-Ogievskiy]

On 11.07.23 20:25, Andrey Drobyshev wrote:
> When target image is using subclusters, and we align the request during
> copy-on-read, it makes sense to align to subcluster_size rather than
> cluster_size.  Otherwise we end up with unnecessary allocations.
> 
> This commit renames bdrv_round_to_clusters() to bdrv_round_to_subclusters()
> and utilizes subcluster_size field of BlockDriverInfo to make necessary
> alignments.  It affects copy-on-read as well as mirror job (which is
> using bdrv_round_to_clusters()).
> 
> This change also fixes the following bug with failing assert (covered by
> the test in the subsequent commit):
> 
> qemu-img create -f qcow2 base.qcow2 64K
> qemu-img create -f qcow2 -o extended_l2=on,backing_file=base.qcow2,backing_fmt=qcow2 img.qcow2 64K
> qemu-io -c "write -P 0xaa 0 2K" img.qcow2
> qemu-io -C -c "read -P 0x00 2K 62K" img.qcow2
> 
> qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.
> 
> Reviewed-by: Eric Blake<eblake@redhat.com>
> Reviewed-by: Denis V. Lunev<den@openvz.org>
> Signed-off-by: Andrey Drobyshev<andrey.drobyshev@virtuozzo.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

[PATCH v2 3/3] tests/qemu-iotests/197: add testcase for CoR with subclusters

[Andrey Drobyshev via]

Add testcase which checks that allocations during copy-on-read are
performed on the subcluster basis when subclusters are enabled in target
image.

This testcase also triggers the following assert with previous commit
not being applied, so we check that as well:

qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/197     | 29 +++++++++++++++++++++++++++++
 tests/qemu-iotests/197.out | 24 ++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
index a2547bc280..f07a9da136 100755
--- a/tests/qemu-iotests/197
+++ b/tests/qemu-iotests/197
@@ -122,6 +122,35 @@ $QEMU_IO -f qcow2 -C -c 'read 0 1024' "$TEST_WRAP" | _filter_qemu_io
 $QEMU_IO -f qcow2 -c map "$TEST_WRAP"
 _check_test_img
 
+echo
+echo '=== Copy-on-read with subclusters ==='
+echo
+
+# Create base and top images 64K (1 cluster) each.  Make subclusters enabled
+# for the top image
+_make_test_img 64K
+IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
+    _make_test_img --no-opts -o extended_l2=true -F "$IMGFMT" -b "$TEST_IMG" \
+    64K | _filter_img_create
+
+$QEMU_IO -c "write -P 0xaa 0 64k" "$TEST_IMG" | _filter_qemu_io
+
+# Allocate individual subclusters in the top image, and not the whole cluster
+$QEMU_IO -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \
+    | _filter_qemu_io
+
+# Only 2 subclusters should be allocated in the top image at this point
+$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
+
+# Actual copy-on-read operation
+$QEMU_IO -C -c "read -P 0xaa 30K 4K" "$TEST_WRAP" | _filter_qemu_io
+
+# And here we should have 4 subclusters allocated right in the middle of the
+# top image. Make sure the whole cluster remains unallocated
+$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
+
+_check_test_img
+
 # success, all done
 echo '*** done'
 status=0
diff --git a/tests/qemu-iotests/197.out b/tests/qemu-iotests/197.out
index ad414c3b0e..8f34a30afe 100644
--- a/tests/qemu-iotests/197.out
+++ b/tests/qemu-iotests/197.out
@@ -31,4 +31,28 @@ read 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
 No errors were found on the image.
+
+=== Copy-on-read with subclusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
+Formatting 'TEST_DIR/t.wrap.IMGFMT', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 2048/2048 bytes at offset 28672
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 2048/2048 bytes at offset 34816
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          File
+0               0x7000          TEST_DIR/t.IMGFMT
+0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
+0x7800          0x1000          TEST_DIR/t.IMGFMT
+0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
+0x9000          0x7000          TEST_DIR/t.IMGFMT
+read 4096/4096 bytes at offset 30720
+4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset          Length          File
+0               0x7000          TEST_DIR/t.IMGFMT
+0x7000          0x2000          TEST_DIR/t.wrap.IMGFMT
+0x9000          0x7000          TEST_DIR/t.IMGFMT
+No errors were found on the image.
 *** done
-- 
2.39.3

Re: [PATCH v2 3/3] tests/qemu-iotests/197: add testcase for CoR with subclusters

[Vladimir Sementsov-Ogievskiy]

On 11.07.23 20:25, Andrey Drobyshev via wrote:
> Add testcase which checks that allocations during copy-on-read are
> performed on the subcluster basis when subclusters are enabled in target
> image.
> 
> This testcase also triggers the following assert with previous commit
> not being applied, so we check that as well:
> 
> qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.
> 
> Reviewed-by: Eric Blake<eblake@redhat.com>
> Reviewed-by: Denis V. Lunev<den@openvz.org>
> Signed-off-by: Andrey Drobyshev<andrey.drobyshev@virtuozzo.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/3] tests/qemu-iotests/197: add testcase for CoR with subclusters

[Denis V. Lunev]

On 7/11/23 19:25, Andrey Drobyshev wrote:
> Add testcase which checks that allocations during copy-on-read are
> performed on the subcluster basis when subclusters are enabled in target
> image.
>
> This testcase also triggers the following assert with previous commit
> not being applied, so we check that as well:
>
> qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion `skip_bytes < pnum' failed.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   tests/qemu-iotests/197     | 29 +++++++++++++++++++++++++++++
>   tests/qemu-iotests/197.out | 24 ++++++++++++++++++++++++
>   2 files changed, 53 insertions(+)
>
> diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
> index a2547bc280..f07a9da136 100755
> --- a/tests/qemu-iotests/197
> +++ b/tests/qemu-iotests/197
> @@ -122,6 +122,35 @@ $QEMU_IO -f qcow2 -C -c 'read 0 1024' "$TEST_WRAP" | _filter_qemu_io
>   $QEMU_IO -f qcow2 -c map "$TEST_WRAP"
>   _check_test_img
>   
> +echo
> +echo '=== Copy-on-read with subclusters ==='
> +echo
> +
> +# Create base and top images 64K (1 cluster) each.  Make subclusters enabled
> +# for the top image
> +_make_test_img 64K
> +IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
> +    _make_test_img --no-opts -o extended_l2=true -F "$IMGFMT" -b "$TEST_IMG" \
> +    64K | _filter_img_create
> +
> +$QEMU_IO -c "write -P 0xaa 0 64k" "$TEST_IMG" | _filter_qemu_io
> +
> +# Allocate individual subclusters in the top image, and not the whole cluster
> +$QEMU_IO -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \
> +    | _filter_qemu_io
> +
> +# Only 2 subclusters should be allocated in the top image at this point
> +$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
> +
> +# Actual copy-on-read operation
> +$QEMU_IO -C -c "read -P 0xaa 30K 4K" "$TEST_WRAP" | _filter_qemu_io
> +
> +# And here we should have 4 subclusters allocated right in the middle of the
> +# top image. Make sure the whole cluster remains unallocated
> +$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
> +
> +_check_test_img
> +
>   # success, all done
>   echo '*** done'
>   status=0
> diff --git a/tests/qemu-iotests/197.out b/tests/qemu-iotests/197.out
> index ad414c3b0e..8f34a30afe 100644
> --- a/tests/qemu-iotests/197.out
> +++ b/tests/qemu-iotests/197.out
> @@ -31,4 +31,28 @@ read 1024/1024 bytes at offset 0
>   1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
>   No errors were found on the image.
> +
> +=== Copy-on-read with subclusters ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
> +Formatting 'TEST_DIR/t.wrap.IMGFMT', fmt=IMGFMT size=65536 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 2048/2048 bytes at offset 28672
> +2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 2048/2048 bytes at offset 34816
> +2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset          Length          File
> +0               0x7000          TEST_DIR/t.IMGFMT
> +0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
> +0x7800          0x1000          TEST_DIR/t.IMGFMT
> +0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
> +0x9000          0x7000          TEST_DIR/t.IMGFMT
> +read 4096/4096 bytes at offset 30720
> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +Offset          Length          File
> +0               0x7000          TEST_DIR/t.IMGFMT
> +0x7000          0x2000          TEST_DIR/t.wrap.IMGFMT
> +0x9000          0x7000          TEST_DIR/t.IMGFMT
> +No errors were found on the image.
>   *** done
It is revealed that this patch seems to break unit tests if run for NBD 
format

iris ~/src/qemu/build/tests/qemu-iotests $ ./check -nbd 197
QEMU          -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-system-x86_64" 
-nodefaults -display none -accel qtest
QEMU_IMG      -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-img"
QEMU_IO       -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-io" --cache 
writeback --aio threads -f raw
QEMU_NBD      -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-nbd"
IMGFMT        -- raw
IMGPROTO      -- nbd
PLATFORM      -- Linux/x86_64 iris 6.2.0-31-generic
TEST_DIR      -- /home/den/src/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR      -- /tmp/tmpzw5ky8d3
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

197   fail       [11:41:26] [11:41:31]   5.7s   (last: 3.8s)  output 
mismatch (see 
/home/den/src/qemu/build/tests/qemu-iotests/scratch/raw-nbd-197/197.out.bad)
--- /home/den/src/qemu/tests/qemu-iotests/197.out
+++ 
/home/den/src/qemu/build/tests/qemu-iotests/scratch/raw-nbd-197/197.out.bad
@@ -43,16 +43,11 @@
  wrote 2048/2048 bytes at offset 34816
  2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  Offset          Length          File
-0               0x7000          TEST_DIR/t.IMGFMT
-0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
-0x7800          0x1000          TEST_DIR/t.IMGFMT
-0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
-0x9000          0x7000          TEST_DIR/t.IMGFMT
+0               0x10000         nbd+unix://?socket=SOCK_DIR/nbd
+Pattern verification failed at offset 30720, 4096 bytes
  read 4096/4096 bytes at offset 30720
  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
  Offset          Length          File
-0               0x7000          TEST_DIR/t.IMGFMT
-0x7000          0x2000          TEST_DIR/t.wrap.IMGFMT
-0x9000          0x7000          TEST_DIR/t.IMGFMT
+0               0x10000         nbd+unix://?socket=SOCK_DIR/nbd
  No errors were found on the image.
  *** done
Failures: 197
Failed 1 of 1 iotests
iris ~/src/qemu/build/tests/qemu-iotests $

it is good for QCOW2 format.

iris ~/src/qemu/build/tests/qemu-iotests $ ./check -qcow2 197
QEMU          -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-system-x86_64" 
-nodefaults -display none -accel qtest
QEMU_IMG      -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-img"
QEMU_IO       -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-io" --cache 
writeback --aio threads -f qcow2
QEMU_NBD      -- 
"/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-nbd"
IMGFMT        -- qcow2
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 iris 6.2.0-31-generic
TEST_DIR      -- /home/den/src/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR      -- /tmp/tmpe1qldhs9
GDB_OPTIONS   --
VALGRIND_QEMU --
PRINT_QEMU_OUTPUT --

197   pass       [11:42:14] [11:42:17]   2.8s
Passed all 1 iotests
iris ~/src/qemu/build/tests/qemu-iotests $

Regards,
     Den

Re: [PATCH v2 3/3] tests/qemu-iotests/197: add testcase for CoR with subclusters

[Andrey Drobyshev]

On 9/6/23 12:43, Denis V. Lunev wrote:
> On 7/11/23 19:25, Andrey Drobyshev wrote:
>> Add testcase which checks that allocations during copy-on-read are
>> performed on the subcluster basis when subclusters are enabled in target
>> image.
>>
>> This testcase also triggers the following assert with previous commit
>> not being applied, so we check that as well:
>>
>> qemu-io: ../block/io.c:1236: bdrv_co_do_copy_on_readv: Assertion
>> `skip_bytes < pnum' failed.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/197     | 29 +++++++++++++++++++++++++++++
>>   tests/qemu-iotests/197.out | 24 ++++++++++++++++++++++++
>>   2 files changed, 53 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
>> index a2547bc280..f07a9da136 100755
>> --- a/tests/qemu-iotests/197
>> +++ b/tests/qemu-iotests/197
>> @@ -122,6 +122,35 @@ $QEMU_IO -f qcow2 -C -c 'read 0 1024'
>> "$TEST_WRAP" | _filter_qemu_io
>>   $QEMU_IO -f qcow2 -c map "$TEST_WRAP"
>>   _check_test_img
>>   +echo
>> +echo '=== Copy-on-read with subclusters ==='
>> +echo
>> +
>> +# Create base and top images 64K (1 cluster) each.  Make subclusters
>> enabled
>> +# for the top image
>> +_make_test_img 64K
>> +IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
>> +    _make_test_img --no-opts -o extended_l2=true -F "$IMGFMT" -b
>> "$TEST_IMG" \
>> +    64K | _filter_img_create
>> +
>> +$QEMU_IO -c "write -P 0xaa 0 64k" "$TEST_IMG" | _filter_qemu_io
>> +
>> +# Allocate individual subclusters in the top image, and not the whole
>> cluster
>> +$QEMU_IO -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K"
>> "$TEST_WRAP" \
>> +    | _filter_qemu_io
>> +
>> +# Only 2 subclusters should be allocated in the top image at this point
>> +$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
>> +
>> +# Actual copy-on-read operation
>> +$QEMU_IO -C -c "read -P 0xaa 30K 4K" "$TEST_WRAP" | _filter_qemu_io
>> +
>> +# And here we should have 4 subclusters allocated right in the middle
>> of the
>> +# top image. Make sure the whole cluster remains unallocated
>> +$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
>> +
>> +_check_test_img
>> +
>>   # success, all done
>>   echo '*** done'
>>   status=0
>> diff --git a/tests/qemu-iotests/197.out b/tests/qemu-iotests/197.out
>> index ad414c3b0e..8f34a30afe 100644
>> --- a/tests/qemu-iotests/197.out
>> +++ b/tests/qemu-iotests/197.out
>> @@ -31,4 +31,28 @@ read 1024/1024 bytes at offset 0
>>   1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
>>   No errors were found on the image.
>> +
>> +=== Copy-on-read with subclusters ===
>> +
>> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536
>> +Formatting 'TEST_DIR/t.wrap.IMGFMT', fmt=IMGFMT size=65536
>> backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
>> +wrote 65536/65536 bytes at offset 0
>> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 2048/2048 bytes at offset 28672
>> +2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +wrote 2048/2048 bytes at offset 34816
>> +2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +Offset          Length          File
>> +0               0x7000          TEST_DIR/t.IMGFMT
>> +0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
>> +0x7800          0x1000          TEST_DIR/t.IMGFMT
>> +0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
>> +0x9000          0x7000          TEST_DIR/t.IMGFMT
>> +read 4096/4096 bytes at offset 30720
>> +4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +Offset          Length          File
>> +0               0x7000          TEST_DIR/t.IMGFMT
>> +0x7000          0x2000          TEST_DIR/t.wrap.IMGFMT
>> +0x9000          0x7000          TEST_DIR/t.IMGFMT
>> +No errors were found on the image.
>>   *** done
> It is revealed that this patch seems to break unit tests if run for NBD
> format
> 
> iris ~/src/qemu/build/tests/qemu-iotests $ ./check -nbd 197
> QEMU          --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-system-x86_64"
> -nodefaults -display none -accel qtest
> QEMU_IMG      --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-img"
> QEMU_IO       --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-io" --cache
> writeback --aio threads -f raw
> QEMU_NBD      --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT        -- raw
> IMGPROTO      -- nbd
> PLATFORM      -- Linux/x86_64 iris 6.2.0-31-generic
> TEST_DIR      -- /home/den/src/qemu/build/tests/qemu-iotests/scratch
> SOCK_DIR      -- /tmp/tmpzw5ky8d3
> GDB_OPTIONS   --
> VALGRIND_QEMU --
> PRINT_QEMU_OUTPUT --
> 
> 197   fail       [11:41:26] [11:41:31]   5.7s   (last: 3.8s)  output
> mismatch (see
> /home/den/src/qemu/build/tests/qemu-iotests/scratch/raw-nbd-197/197.out.bad)
> --- /home/den/src/qemu/tests/qemu-iotests/197.out
> +++
> /home/den/src/qemu/build/tests/qemu-iotests/scratch/raw-nbd-197/197.out.bad
> @@ -43,16 +43,11 @@
>  wrote 2048/2048 bytes at offset 34816
>  2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  Offset          Length          File
> -0               0x7000          TEST_DIR/t.IMGFMT
> -0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
> -0x7800          0x1000          TEST_DIR/t.IMGFMT
> -0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
> -0x9000          0x7000          TEST_DIR/t.IMGFMT
> +0               0x10000         nbd+unix://?socket=SOCK_DIR/nbd
> +Pattern verification failed at offset 30720, 4096 bytes
>  read 4096/4096 bytes at offset 30720
>  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  Offset          Length          File
> -0               0x7000          TEST_DIR/t.IMGFMT
> -0x7000          0x2000          TEST_DIR/t.wrap.IMGFMT
> -0x9000          0x7000          TEST_DIR/t.IMGFMT
> +0               0x10000         nbd+unix://?socket=SOCK_DIR/nbd
>  No errors were found on the image.
>  *** done
> Failures: 197
> Failed 1 of 1 iotests
> iris ~/src/qemu/build/tests/qemu-iotests $
> 
> it is good for QCOW2 format.
> 
> iris ~/src/qemu/build/tests/qemu-iotests $ ./check -qcow2 197
> QEMU          --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-system-x86_64"
> -nodefaults -display none -accel qtest
> QEMU_IMG      --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-img"
> QEMU_IO       --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-io" --cache
> writeback --aio threads -f qcow2
> QEMU_NBD      --
> "/home/den/src/qemu/build/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT        -- qcow2
> IMGPROTO      -- file
> PLATFORM      -- Linux/x86_64 iris 6.2.0-31-generic
> TEST_DIR      -- /home/den/src/qemu/build/tests/qemu-iotests/scratch
> SOCK_DIR      -- /tmp/tmpe1qldhs9
> GDB_OPTIONS   --
> VALGRIND_QEMU --
> PRINT_QEMU_OUTPUT --
> 
> 197   pass       [11:42:14] [11:42:17]   2.8s
> Passed all 1 iotests
> iris ~/src/qemu/build/tests/qemu-iotests $
> 
> Regards,
>     Den

Thanks for pointing out, apparently I didn't realize that this test
should accept ANY format as a backing file.  I've sent a fixup as patch 4/3.

Andrey

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev]

On 7/11/23 20:25, Andrey Drobyshev wrote:
> v1 --> v2:
>  * Fixed line indentation;
>  * Fixed wording in a comment;
>  * Added R-b.
> 
> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
> 
> Andrey Drobyshev (3):
>   block: add subcluster_size field to BlockDriverInfo
>   block/io: align requests to subcluster_size
>   tests/qemu-iotests/197: add testcase for CoR with subclusters
> 
>  block.c                      |  7 +++++
>  block/io.c                   | 50 ++++++++++++++++++------------------
>  block/mirror.c               |  8 +++---
>  block/qcow2.c                |  1 +
>  include/block/block-common.h |  5 ++++
>  include/block/block-io.h     |  8 +++---
>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>  8 files changed, 99 insertions(+), 33 deletions(-)
> 

Ping

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev]

On 7/24/23 16:11, Andrey Drobyshev wrote:
> On 7/11/23 20:25, Andrey Drobyshev wrote:
>> v1 --> v2:
>>  * Fixed line indentation;
>>  * Fixed wording in a comment;
>>  * Added R-b.
>>
>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
>>
>> Andrey Drobyshev (3):
>>   block: add subcluster_size field to BlockDriverInfo
>>   block/io: align requests to subcluster_size
>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
>>
>>  block.c                      |  7 +++++
>>  block/io.c                   | 50 ++++++++++++++++++------------------
>>  block/mirror.c               |  8 +++---
>>  block/qcow2.c                |  1 +
>>  include/block/block-common.h |  5 ++++
>>  include/block/block-io.h     |  8 +++---
>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>>  8 files changed, 99 insertions(+), 33 deletions(-)
>>
> 
> Ping

Another ping

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev]

On 7/31/23 17:51, Andrey Drobyshev wrote:
> On 7/24/23 16:11, Andrey Drobyshev wrote:
>> On 7/11/23 20:25, Andrey Drobyshev wrote:
>>> v1 --> v2:
>>>  * Fixed line indentation;
>>>  * Fixed wording in a comment;
>>>  * Added R-b.
>>>
>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
>>>
>>> Andrey Drobyshev (3):
>>>   block: add subcluster_size field to BlockDriverInfo
>>>   block/io: align requests to subcluster_size
>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
>>>
>>>  block.c                      |  7 +++++
>>>  block/io.c                   | 50 ++++++++++++++++++------------------
>>>  block/mirror.c               |  8 +++---
>>>  block/qcow2.c                |  1 +
>>>  include/block/block-common.h |  5 ++++
>>>  include/block/block-io.h     |  8 +++---
>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>>>  8 files changed, 99 insertions(+), 33 deletions(-)
>>>
>>
>> Ping
> 
> Another ping

Yet another friendly ping

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev]

On 8/16/23 12:22, Andrey Drobyshev wrote:
> On 7/31/23 17:51, Andrey Drobyshev wrote:
>> On 7/24/23 16:11, Andrey Drobyshev wrote:
>>> On 7/11/23 20:25, Andrey Drobyshev wrote:
>>>> v1 --> v2:
>>>>  * Fixed line indentation;
>>>>  * Fixed wording in a comment;
>>>>  * Added R-b.
>>>>
>>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
>>>>
>>>> Andrey Drobyshev (3):
>>>>   block: add subcluster_size field to BlockDriverInfo
>>>>   block/io: align requests to subcluster_size
>>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
>>>>
>>>>  block.c                      |  7 +++++
>>>>  block/io.c                   | 50 ++++++++++++++++++------------------
>>>>  block/mirror.c               |  8 +++---
>>>>  block/qcow2.c                |  1 +
>>>>  include/block/block-common.h |  5 ++++
>>>>  include/block/block-io.h     |  8 +++---
>>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>>>>  8 files changed, 99 insertions(+), 33 deletions(-)
>>>>
>>>
>>> Ping
>>
>> Another ping
> 
> Yet another friendly ping

One more friendly ping

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[John Snow]

On Tue, Aug 22, 2023 at 1:33 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> On 8/16/23 12:22, Andrey Drobyshev wrote:
> > On 7/31/23 17:51, Andrey Drobyshev wrote:
> >> On 7/24/23 16:11, Andrey Drobyshev wrote:
> >>> On 7/11/23 20:25, Andrey Drobyshev wrote:
> >>>> v1 --> v2:
> >>>>  * Fixed line indentation;
> >>>>  * Fixed wording in a comment;
> >>>>  * Added R-b.
> >>>>
> >>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
> >>>>
> >>>> Andrey Drobyshev (3):
> >>>>   block: add subcluster_size field to BlockDriverInfo
> >>>>   block/io: align requests to subcluster_size
> >>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
> >>>>
> >>>>  block.c                      |  7 +++++
> >>>>  block/io.c                   | 50 ++++++++++++++++++------------------
> >>>>  block/mirror.c               |  8 +++---
> >>>>  block/qcow2.c                |  1 +
> >>>>  include/block/block-common.h |  5 ++++
> >>>>  include/block/block-io.h     |  8 +++---
> >>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
> >>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
> >>>>  8 files changed, 99 insertions(+), 33 deletions(-)
> >>>>
> >>>
> >>> Ping
> >>
> >> Another ping
> >
> > Yet another friendly ping
>
> One more friendly ping

Looks like Stefan gave you an R-B for the series; do we just need an
ACK by the block maintainers at this point or is there someone
specific you're hoping will review this?

--js

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev]

On 8/22/23 22:58, John Snow wrote:
> On Tue, Aug 22, 2023 at 1:33 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>>
>> On 8/16/23 12:22, Andrey Drobyshev wrote:
>>> On 7/31/23 17:51, Andrey Drobyshev wrote:
>>>> On 7/24/23 16:11, Andrey Drobyshev wrote:
>>>>> On 7/11/23 20:25, Andrey Drobyshev wrote:
>>>>>> v1 --> v2:
>>>>>>  * Fixed line indentation;
>>>>>>  * Fixed wording in a comment;
>>>>>>  * Added R-b.
>>>>>>
>>>>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
>>>>>>
>>>>>> Andrey Drobyshev (3):
>>>>>>   block: add subcluster_size field to BlockDriverInfo
>>>>>>   block/io: align requests to subcluster_size
>>>>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
>>>>>>
>>>>>>  block.c                      |  7 +++++
>>>>>>  block/io.c                   | 50 ++++++++++++++++++------------------
>>>>>>  block/mirror.c               |  8 +++---
>>>>>>  block/qcow2.c                |  1 +
>>>>>>  include/block/block-common.h |  5 ++++
>>>>>>  include/block/block-io.h     |  8 +++---
>>>>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>>>>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>>>>>>  8 files changed, 99 insertions(+), 33 deletions(-)
>>>>>>
>>>>>
>>>>> Ping
>>>>
>>>> Another ping
>>>
>>> Yet another friendly ping
>>
>> One more friendly ping
> 
> Looks like Stefan gave you an R-B for the series; do we just need an
> ACK by the block maintainers at this point or is there someone
> specific you're hoping will review this?
> 
> --js
> 

Hi John,

I figure a maintainer's R-b doesn't imply the patches being merged into
the tree.  Hence I'm waiting for the notice that they actually are merged.

Please let me know if the process should be different.

Andrey

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2256 bytes --]

On Wed, Aug 23, 2023 at 03:50:55PM +0300, Andrey Drobyshev wrote:
> On 8/22/23 22:58, John Snow wrote:
> > On Tue, Aug 22, 2023 at 1:33 PM Andrey Drobyshev
> > <andrey.drobyshev@virtuozzo.com> wrote:
> >>
> >> On 8/16/23 12:22, Andrey Drobyshev wrote:
> >>> On 7/31/23 17:51, Andrey Drobyshev wrote:
> >>>> On 7/24/23 16:11, Andrey Drobyshev wrote:
> >>>>> On 7/11/23 20:25, Andrey Drobyshev wrote:
> >>>>>> v1 --> v2:
> >>>>>>  * Fixed line indentation;
> >>>>>>  * Fixed wording in a comment;
> >>>>>>  * Added R-b.
> >>>>>>
> >>>>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
> >>>>>>
> >>>>>> Andrey Drobyshev (3):
> >>>>>>   block: add subcluster_size field to BlockDriverInfo
> >>>>>>   block/io: align requests to subcluster_size
> >>>>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
> >>>>>>
> >>>>>>  block.c                      |  7 +++++
> >>>>>>  block/io.c                   | 50 ++++++++++++++++++------------------
> >>>>>>  block/mirror.c               |  8 +++---
> >>>>>>  block/qcow2.c                |  1 +
> >>>>>>  include/block/block-common.h |  5 ++++
> >>>>>>  include/block/block-io.h     |  8 +++---
> >>>>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
> >>>>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
> >>>>>>  8 files changed, 99 insertions(+), 33 deletions(-)
> >>>>>>
> >>>>>
> >>>>> Ping
> >>>>
> >>>> Another ping
> >>>
> >>> Yet another friendly ping
> >>
> >> One more friendly ping
> > 
> > Looks like Stefan gave you an R-B for the series; do we just need an
> > ACK by the block maintainers at this point or is there someone
> > specific you're hoping will review this?
> > 
> > --js
> > 
> 
> Hi John,
> 
> I figure a maintainer's R-b doesn't imply the patches being merged into
> the tree.  Hence I'm waiting for the notice that they actually are merged.
> 
> Please let me know if the process should be different.

Hi Andrey,
Kevin is away right now but seemed happy enough when I mentioned this
series to him, so I have merged this into my own tree:

  https://gitlab.com/stefanha/qemu block

Sorry that your patch series have not been merged in a timely manner.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Andrey Drobyshev]

On 8/24/23 17:32, Stefan Hajnoczi wrote:
> On Wed, Aug 23, 2023 at 03:50:55PM +0300, Andrey Drobyshev wrote:
>> On 8/22/23 22:58, John Snow wrote:
>>> On Tue, Aug 22, 2023 at 1:33 PM Andrey Drobyshev
>>> <andrey.drobyshev@virtuozzo.com> wrote:
>>>>
>>>> On 8/16/23 12:22, Andrey Drobyshev wrote:
>>>>> On 7/31/23 17:51, Andrey Drobyshev wrote:
>>>>>> On 7/24/23 16:11, Andrey Drobyshev wrote:
>>>>>>> On 7/11/23 20:25, Andrey Drobyshev wrote:
>>>>>>>> v1 --> v2:
>>>>>>>>  * Fixed line indentation;
>>>>>>>>  * Fixed wording in a comment;
>>>>>>>>  * Added R-b.
>>>>>>>>
>>>>>>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
>>>>>>>>
>>>>>>>> Andrey Drobyshev (3):
>>>>>>>>   block: add subcluster_size field to BlockDriverInfo
>>>>>>>>   block/io: align requests to subcluster_size
>>>>>>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
>>>>>>>>
>>>>>>>>  block.c                      |  7 +++++
>>>>>>>>  block/io.c                   | 50 ++++++++++++++++++------------------
>>>>>>>>  block/mirror.c               |  8 +++---
>>>>>>>>  block/qcow2.c                |  1 +
>>>>>>>>  include/block/block-common.h |  5 ++++
>>>>>>>>  include/block/block-io.h     |  8 +++---
>>>>>>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>>>>>>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>>>>>>>>  8 files changed, 99 insertions(+), 33 deletions(-)
>>>>>>>>
>>>>>>>
>>>>>>> Ping
>>>>>>
>>>>>> Another ping
>>>>>
>>>>> Yet another friendly ping
>>>>
>>>> One more friendly ping
>>>
>>> Looks like Stefan gave you an R-B for the series; do we just need an
>>> ACK by the block maintainers at this point or is there someone
>>> specific you're hoping will review this?
>>>
>>> --js
>>>
>>
>> Hi John,
>>
>> I figure a maintainer's R-b doesn't imply the patches being merged into
>> the tree.  Hence I'm waiting for the notice that they actually are merged.
>>
>> Please let me know if the process should be different.
> 
> Hi Andrey,
> Kevin is away right now but seemed happy enough when I mentioned this
> series to him, so I have merged this into my own tree:
> 
>   https://gitlab.com/stefanha/qemu block
> 
> Sorry that your patch series have not been merged in a timely manner.
> 
> Stefan

Hi Stefan,
Good news! Thank you for the notice.

When you have some time would you mind taking a look at these series as
well:

https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00068.html
https://lists.nongnu.org/archive/html/qemu-block/2023-07/msg00106.html

They've been hanging in the list for weeks (probably even longer than
this one) with little to no feedback.  Would be nice to know if people
find those changes useful at all.

Thanks in advance!

Andrey

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

On Thu, Aug 24, 2023 at 06:47:20PM +0300, Andrey Drobyshev wrote:
> On 8/24/23 17:32, Stefan Hajnoczi wrote:
> > On Wed, Aug 23, 2023 at 03:50:55PM +0300, Andrey Drobyshev wrote:
> >> On 8/22/23 22:58, John Snow wrote:
> >>> On Tue, Aug 22, 2023 at 1:33 PM Andrey Drobyshev
> >>> <andrey.drobyshev@virtuozzo.com> wrote:
> >>>>
> >>>> On 8/16/23 12:22, Andrey Drobyshev wrote:
> >>>>> On 7/31/23 17:51, Andrey Drobyshev wrote:
> >>>>>> On 7/24/23 16:11, Andrey Drobyshev wrote:
> >>>>>>> On 7/11/23 20:25, Andrey Drobyshev wrote:
> >>>>>>>> v1 --> v2:
> >>>>>>>>  * Fixed line indentation;
> >>>>>>>>  * Fixed wording in a comment;
> >>>>>>>>  * Added R-b.
> >>>>>>>>
> >>>>>>>> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
> >>>>>>>>
> >>>>>>>> Andrey Drobyshev (3):
> >>>>>>>>   block: add subcluster_size field to BlockDriverInfo
> >>>>>>>>   block/io: align requests to subcluster_size
> >>>>>>>>   tests/qemu-iotests/197: add testcase for CoR with subclusters
> >>>>>>>>
> >>>>>>>>  block.c                      |  7 +++++
> >>>>>>>>  block/io.c                   | 50 ++++++++++++++++++------------------
> >>>>>>>>  block/mirror.c               |  8 +++---
> >>>>>>>>  block/qcow2.c                |  1 +
> >>>>>>>>  include/block/block-common.h |  5 ++++
> >>>>>>>>  include/block/block-io.h     |  8 +++---
> >>>>>>>>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
> >>>>>>>>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
> >>>>>>>>  8 files changed, 99 insertions(+), 33 deletions(-)
> >>>>>>>>
> >>>>>>>
> >>>>>>> Ping
> >>>>>>
> >>>>>> Another ping
> >>>>>
> >>>>> Yet another friendly ping
> >>>>
> >>>> One more friendly ping
> >>>
> >>> Looks like Stefan gave you an R-B for the series; do we just need an
> >>> ACK by the block maintainers at this point or is there someone
> >>> specific you're hoping will review this?
> >>>
> >>> --js
> >>>
> >>
> >> Hi John,
> >>
> >> I figure a maintainer's R-b doesn't imply the patches being merged into
> >> the tree.  Hence I'm waiting for the notice that they actually are merged.
> >>
> >> Please let me know if the process should be different.
> > 
> > Hi Andrey,
> > Kevin is away right now but seemed happy enough when I mentioned this
> > series to him, so I have merged this into my own tree:
> > 
> >   https://gitlab.com/stefanha/qemu block
> > 
> > Sorry that your patch series have not been merged in a timely manner.
> > 
> > Stefan
> 
> Hi Stefan,
> Good news! Thank you for the notice.
> 
> When you have some time would you mind taking a look at these series as
> well:
> 
> https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00068.html
> https://lists.nongnu.org/archive/html/qemu-block/2023-07/msg00106.html
> 
> They've been hanging in the list for weeks (probably even longer than
> this one) with little to no feedback.  Would be nice to know if people
> find those changes useful at all.

Kevin will be back on Tuesday and Hanna Czenczek said she may have time
to look before then. Hopefully they will get taken care of soon.

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Tue, Jul 11, 2023 at 08:25:50PM +0300, Andrey Drobyshev via wrote:
> v1 --> v2:
>  * Fixed line indentation;
>  * Fixed wording in a comment;
>  * Added R-b.
> 
> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
> 
> Andrey Drobyshev (3):
>   block: add subcluster_size field to BlockDriverInfo
>   block/io: align requests to subcluster_size
>   tests/qemu-iotests/197: add testcase for CoR with subclusters
> 
>  block.c                      |  7 +++++
>  block/io.c                   | 50 ++++++++++++++++++------------------
>  block/mirror.c               |  8 +++---
>  block/qcow2.c                |  1 +
>  include/block/block-common.h |  5 ++++
>  include/block/block-io.h     |  8 +++---
>  tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>  tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>  8 files changed, 99 insertions(+), 33 deletions(-)
> 
> -- 
> 2.39.3
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Michael Tokarev]

11.07.2023 20:25, Andrey Drobyshev via wrote:
> v1 --> v2:
>   * Fixed line indentation;
>   * Fixed wording in a comment;
>   * Added R-b.
> 
> v1: https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
> 
> Andrey Drobyshev (3):
>    block: add subcluster_size field to BlockDriverInfo
>    block/io: align requests to subcluster_size
>    tests/qemu-iotests/197: add testcase for CoR with subclusters
> 
>   block.c                      |  7 +++++
>   block/io.c                   | 50 ++++++++++++++++++------------------
>   block/mirror.c               |  8 +++---
>   block/qcow2.c                |  1 +
>   include/block/block-common.h |  5 ++++
>   include/block/block-io.h     |  8 +++---
>   tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>   tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>   8 files changed, 99 insertions(+), 33 deletions(-)

So, given the size of patch series and amount of time the series
were sitting there.. I'm hesitating to apply it to -stable.
The whole issue, while real, smells like somewhat unusual case.

Any comments on this?

Thanks,

/mjt

Re: [PATCH v2 0/3] block: align CoR requests to subclusters

[Denis V. Lunev]

On 9/7/23 22:11, Michael Tokarev wrote:
> 11.07.2023 20:25, Andrey Drobyshev via wrote:
>> v1 --> v2:
>>   * Fixed line indentation;
>>   * Fixed wording in a comment;
>>   * Added R-b.
>>
>> v1: 
>> https://lists.nongnu.org/archive/html/qemu-block/2023-06/msg00606.html
>>
>> Andrey Drobyshev (3):
>>    block: add subcluster_size field to BlockDriverInfo
>>    block/io: align requests to subcluster_size
>>    tests/qemu-iotests/197: add testcase for CoR with subclusters
>>
>>   block.c                      |  7 +++++
>>   block/io.c                   | 50 ++++++++++++++++++------------------
>>   block/mirror.c               |  8 +++---
>>   block/qcow2.c                |  1 +
>>   include/block/block-common.h |  5 ++++
>>   include/block/block-io.h     |  8 +++---
>>   tests/qemu-iotests/197       | 29 +++++++++++++++++++++
>>   tests/qemu-iotests/197.out   | 24 +++++++++++++++++
>>   8 files changed, 99 insertions(+), 33 deletions(-)
>
> So, given the size of patch series and amount of time the series
> were sitting there.. I'm hesitating to apply it to -stable.
> The whole issue, while real, smells like somewhat unusual case.
>
> Any comments on this?
>
> Thanks,
>
> /mjt

The issue was observed by us in the reality on not yet
released product, but the series requires correct, which
has been sent as 4/3 patch today by Andrey. Unit tests
are broken at the moment.

Thanks,
     Den

[PATCH v2 4/3] qemu-iotests/197: use more generic commands for formats other than qcow2

[Andrey Drobyshev via]

In the previous commit e2f938265e0 ("tests/qemu-iotests/197: add
testcase for CoR with subclusters") we've introduced a new testcase for
copy-on-read with subclusters.  Test 197 always forces qcow2 as the top
image, but allows backing image to be in any format.  That last test
case didn't meet these requirements, so let's fix it by using more
generic "qemu-io -c map" command.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/197     |  8 ++++----
 tests/qemu-iotests/197.out | 18 ++++++++----------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
index f07a9da136..8ad2bdb035 100755
--- a/tests/qemu-iotests/197
+++ b/tests/qemu-iotests/197
@@ -136,18 +136,18 @@ IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
 $QEMU_IO -c "write -P 0xaa 0 64k" "$TEST_IMG" | _filter_qemu_io
 
 # Allocate individual subclusters in the top image, and not the whole cluster
-$QEMU_IO -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \
+$QEMU_IO -f qcow2 -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \
     | _filter_qemu_io
 
 # Only 2 subclusters should be allocated in the top image at this point
-$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
+$QEMU_IO -f qcow2 -c map "$TEST_WRAP"
 
 # Actual copy-on-read operation
-$QEMU_IO -C -c "read -P 0xaa 30K 4K" "$TEST_WRAP" | _filter_qemu_io
+$QEMU_IO -f qcow2 -C -c "read -P 0xaa 30K 4K" "$TEST_WRAP" | _filter_qemu_io
 
 # And here we should have 4 subclusters allocated right in the middle of the
 # top image. Make sure the whole cluster remains unallocated
-$QEMU_IMG map "$TEST_WRAP" | _filter_qemu_img_map
+$QEMU_IO -f qcow2 -c map "$TEST_WRAP"
 
 _check_test_img
 
diff --git a/tests/qemu-iotests/197.out b/tests/qemu-iotests/197.out
index 8f34a30afe..86c57b51d3 100644
--- a/tests/qemu-iotests/197.out
+++ b/tests/qemu-iotests/197.out
@@ -42,17 +42,15 @@ wrote 2048/2048 bytes at offset 28672
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 2048/2048 bytes at offset 34816
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Offset          Length          File
-0               0x7000          TEST_DIR/t.IMGFMT
-0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
-0x7800          0x1000          TEST_DIR/t.IMGFMT
-0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
-0x9000          0x7000          TEST_DIR/t.IMGFMT
+28 KiB (0x7000) bytes not allocated at offset 0 bytes (0x0)
+2 KiB (0x800) bytes     allocated at offset 28 KiB (0x7000)
+4 KiB (0x1000) bytes not allocated at offset 30 KiB (0x7800)
+2 KiB (0x800) bytes     allocated at offset 34 KiB (0x8800)
+28 KiB (0x7000) bytes not allocated at offset 36 KiB (0x9000)
 read 4096/4096 bytes at offset 30720
 4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-Offset          Length          File
-0               0x7000          TEST_DIR/t.IMGFMT
-0x7000          0x2000          TEST_DIR/t.wrap.IMGFMT
-0x9000          0x7000          TEST_DIR/t.IMGFMT
+28 KiB (0x7000) bytes not allocated at offset 0 bytes (0x0)
+8 KiB (0x2000) bytes     allocated at offset 28 KiB (0x7000)
+28 KiB (0x7000) bytes not allocated at offset 36 KiB (0x9000)
 No errors were found on the image.
 *** done
-- 
2.39.3

Re: [PATCH v2 4/3] qemu-iotests/197: use more generic commands for formats other than qcow2

[Eric Blake]

On Fri, Sep 08, 2023 at 01:07:18AM +0300, Andrey Drobyshev via wrote:
> In the previous commit e2f938265e0 ("tests/qemu-iotests/197: add
> testcase for CoR with subclusters") we've introduced a new testcase for
> copy-on-read with subclusters.  Test 197 always forces qcow2 as the top
> image, but allows backing image to be in any format.  That last test
> case didn't meet these requirements, so let's fix it by using more
> generic "qemu-io -c map" command.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  tests/qemu-iotests/197     |  8 ++++----
>  tests/qemu-iotests/197.out | 18 ++++++++----------
>  2 files changed, 12 insertions(+), 14 deletions(-)

Tested-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
> index f07a9da136..8ad2bdb035 100755
> --- a/tests/qemu-iotests/197
> +++ b/tests/qemu-iotests/197
> @@ -136,18 +136,18 @@ IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
>  $QEMU_IO -c "write -P 0xaa 0 64k" "$TEST_IMG" | _filter_qemu_io
>  
>  # Allocate individual subclusters in the top image, and not the whole cluster
> -$QEMU_IO -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \
> +$QEMU_IO -f qcow2 -c "write -P 0xbb 28K 2K" -c "write -P 0xcc 34K 2K" "$TEST_WRAP" \
>      | _filter_qemu_io

Adding the -f qcow2 makes sense (this is a test of subcluster
behavior); and the backing file remains whatever format was passed to
./check.

> +++ b/tests/qemu-iotests/197.out
> @@ -42,17 +42,15 @@ wrote 2048/2048 bytes at offset 28672
>  2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  wrote 2048/2048 bytes at offset 34816
>  2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -Offset          Length          File
> -0               0x7000          TEST_DIR/t.IMGFMT
> -0x7000          0x800           TEST_DIR/t.wrap.IMGFMT
> -0x7800          0x1000          TEST_DIR/t.IMGFMT
> -0x8800          0x800           TEST_DIR/t.wrap.IMGFMT
> -0x9000          0x7000          TEST_DIR/t.IMGFMT
> +28 KiB (0x7000) bytes not allocated at offset 0 bytes (0x0)
> +2 KiB (0x800) bytes     allocated at offset 28 KiB (0x7000)
> +4 KiB (0x1000) bytes not allocated at offset 30 KiB (0x7800)
> +2 KiB (0x800) bytes     allocated at offset 34 KiB (0x8800)
> +28 KiB (0x7000) bytes not allocated at offset 36 KiB (0x9000)
>  read 4096/4096 bytes at offset 30720

Same information, but without the backing file details (which clears
up the problem with -nbd).

Reviewed-by: Eric Blake <eblake@redhat.com>

Adding to my NBD queue, for a pull request soon.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org