All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] qcow2: Expose bitmaps' size during measure
@ 2020-04-16 21:23 Eric Blake
  2020-04-16 21:47 ` Eric Blake
  2020-04-16 22:49 ` no-reply
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Blake @ 2020-04-16 21:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Max Reitz, nsoffer, jsnow

It's useful to know how much space can be occupied by qcow2 persistent
bitmaps, even though such metadata is unrelated to the guest-visible
data.  Report this value as an additional field.

Reported-by: Nir Soffer <nsoffer@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---

This is independent from my 'qemu-img convert --bitmaps' series, but
highly related.  As an example, if I create a 100M image, then 2
persistent bitmaps, all with default cluster/granularity sizing, I now
see:

$ ./qemu-img measure -f qcow2 -O qcow2 build/img.top
required size: 52756480
fully allocated size: 105185280
bitmaps size: 327680

which argues that I should allocate 52756480 + 327680 bytes prior to
attempting qemu-img convert --bitmaps to a pre-sized destination.

If we like the idea, I probably need to submit a 2/1 patch adding
iotest coverage of the new measurement.

 qapi/block-core.json | 15 ++++++++++-----
 block/qcow2.c        | 25 +++++++++++++++++++++++++
 qemu-img.c           |  3 +++
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 943df1926a91..b47c6d69ba27 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -633,18 +633,23 @@
 # efficiently so file size may be smaller than virtual disk size.
 #
 # The values are upper bounds that are guaranteed to fit the new image file.
-# Subsequent modification, such as internal snapshot or bitmap creation, may
-# require additional space and is not covered here.
+# Subsequent modification, such as internal snapshot or further bitmap
+# creation, may require additional space and is not covered here.
 #
-# @required: Size required for a new image file, in bytes.
+# @required: Size required for a new image file, in bytes, when copying just
+#            guest-visible contents.
 #
 # @fully-allocated: Image file size, in bytes, once data has been written
-#                   to all sectors.
+#                   to all sectors, when copying just guest-visible contents.
+#
+# @bitmaps: Additional size required for bitmap metadata not directly used
+#           for guest contents, when that metadata can be copied in addition
+#           to guest contents. (since 5.1)
 #
 # Since: 2.10
 ##
 { 'struct': 'BlockMeasureInfo',
-  'data': {'required': 'int', 'fully-allocated': 'int'} }
+  'data': {'required': 'int', 'fully-allocated': 'int', '*bitmaps': 'int'} }

 ##
 # @query-block:
diff --git a/block/qcow2.c b/block/qcow2.c
index b524b0c53f84..eba6c2511e60 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4657,6 +4657,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
     PreallocMode prealloc;
     bool has_backing_file;
     bool has_luks;
+    uint64_t bitmaps_size = 0; /* size occupied by bitmaps in in_bs */

     /* Parse image creation options */
     cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
@@ -4732,6 +4733,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,

     /* Account for input image */
     if (in_bs) {
+        BdrvDirtyBitmap *bm;
+        size_t bitmap_overhead = 0;
         int64_t ssize = bdrv_getlength(in_bs);
         if (ssize < 0) {
             error_setg_errno(&local_err, -ssize,
@@ -4739,6 +4742,26 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
             goto err;
         }

+        FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
+            if (bdrv_dirty_bitmap_get_persistence(bm)) {
+                uint64_t bmsize = bdrv_dirty_bitmap_size(bm);
+                uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
+                const char *name = bdrv_dirty_bitmap_name(bm);
+                uint64_t bmclusters = DIV_ROUND_UP(bmsize / granularity
+                                                   / CHAR_BIT, cluster_size);
+
+                /* Assume the entire bitmap is allocated */
+                bitmaps_size += bmclusters * cluster_size;
+                /* Also reserve space for the bitmap table entries */
+                bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
+                                         cluster_size);
+                /* Guess at contribution to bitmap directory size */
+                bitmap_overhead += ROUND_UP(strlen(name) + 24,
+                                            sizeof(uint64_t));
+            }
+        }
+        bitmaps_size += ROUND_UP(bitmap_overhead, cluster_size);
+
         virtual_size = ROUND_UP(ssize, cluster_size);

         if (has_backing_file) {
@@ -4795,6 +4818,8 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
      * still counted.
      */
     info->required = info->fully_allocated - virtual_size + required;
+    info->has_bitmaps = !!bitmaps_size;
+    info->bitmaps = bitmaps_size;
     return info;

 err:
diff --git a/qemu-img.c b/qemu-img.c
index 6541357179c2..d900bde89911 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5084,6 +5084,9 @@ static int img_measure(int argc, char **argv)
     if (output_format == OFORMAT_HUMAN) {
         printf("required size: %" PRIu64 "\n", info->required);
         printf("fully allocated size: %" PRIu64 "\n", info->fully_allocated);
+        if (info->has_bitmaps) {
+            printf("bitmaps size: %" PRIu64 "\n", info->bitmaps);
+        }
     } else {
         dump_json_block_measure_info(info);
     }
-- 
2.26.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] qcow2: Expose bitmaps' size during measure
  2020-04-16 21:23 [PATCH] qcow2: Expose bitmaps' size during measure Eric Blake
