[PATCH 0/3] Add a for_commit option to qemu-img measure

[PATCH 0/3] Add a for_commit option to qemu-img measure

[Jean-Louis Dupond]

This fixed bug #2369 [1]

This patch is a new implementation for a previous (non-merged) patch to make measure useful to calculate
the new size of the target image after commit.
It is very useful to have this kind of calculation if you do qcow2 on block storage (like iSCSi).
This because you need to be able to size the target volume big enough to handle the committed image.

Instead of modifying the existing measure logic, a new logic was added that really looks into the allocated
clusters and use the (local) next_cluster_index to calculate the target image size.
It also takes into account discards from the top into base to lower the index when a cluster is freed.

New test was added to verify the code.

When testing the new code on the impacted images in bug #2369 [1], this gives the following results:

Start:
<filesize>  <image>
 6174015488 top
13421772800 base

calculate with discard-no-unref=on:
required -> 13624475648
Commited image size: 13610385408

When calculating with discard-no-unref=off:
required -> 13520404480
Commited image size: 13504806912

Let me know if I made some mistakes :)

[1]: https://gitlab.com/qemu-project/qemu/-/issues/2369

Jean-Louis Dupond (3):
  block: add for_commit option to measure
  qcow2: make measure for_commit aware
  iotests/290: add test case for for_commit measure

 block/qcow2.c                    | 137 +++++++++++++++++++++++++++++--
 include/block/block_int-common.h |   4 +
 qapi/block-core.json             |  28 +++++++
 qemu-img.c                       |  40 +++++++--
 tests/qemu-iotests/290           |  45 ++++++++++
 tests/qemu-iotests/290.out       |  61 ++++++++++++++
 6 files changed, 304 insertions(+), 11 deletions(-)

-- 
2.49.0

[PATCH 1/3] block: add for_commit option to measure

[Jean-Louis Dupond]

To specify we use measure call for commit size calculations, we add a
new 'for_commit' option to the measure call.
This will be used in following commit to do a different measurement.

Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
 block/qcow2.c                    | 16 +++++++++++++
 include/block/block_int-common.h |  4 ++++
 qapi/block-core.json             | 28 ++++++++++++++++++++++
 qemu-img.c                       | 40 ++++++++++++++++++++++++++++----
 4 files changed, 83 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7774e7f090..19028e051c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3945,6 +3945,7 @@ qcow2_co_create_opts(BlockDriver *drv, const char *filename, QemuOpts *opts,
         { BLOCK_OPT_COMPAT_LEVEL,       "version" },
         { BLOCK_OPT_DATA_FILE_RAW,      "data-file-raw" },
         { BLOCK_OPT_COMPRESSION_TYPE,   "compression-type" },
+        { BLOCK_OPT_FOR_COMMIT,         "for-commit" },
         { NULL, NULL },
     };
 
@@ -6066,6 +6067,20 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
         .def_value_str = "16"                                       \
     }
 
+static QemuOptsList qcow2_measure_opts = {
+    .name = "qcow2-measure-opts",
+    .head = QTAILQ_HEAD_INITIALIZER(qcow2_measure_opts.head),
+    .desc = {
+        {                                                       \
+            .name = BLOCK_OPT_FOR_COMMIT,                       \
+            .type = QEMU_OPT_BOOL,                              \
+            .help = "Use measure for commit",                   \
+            .def_value_str = "off"                              \
+        },                                                      \
+        { /* end of list */ }
+    }
+};
+
 static QemuOptsList qcow2_create_opts = {
     .name = "qcow2-create-opts",
     .head = QTAILQ_HEAD_INITIALIZER(qcow2_create_opts.head),
@@ -6190,6 +6205,7 @@ BlockDriver bdrv_qcow2 = {
 
     .create_opts                        = &qcow2_create_opts,
     .amend_opts                         = &qcow2_amend_opts,
+    .measure_opts                       = &qcow2_measure_opts,
     .strong_runtime_opts                = qcow2_strong_runtime_opts,
     .mutable_opts                       = mutable_opts,
     .bdrv_co_check                      = qcow2_co_check,
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index ebb4e56a50..26d521459d 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -57,6 +57,7 @@
 #define BLOCK_OPT_DATA_FILE_RAW     "data_file_raw"
 #define BLOCK_OPT_COMPRESSION_TYPE  "compression_type"
 #define BLOCK_OPT_EXTL2             "extended_l2"
+#define BLOCK_OPT_FOR_COMMIT        "for_commit"
 
 #define BLOCK_PROBE_BUF_SIZE        512
 
@@ -177,6 +178,9 @@ struct BlockDriver {
     /* List of options for image amend */
     QemuOptsList *amend_opts;
 
+    /* List of options for image measure */
+    QemuOptsList *measure_opts;
+
     /*
      * If this driver supports reopening images this contains a
      * NULL-terminated list of the runtime options that can be
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b1937780e1..ab897be404 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -5557,6 +5557,34 @@
   'features': [ 'unstable' ],
   'allow-preconfig': true }
 
+  ##
+  # @BlockdevMeasureOptionsQcow2:
+  #
+  # Driver specific image measure options for qcow2.
+  #
+  # @for-commit: Use the measure command to calculate commit image size
+  #
+  # Since: 10.0
+  ##
+  { 'struct': 'BlockdevMeasureOptionsQcow2',
+    'data': { '*for-commit': 'bool' } }
+
+  ##
+  # @BlockdevMeasureOptions:
+  #
+  # Options for measuring an image format
+  #
+  # @driver: Block driver of the node to measure.
+  #
+  # Since: 10.0
+  ##
+  { 'union': 'BlockdevMeasureOptions',
+    'base': {
+        'driver':         'BlockdevDriver' },
+    'discriminator': 'driver',
+    'data': {
+        'qcow2':           'BlockdevMeasureOptionsQcow2' } }
+
 ##
 # @BlockErrorAction:
 #
diff --git a/qemu-img.c b/qemu-img.c
index 2044c22a4c..a4673c3f32 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5327,6 +5327,31 @@ out:
     return 0;
 }
 
+static int print_measure_option_help(const char *format)
+{
+    BlockDriver *drv;
+
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+    /* Find driver and parse its options */
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        error_report("Unknown file format '%s'", format);
+        return 1;
+    }
+
+    if (!drv->measure_opts) {
+        error_report("Format driver '%s' does not support measure options",
+            format);
+        return 1;
+    }
+
+    printf("Measure options for '%s':\n", format);
+    qemu_opts_print_help(drv->measure_opts, false);
+
+    return 0;
+}
+
 static void dump_json_block_measure_info(BlockMeasureInfo *info)
 {
     GString *str;
@@ -5366,7 +5391,7 @@ static int img_measure(int argc, char **argv)
     QemuOpts *opts = NULL;
     QemuOpts *object_opts = NULL;
     QemuOpts *sn_opts = NULL;
-    QemuOptsList *create_opts = NULL;
+    QemuOptsList *all_opts = NULL;
     bool image_opts = false;
     uint64_t img_size = UINT64_MAX;
     BlockMeasureInfo *info = NULL;
@@ -5491,10 +5516,15 @@ static int img_measure(int argc, char **argv)
                      drv->format_name);
         goto out;
     }
+    if (options && has_help_option(options)) {
+        ret = print_measure_option_help(drv->format_name);
+        goto out;
+    }
 