@ 2020-04-16 21:47 ` Eric Blake
  2020-04-16 22:49 ` no-reply
  1 sibling, 0 replies; 5+ messages in thread
From: Eric Blake @ 2020-04-16 21:47 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Max Reitz, nsoffer, jsnow

On 4/16/20 4:23 PM, Eric Blake wrote:
> It's useful to know how much space can be occupied by qcow2 persistent
> bitmaps, even though such metadata is unrelated to the guest-visible
> data.  Report this value as an additional field.
> 
> Reported-by: Nir Soffer <nsoffer@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 

Per https://bugzilla.redhat.com/show_bug.cgi?id=1779904#c0, I didn't 
quite round up in enough places:

> @@ -4739,6 +4742,26 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts *opts, BlockDriverState *in_bs,
>               goto err;
>           }
> 
> +        FOR_EACH_DIRTY_BITMAP(in_bs, bm) {
> +            if (bdrv_dirty_bitmap_get_persistence(bm)) {
> +                uint64_t bmsize = bdrv_dirty_bitmap_size(bm);
> +                uint32_t granularity = bdrv_dirty_bitmap_granularity(bm);
> +                const char *name = bdrv_dirty_bitmap_name(bm);
> +                uint64_t bmclusters = DIV_ROUND_UP(bmsize / granularity
> +                                                   / CHAR_BIT, cluster_size);

All of these divisions need to round up.  For example, in an image with 
512-byte clusters and granularity, and a bitmap covering 512*512*8+512 
bytes (2097664), we need 2 clusters, not 1, for the bitmap itself. 
Fortunately, it is an edge case, and we usually have enough slop in the 
final round up to cluster size that most users won't trip on this.

> +
> +                /* Assume the entire bitmap is allocated */
> +                bitmaps_size += bmclusters * cluster_size;
> +                /* Also reserve space for the bitmap table entries */
> +                bitmaps_size += ROUND_UP(bmclusters * sizeof(uint64_t),
> +                                         cluster_size);
> +                /* Guess at contribution to bitmap directory size */
> +                bitmap_overhead += ROUND_UP(strlen(name) + 24,

And I don't like this magic number, but sizeof(Qcow2BitmapDirEntry) from 
qcow2-bitmap.c is a private struct not accessible here.

> +                                            sizeof(uint64_t));
> +            }
> +        }
> +        bitmaps_size += ROUND_UP(bitmap_overhead, cluster_size);
> +
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] qcow2: Expose bitmaps' size during measure
  2020-04-16 21:23 [PATCH] qcow2: Expose bitmaps' size during measure Eric Blake
  2020-04-16 21:47 ` Eric Blake
@ 2020-04-16 22:49 ` no-reply
  2020-04-17  0:17   ` Eric Blake
  1 sibling, 1 reply; 5+ messages in thread
From: no-reply @ 2020-04-16 22:49 UTC (permalink / raw)
  To: eblake; +Cc: kwolf, qemu-block, armbru, qemu-devel, mreitz, nsoffer, jsnow

Patchew URL: https://patchew.org/QEMU/20200416212349.731404-1-eblake@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Not run: 259
Failures: 190
Failed 1 of 117 iotests
make: *** [check-tests/check-block.sh] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-aarch64: tests/qtest/qos-test
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=fab7467314384d429b3d07d0ac891780', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-o6sxcgfh/src/docker-src.2020-04-16-18.34.06.7222:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=fab7467314384d429b3d07d0ac891780
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-o6sxcgfh/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    15m8.073s
user    0m8.781s


The full log is available at
http://patchew.org/logs/20200416212349.731404-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] qcow2: Expose bitmaps' size during measure
  2020-04-16 22:49 ` no-reply
@ 2020-04-17  0:17   ` Eric Blake
  2020-04-17 13:20     ` Eric Blake
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Blake @ 2020-04-17  0:17 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-block, armbru, mreitz, nsoffer, jsnow