-    create_opts = qemu_opts_append(create_opts, drv->create_opts);
-    create_opts = qemu_opts_append(create_opts, bdrv_file.create_opts);
-    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
+    all_opts = qemu_opts_append(all_opts, drv->create_opts);
+    all_opts = qemu_opts_append(all_opts, bdrv_file.create_opts);
+    all_opts = qemu_opts_append(all_opts, drv->measure_opts);
+    opts = qemu_opts_create(all_opts, NULL, 0, &error_abort);
     if (options) {
         if (!qemu_opts_do_parse(opts, options, NULL, &local_err)) {
             error_report_err(local_err);
@@ -5529,7 +5559,7 @@ out:
     qemu_opts_del(object_opts);
     qemu_opts_del(opts);
     qemu_opts_del(sn_opts);
-    qemu_opts_free(create_opts);
+    qemu_opts_free(all_opts);
     g_free(options);
     blk_unref(in_blk);
     return ret;
-- 
2.49.0

[PATCH 2/3] qcow2: make measure for_commit aware

[Jean-Louis Dupond]

Sometimes (for example when using block storage for qcow2 images), we
want to be able to calculate the size the commit target image will have.

This patch implements this functionality in the qemu-img command when
the 'for_commit' option is passed.

When calculating for_commit, we check the blocks of the top and base
image, and if new blocks are needed, we increment the next_cluster_index
until everything is allocated for all blocks in the top image.
Then we have a new cluster_index, from where we can calculate the size
of the target image after commit.

Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
 block/qcow2.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 19028e051c..f86d4f1673 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5086,6 +5086,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
     bool has_backing_file;
     bool has_luks;
     bool extended_l2;
+    bool for_commit;
     size_t l2e_size;
 
     /* Parse image creation options */
@@ -5157,6 +5158,9 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
         goto err;
     }
 
+    /* Check if this measure is for commit size calculation */
+    for_commit = qemu_opt_get_bool_del(opts, BLOCK_OPT_FOR_COMMIT, false);
+
     /* Account for input image */
     if (in_bs) {
         int64_t ssize = bdrv_getlength(in_bs);
@@ -5178,6 +5182,28 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
         } else {
             int64_t offset;
             int64_t pnum = 0;
+            BlockDriverState *parent = NULL;
+            BDRVQcow2State *sp = NULL;
+            int64_t next_cluster_index = 0;
+            int64_t last_cluster_index = 0;
+            int64_t max_allocated_clusters = 0;
+            int64_t freed_clusters = 0;
+
+            if (for_commit) {
+                int64_t psize;
+
+                parent = bdrv_filter_or_cow_bs(in_bs);
+                if (parent) {
+                    sp = parent->opaque;
+                } else {
+                    error_setg(&local_err,
+                        "No parent found, cannot measure for commit");
+                    goto err;
+                }
+                psize = bdrv_getlength(parent);
+                last_cluster_index = qcow2_get_last_cluster(parent, psize);
+                max_allocated_clusters = last_cluster_index;
+            }
 
             for (offset = 0; offset < ssize; offset += pnum) {
                 int ret;
@@ -5191,17 +5217,100 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
                     goto err;
                 }
 
-                if (ret & BDRV_BLOCK_ZERO) {
-                    /* Skip zero regions (safe with no backing file) */
-                } else if ((ret & (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
-                           (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) {
+                /*
+                 * If this is a measure for_commit then we have a parent
+                 * We check the allocation status of the parent blocks to see
+                 * if we need to allocate new blocks or not.
+                 * We also keep track of the number of freed clusters.
+                 */
+                if (for_commit) {
+                    int retp;
+                    int64_t pnum_parent = 0;
+
+                    /* Check if the parent block is allocated */
+                    retp = bdrv_block_status_above(parent, NULL, offset,
+                                            ssize - offset, &pnum_parent, NULL,
+                                            NULL);
+
+                    if (retp < 0) {
+                        error_setg_errno(&local_err, -ret,
+                                            "Unable to get block status for parent");
+                        goto err;
+                    }
+                    /*
+                     * If the parent continuous block is smaller, use that pnum,
+                     * so the next iteration starts with the smallest offset.
+                     */
+                    if (pnum_parent < pnum) {
+                        pnum = pnum_parent;
+                    }
+
                     /* Extend pnum to end of cluster for next iteration */
                     pnum = ROUND_UP(offset + pnum, cluster_size) - offset;
 
-                    /* Count clusters we've seen */
-                    required += offset % cluster_size + pnum;
+                    uint64_t nb_clusters = size_to_clusters(sp, pnum);
+
+                    /*
+                     * When the block has no offset and the new
+                     * block is non-zero, we will need to
+                     * allocate a new cluster for the commit.
+                     */
+                    if (~retp & BDRV_BLOCK_OFFSET_VALID &&
+                        ~ret & BDRV_BLOCK_ZERO) {
+                        uint64_t i, refcount = 0;
+
+                    retry:
+                        for (i = 0; i < nb_clusters; i++) {
+                            int retr;
+                            next_cluster_index++;
+
+                            retr = qcow2_get_refcount(parent,
+                                next_cluster_index, &refcount);
+                            if (retr < 0) {
+                                error_setg_errno(&local_err, -retr,
+                                    "Unable to get refcount");
+                                goto err;
+                            }
+                            /* No free block found, retry */
+                            if (refcount != 0) {
+                                goto retry;
+                            }
+                        }
+                        /* Check if we have a new maximum cluster index */
+                        if ((next_cluster_index - freed_clusters) >
+                            last_cluster_index &&
+                            (next_cluster_index - freed_clusters) >
+                            max_allocated_clusters) {
+                            max_allocated_clusters =
+                                next_cluster_index - freed_clusters;
+                        }
+                    } else if (!sp->discard_no_unref &&
+                               (ret & BDRV_BLOCK_ZERO) &&
+                               (retp & BDRV_BLOCK_DATA)) {
+                        /*
+                         * Parent block is allocated but new block is zero
+                         * we can free. Except if the parent block is zero.
+                         */
+                        freed_clusters += nb_clusters;
+                    }
+                } else {
+                    if (ret & BDRV_BLOCK_ZERO) {
+                        /* Skip zero regions (safe with no backing file) */
+                    } else if (((ret &
+                                 (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED)) ==
+                                (BDRV_BLOCK_DATA | BDRV_BLOCK_ALLOCATED))) {
+                        /* Extend pnum to end of cluster for next iteration */
+                        pnum = ROUND_UP(offset + pnum, cluster_size) - offset;
+
+                        /* Count clusters we've seen */
+                        required += offset % cluster_size + pnum;
+                    }
                 }
             }
+            if (for_commit) {
+                /* Then the required size is just until the last cluster */
+                required = max_allocated_clusters << sp->cluster_bits;
+            }
         }
     }
 
-- 
2.49.0

[PATCH 3/3] iotests/290: add test case for for_commit measure

[Jean-Louis Dupond]

We create an image, write and discard some data in it, and then create a
snapshot. In the snapshot we write and discard again some data.
Then we measure the images with 'for_commit' option to calculate the
merged image size. Finally we commit the image and check it's size.

This scenario is executed for discard-no-unref enabled and disabled.

Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
 tests/qemu-iotests/290     | 45 ++++++++++++++++++++++++++++
 tests/qemu-iotests/290.out | 61 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
index 776b59de1b..1d6608ad13 100755
--- a/tests/qemu-iotests/290
+++ b/tests/qemu-iotests/290
@@ -92,6 +92,51 @@ for qcow2_compat in 0.10 1.1; do
     $QEMU_IMG map "$TEST_IMG" | _filter_testdir
 done
 
+echo
+echo "### Test qemu-img measure for commit differences with 'discard-no-unref' option enabled"
+echo
+
+for DISCARD_NO_UNREF in true false; do
+    echo "# Create a base image and fill it with data"
+    TEST_IMG="$TEST_IMG.base" _make_test_img 128M
+
+    $QEMU_IO -c 'write 0 8M' "$TEST_IMG.base" | _filter_qemu_io
+    $QEMU_IO -c 'write 10M 8M' "$TEST_IMG.base" | _filter_qemu_io
+    $QEMU_IO -c 'write 24M 32M' "$TEST_IMG.base" | _filter_qemu_io
+    $QEMU_IO -c 'write 56M 20M' "$TEST_IMG.base" | _filter_qemu_io
+    $QEMU_IO -c "reopen -o discard=unmap,discard-no-unref=$DISCARD_NO_UNREF" \
+        -c 'discard 32M 10M' "$TEST_IMG.base" | _filter_qemu_io
+
+    echo "# Create a top image and do some writes and discards"
+    TEST_IMG="$TEST_IMG.top" _make_test_img -b "$TEST_IMG.base" -F $IMGFMT 128M
+
+    $QEMU_IO -c "reopen -o discard=unmap,discard-no-unref=$DISCARD_NO_UNREF" -c 'write 16M 8M' \
+        -c 'discard 60M 20M' -c 'write 84M 10M' "$TEST_IMG.top" | _filter_qemu_io
+
+    FILE_JSON="json:{
+        'file': {
+            'driver': 'file',
+            'filename': '$TEST_IMG.top'
+        },
+        'driver': 'qcow2',
+        'discard': 'unmap',
+        'discard-no-unref': '$DISCARD_NO_UNREF',
+        'backing': {
+            'driver': 'qcow2',
+            'discard-no-unref': '$DISCARD_NO_UNREF',
+            'file': {
+                'driver': 'file',
+                'filename': '$TEST_IMG.base'
+            },
+            'backing': null
+        }}"
+    echo "# Measure size with discard-no-unref=$DISCARD_NO_UNREF"
+    $QEMU_IMG measure --output=json -ofor_commit=on -O qcow2 "${FILE_JSON}"
+    echo "# Merging the top image into the base image"
+    $QEMU_IMG commit -t none -f qcow2 "${FILE_JSON}"
+    stat -c"base disk image file size in bytes: %s" "$TEST_IMG.base"
+done
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/290.out b/tests/qemu-iotests/290.out
index 22b476594f..4207572118 100644
--- a/tests/qemu-iotests/290.out
+++ b/tests/qemu-iotests/290.out
@@ -58,4 +58,65 @@ read 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 # Output of qemu-img map
 Offset          Length          Mapped to       File
+
+### Test qemu-img measure for commit differences with 'discard-no-unref' option enabled
+
+# Create a base image and fill it with data
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
+wrote 8388608/8388608 bytes at offset 0
+8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 8388608/8388608 bytes at offset 10485760
+8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 33554432/33554432 bytes at offset 25165824
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 20971520/20971520 bytes at offset 58720256
+20 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 10485760/10485760 bytes at offset 33554432
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Create a top image and do some writes and discards
+Formatting 'TEST_DIR/t.IMGFMT.top', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 8388608/8388608 bytes at offset 16777216
+8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 20971520/20971520 bytes at offset 62914560
+20 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 88080384
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Measure size with discard-no-unref=true
+{
+    "bitmaps": 0,
+    "required": 88670208,
+    "fully-allocated": 134545408
+}
+# Merging the top image into the base image
+Image committed.
+base disk image file size in bytes: 88408064
+# Create a base image and fill it with data
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728
+wrote 8388608/8388608 bytes at offset 0
+8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 8388608/8388608 bytes at offset 10485760
+8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 33554432/33554432 bytes at offset 25165824
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 20971520/20971520 bytes at offset 58720256
+20 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 10485760/10485760 bytes at offset 33554432
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Create a top image and do some writes and discards
+Formatting 'TEST_DIR/t.IMGFMT.top', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
+wrote 8388608/8388608 bytes at offset 16777216
+8 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 20971520/20971520 bytes at offset 62914560
+20 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 10485760/10485760 bytes at offset 88080384
+10 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Measure size with discard-no-unref=false
+{
+    "bitmaps": 0,
+    "required": 71892992,
+    "fully-allocated": 134545408
+}
+# Merging the top image into the base image
+Image committed.
+base disk image file size in bytes: 71630848
 *** done
-- 
2.49.0

Re: [PATCH 1/3] block: add for_commit option to measure

[Hanna Czenczek]

On 16.04.25 09:16, Jean-Louis Dupond wrote:
> To specify we use measure call for commit size calculations, we add a
> new 'for_commit' option to the measure call.
> This will be used in following commit to do a different measurement.

Why not allow specifying the node name (or filename) of the commit 
target instead of this just being a boolean?

(That was my main problem with the original series, that it wasn’t 
possible to specify the commit target (in an obvious way at least). I’m 
not a fan of deferring to JSON parameters for this, although I’m aware 
that to specify the node name of the commit target you would have to use 
JSON again, unless qemu-img can do some translation on behalf of the 
user: Looking at `commit -b`, that takes a filename; it could make sense 
to have the internal QAPI parameter use a node name, and qemu-img 
translating the filename parameter into a node name.  Actually, 
`measure` could just accept the same `-b` option as `commit`, which it 
would then translate into the QAPI `for-commit=<node-name>` option.)

Hanna

> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
>   block/qcow2.c                    | 16 +++++++++++++
>   include/block/block_int-common.h |  4 ++++
>   qapi/block-core.json             | 28 ++++++++++++++++++++++
>   qemu-img.c                       | 40 ++++++++++++++++++++++++++++----
>   4 files changed, 83 insertions(+), 5 deletions(-)

Re: [PATCH 1/3] block: add for_commit option to measure

[Jean-Louis Dupond]

On 5/12/25 13:16, Hanna Czenczek wrote:
> On 16.04.25 09:16, Jean-Louis Dupond wrote:
>> To specify we use measure call for commit size calculations, we add a
>> new 'for_commit' option to the measure call.
>> This will be used in following commit to do a different measurement.
>
> Why not allow specifying the node name (or filename) of the commit 
> target instead of this just being a boolean?
Honestly didn't even thought about that option :)
>
> (That was my main problem with the original series, that it wasn’t 
> possible to specify the commit target (in an obvious way at least). 
> I’m not a fan of deferring to JSON parameters for this, although I’m 
> aware that to specify the node name of the commit target you would 
> have to use JSON again, unless qemu-img can do some translation on 
> behalf of the user: Looking at `commit -b`, that takes a filename; it 
> could make sense to have the internal QAPI parameter use a node name, 
> and qemu-img translating the filename parameter into a node name.  
> Actually, `measure` could just accept the same `-b` option as 
> `commit`, which it would then translate into the QAPI 
> `for-commit=<node-name>` option.)
And you will need to use JSON parameters if you want to specify 
something special, like for example to note that the commit target has 
discard-no-unref enabled.
I think not many people will call the command with the for_commit 
option, but it will be (always) called from code where the JSON 
parameters give much more flexibility to add some additional options 
(like discard-no-unref).

We could of course support both (JSON + for-commit=<node-name>).
But then we will also need to validate somehow that the for-commit 
node-name is equal to the target specified in the JSON somehow.
Or have something like for-commit=true/false + json or 
for-commit-node=<node-name> if you don't use a JSON?

Do you think it's worth the effort ? :)
>
> Hanna
>
>
Thanks for the review!
Jean-Louis
>
>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>> ---
>>   block/qcow2.c                    | 16 +++++++++++++
>>   include/block/block_int-common.h |  4 ++++
>>   qapi/block-core.json             | 28 ++++++++++++++++++++++
>>   qemu-img.c                       | 40 ++++++++++++++++++++++++++++----
>>   4 files changed, 83 insertions(+), 5 deletions(-)
>

Re: [PATCH 1/3] block: add for_commit option to measure

[Jean-Louis Dupond]

On 13/05/2025 16:10, Jean-Louis Dupond wrote:
>
> On 5/12/25 13:16, Hanna Czenczek wrote:
>> On 16.04.25 09:16, Jean-Louis Dupond wrote:
>>> To specify we use measure call for commit size calculations, we add a
>>> new 'for_commit' option to the measure call.
>>> This will be used in following commit to do a different measurement.
>>
>> Why not allow specifying the node name (or filename) of the commit 
>> target instead of this just being a boolean?
> Honestly didn't even thought about that option :)
>>
>> (That was my main problem with the original series, that it wasn’t 
>> possible to specify the commit target (in an obvious way at least). 
>> I’m not a fan of deferring to JSON parameters for this, although I’m 
>> aware that to specify the node name of the commit target you would 
>> have to use JSON again, unless qemu-img can do some translation on 
>> behalf of the user: Looking at `commit -b`, that takes a filename; it 
>> could make sense to have the internal QAPI parameter use a node name, 
>> and qemu-img translating the filename parameter into a node name.  
>> Actually, `measure` could just accept the same `-b` option as 
>> `commit`, which it would then translate into the QAPI 
>> `for-commit=<node-name>` option.)
> And you will need to use JSON parameters if you want to specify 
> something special, like for example to note that the commit target has 
> discard-no-unref enabled.
> I think not many people will call the command with the for_commit 
> option, but it will be (always) called from code where the JSON 
> parameters give much more flexibility to add some additional options 
> (like discard-no-unref).
>
> We could of course support both (JSON + for-commit=<node-name>).
> But then we will also need to validate somehow that the for-commit 
> node-name is equal to the target specified in the JSON somehow.
> Or have something like for-commit=true/false + json or 
> for-commit-node=<node-name> if you don't use a JSON?
>
> Do you think it's worth the effort ? :)
Kind ping, no response on it since then.

Thanks
Jean-Louis
>>
>> Hanna
>>
>>
> Thanks for the review!
> Jean-Louis
>>
>>> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
>>> ---
>>>   block/qcow2.c                    | 16 +++++++++++++
>>>   include/block/block_int-common.h |  4 ++++
>>>   qapi/block-core.json             | 28 ++++++++++++++++++++++
>>>   qemu-img.c                       | 40 
>>> ++++++++++++++++++++++++++++----
>>>   4 files changed, 83 insertions(+), 5 deletions(-)
>>