On 4/16/20 5:49 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200416212349.731404-1-eblake@redhat.com/
> 
> 
> 
> Hi,
> 
> This series failed the docker-quick@centos7 build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-centos7 V=1 NETWORK=1
> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
> === TEST SCRIPT END ===
> 
> Not run: 259
> Failures: 190
> Failed 1 of 117 iotests

Hmm - the email truncated the useful part of the failure.  Anyways, 
reading from...

> The full log is available at
> http://patchew.org/logs/20200416212349.731404-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message.

I see:

--- /tmp/qemu-test/src/tests/qemu-iotests/190.out	2020-04-16 
21:15:51.000000000 +0000
+++ /tmp/qemu-test/build/tests/qemu-iotests/190.out.bad	2020-04-16 
22:45:47.504493172 +0000
@@ -4,6 +4,7 @@
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
  required size: 2199023255552
  fully allocated size: 2199023255552
+bitmaps size: 4846791580151137091
  required size: 335806464

which looks suspiciously like an uninitialized variable leaking through 
when there are no bitmaps to be measured.  I'll fix it in v2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] qcow2: Expose bitmaps' size during measure
  2020-04-17  0:17   ` Eric Blake
@ 2020-04-17 13:20     ` Eric Blake
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Blake @ 2020-04-17 13:20 UTC (permalink / raw)
  To: qemu-devel; +Cc: kwolf, qemu-block, armbru, mreitz, nsoffer, jsnow

On 4/16/20 7:17 PM, Eric Blake wrote:

>> The full log is available at
>> http://patchew.org/logs/20200416212349.731404-1-eblake@redhat.com/testing.docker-quick@centos7/?type=message. 
>>
> 
> I see:
> 
> --- /tmp/qemu-test/src/tests/qemu-iotests/190.out    2020-04-16 
> 21:15:51.000000000 +0000
> +++ /tmp/qemu-test/build/tests/qemu-iotests/190.out.bad    2020-04-16 
> 22:45:47.504493172 +0000
> @@ -4,6 +4,7 @@
>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2199023255552
>   required size: 2199023255552
>   fully allocated size: 2199023255552
> +bitmaps size: 4846791580151137091
>   required size: 335806464
> 
> which looks suspiciously like an uninitialized variable leaking through 
> when there are no bitmaps to be measured.  I'll fix it in v2.

Here's what I'm squashing in:

diff --git i/block/crypto.c w/block/crypto.c
index d577f89659fa..4e0f3ec97f0e 100644
--- i/block/crypto.c
+++ w/block/crypto.c
@@ -535,7 +535,7 @@ static BlockMeasureInfo 
*block_crypto_measure(QemuOpts *opts,
       * Unallocated blocks are still encrypted so allocation status 
makes no
       * difference to the file size.
       */
-    info = g_new(BlockMeasureInfo, 1);
+    info = g_new0(BlockMeasureInfo, 1);
      info->fully_allocated = luks_payload_size + size;
      info->required = luks_payload_size + size;
      return info;
diff --git i/block/qcow2.c w/block/qcow2.c
index eba6c2511e60..8d7a9e87fba0 100644
--- i/block/qcow2.c
+++ w/block/qcow2.c
@@ -4808,7 +4808,7 @@ static BlockMeasureInfo *qcow2_measure(QemuOpts 
*opts, BlockDriverState *in_bs,
          required = virtual_size;
      }

-    info = g_new(BlockMeasureInfo, 1);
+    info = g_new0(BlockMeasureInfo, 1);
      info->fully_allocated =
          qcow2_calc_prealloc_size(virtual_size, cluster_size,
                                   ctz32(refcount_bits)) + 
luks_payload_size;
diff --git i/block/raw-format.c w/block/raw-format.c
index 93b25e1b6b0b..4bb54f4ac6c5 100644
--- i/block/raw-format.c
+++ w/block/raw-format.c
@@ -346,7 +346,7 @@ static BlockMeasureInfo *raw_measure(QemuOpts *opts, 
BlockDriverState *in_bs,
                              BDRV_SECTOR_SIZE);
      }

-    info = g_new(BlockMeasureInfo, 1);
+    info = g_new0(BlockMeasureInfo, 1);
      info->required = required;

      /* Unallocated sectors count towards the file size in raw images */


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-17 13:21 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-16 21:23 [PATCH] qcow2: Expose bitmaps' size during measure Eric Blake
2020-04-16 21:47 ` Eric Blake
2020-04-16 22:49 ` no-reply
2020-04-17  0:17   ` Eric Blake
2020-04-17 13:20     ` Eric Blake

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.