minor qcow2 compression improvements

Match our code to the spec change in the previous patch - there's no reason for the refcount table to allow larger offsets than the L1/L2 tables.

In practice, no image has more than 64PB of allocated clusters anyways, as anything beyond that can't be expressed via L2 mappings to host offsets.

Suggested-by Alberto Garcia <[email protected]>

Signed-off-by Eric Blake <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

---
v4: new patch

---
 block/qcow2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/qcow2.h +1 -1

index 01b5250415f..3774229ef9c 100644
@@ -439,7 +439,7 @@ typedef enum QCow2MetadataOverlap {
 #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
 #define L2E_COMPRESSED_OFFSET_SIZE_MASK 0x3fffffffffffffffULL
 
-#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
+#define REFT_OFFSET_MASK 0x00fffffffffffe00ULL
 
 static inline int64_t start_of_cluster(BDRVQcow2State *s, int64_t offset)
 {

Signature · 1 line

2.14.4

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 3 lines

Match our code to the spec change in the previous patch - there's no reason for the refcount table to allow larger offsets than the L1/L2 tables.

What about internal snapshots? And anyway, because of the metadata overhead, the physical image size of a fully allocated image is always going to be at least minimally larger than the virtual disk size.

I'm not necessarily opposed to making the change if there is a good reason to make it, but I don't see a real need for it and the justification used here and also in the previous patch is incorrect.

Kevin

Quoted 3 lines

In practice, no image has more than 64PB of allocated clusters anyways, as anything beyond that can't be expressed via L2 mappings to host offsets.

Quoted 19 lines

Suggested-by Alberto Garcia <[email protected]>

Signed-off-by Eric Blake <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

+++ b/block/qcow2.h

block/qcow2.h +1 -1

@@ -439,7 +439,7 @@ typedef enum QCow2MetadataOverlap {
  #define L2E_OFFSET_MASK 0x00fffffffffffe00ULL
  #define L2E_COMPRESSED_OFFSET_SIZE_MASK 0x3fffffffffffffffULL
 
-#define REFT_OFFSET_MASK 0xfffffffffffffe00ULL
+#define REFT_OFFSET_MASK 0x00fffffffffffe00ULL
 
  static inline int64_t start_of_cluster(BDRVQcow2State *s, int64_t offset)
  {

Signature · 1 line

2.14.4

Quoted 9 lines · Kevin Wolf

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 3 lines

Match our code to the spec change in the previous patch - there's no reason for the refcount table to allow larger offsets than the L1/L2 tables.

What about internal snapshots? And anyway, because of the metadata overhead, the physical image size of a fully allocated image is always going to be at least minimally larger than the virtual disk size.

Quoted 3 lines

I'm not necessarily opposed to making the change if there is a good reason to make it, but I don't see a real need for it and the justification used here and also in the previous patch is incorrect.

The fact that ext4 cannot hold an image this large is already an indication that setting this limit on the refcount table is NOT going to bite real users.

Quoted 5 lines

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 3 lines

Match our code to the spec change in the previous patch - there's no reason for the refcount table to allow larger offsets than the L1/L2 tables.

Quoted 7 lines

What about internal snapshots? And anyway, because of the metadata overhead, the physical image size of a fully allocated image is always going to be at least minimally larger than the virtual disk size.

I'm not necessarily opposed to making the change if there is a good reason to make it, but I don't see a real need for it and the justification used here and also in the previous patch is incorrect.

The fact that ext4 cannot hold an image this large is already an indication that setting this limit on the refcount table is NOT going to bite real users.

Yes, you can argue that with lots of metadata, including internal snapshots, and on a capable file system (such as tmpfs) that can even hold files this large to begin with, then yes, allowing the refcount to exceed this limit will allow slightly more metadata to be crammed into a single image. But will it actually help anyone?

Do I need to respin the series to split patch 2 into the obvious changes (stuff unrelated to capping refcount size) vs. the controversial stuff (refcount cap and this code change)?

Quoted 3 lines

Kevin

Quoted 3 lines

Quoted 3 lines

In practice, no image has more than 64PB of allocated clusters anyways, as anything beyond that can't be expressed via L2 mappings to host offsets.

If you're opposed to an exact 56-bit limit on the grounds that 56-bit guest data plus minimal metadata should still be expressable, would it be better to cap refcount at 57-bits?

Am 29.06.2018 um 17:22 hat Eric Blake geschrieben:

Quoted 35 lines · Kevin Wolf

Quoted 5 lines

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 3 lines

Match our code to the spec change in the previous patch - there's no reason for the refcount table to allow larger offsets than the L1/L2 tables.

Quoted 7 lines

What about internal snapshots? And anyway, because of the metadata overhead, the physical image size of a fully allocated image is always going to be at least minimally larger than the virtual disk size.

I'm not necessarily opposed to making the change if there is a good reason to make it, but I don't see a real need for it and the justification used here and also in the previous patch is incorrect.

The fact that ext4 cannot hold an image this large is already an indication that setting this limit on the refcount table is NOT going to bite real users.

Yes, you can argue that with lots of metadata, including internal snapshots, and on a capable file system (such as tmpfs) that can even hold files this large to begin with, then yes, allowing the refcount to exceed this limit will allow slightly more metadata to be crammed into a single image. But will it actually help anyone?

Do I need to respin the series to split patch 2 into the obvious changes (stuff unrelated to capping refcount size) vs. the controversial stuff (refcount cap and this code change)?

Quoted 6 lines

Kevin

Quoted 3 lines

In practice, no image has more than 64PB of allocated clusters anyways, as anything beyond that can't be expressed via L2 mappings to host offsets.

Quoted 3 lines

If you're opposed to an exact 56-bit limit on the grounds that 56-bit guest data plus minimal metadata should still be expressable, would it be better to cap refcount at 57-bits?

You're arguing that the patch doesn't hurt in practice, but what I'm really looking for is what do we gain from it?

We generally don't apply patches just because they don't hurt, but because they are helpful for something. In the simple, fully compatible cases "helpful" could just mean "we like the code better".

This is a slightly different category: "it's technically incompatible, but we don't think anyone uses this". We have done such changes before when they allowed us to improve something substantially, but I think the requirements for this are at least a little higher than just "because we like it better". And I'm not even sure yet if I like it better because it seems like an arbitrary restriction.

So apart from not hurting in practice, what does the restriction buy us?

When reading a compressed image, we were allocating s->cluster_data to 32*cluster_size + 512 (possibly over 64 megabytes, for an image with 2M clusters). Let's check out the history:

Back when qcow2 was first written, we used s->cluster_data for everything, including copy_sectors() and encryption, where we want to operate on more than one cluster at once. Obviously, at that point, the buffer had to be aligned for other users, even though compression itself doesn't require any alignment (the fact that the compressed data generally starts mid-sector means that aligning our buffer buys us nothing - either the protocol already supports byte-based access into whatever offset we want, or we are already using a bounce buffer to read a full sector, and copying into our destination no longer requires alignment).

But commit 1b9f1491 (v1.1!) changed things to allocate parallel buffers on demand rather than sharing a single buffer, for encryption and COW, leaving compression as the final client of s->cluster_data. That use was still preserved, because if a single compressed cluster is read more than once, we reuse the cache instead of decompressing it a second time (someday, we may come up with better caching to avoid wasting repeated decompressions while still being more parallel, but that is a task for another patch; the XXX comment in qcow2_co_preadv for QCOW2_CLUSTER_COMPRESSED is telling).

Much later, in commit de82815d (v2.2), we noticed that a 64M allocation is prone to failure, so we switched over to a graceful memory allocation error message. Elsewhere in the code, we do g_malloc(2 * cluster_size) without ever checking for failure, but even 4M starts to be large enough that trying to be nice is worth the effort, so we want to keep that aspect.

Then even later, in 3e4c7052 (2.11), we realized that allocating a large buffer up front for every qcow2 image is expensive, and switched to lazy allocation only for images that actually had compressed clusters. But in the process, we never even bothered to check whether what we were allocating still made sense in its new context!

So, it's time to cut back on the waste. A compressed cluster written by qemu will NEVER occupy more than an uncompressed cluster, but based on mid-sector alignment, we may still need to read 1 cluster + 1 sector in order to recover enough bytes for the decompression. Meanwhile, third-party producers of qcow2 may not be as smart, and gzip DOES document that because the compression stream adds metadata, and because of the pigeonhole principle, there are worst case scenarios where attempts to compress will actually inflate an image, by up to 0.015% (at most 1 sector larger for an unfortunate 2M compression), meaning we should realistically expect to sometimes need to read 1 cluster + 2 sectors.

However, the qcow2 spec permits an all-ones sector count, plus a full sector containing the initial offset, for a maximum read of 2 full clusters. Thanks to the way decompression works, even though such a value is too large for the actual compressed data (for all but 512-byte clusters), it really doesn't matter if we read too much (gzip ignores slop, once it has decoded a full cluster). So it's easier to just allocate cluster_data to be as large as we can ever possibly see; even if it still wastes up to 2M on any image created by qcow2, that's still an improvment of 60M less waste than pre-patch.

Signed-off-by Eric Blake <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

---
v5: fix math error in commit message [Max]
v4: fix off-by-one in assertion and commit message [Berto]
v3: tighten allocation [Berto]
v2: actually check allocation failure (previous version meant to use g_malloc, but ended up posted with g_try_malloc without checking); add assertions outside of conditional, improve commit message to better match reality now that qcow2 spec bug has been fixed

---
 block/qcow2-cluster.c | 27 ++++++++++++++++++---------
 block/qcow2.c         |  2 +-
 2 files changed, 19 insertions(+), 10 deletions(-)

block/qcow2-cluster.c +12 -6

index 4701fbc7a12..b98fe683726 100644
@@ -1599,20 +1599,29 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
         sector_offset = coffset & 511;
         csize = nb_csectors * 512 - sector_offset;
 
-        /* Allocate buffers on first decompress operation, most images are
-         * uncompressed and the memory overhead can be avoided.  The buffers
-         * are freed in .bdrv_close().
+        /* Allocate buffers on the first decompress operation; most
+         * images are uncompressed and the memory overhead can be
+         * avoided.  The buffers are freed in .bdrv_close().  qemu
+         * never writes an inflated cluster, and gzip itself never
+         * inflates a problematic cluster by more than 0.015%, but the
+         * qcow2 format allows up to 1 byte short of 2 full clusters
+         * when including the sector containing offset.  gzip ignores
+         * trailing slop, so just allocate that much up front rather
+         * than reject third-party images with overlarge csize.
          */
+        assert(!!s->cluster_data == !!s->cluster_cache);
+        assert(csize <= 2 * s->cluster_size);
         if (!s->cluster_data) {
-            /* one more sector for decompressed data alignment */
-            s->cluster_data = qemu_try_blockalign(bs->file->bs,
-                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
+            s->cluster_data = g_try_malloc(2 * s->cluster_size);
             if (!s->cluster_data) {
                 return -ENOMEM;
             }

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 14 lines

When reading a compressed image, we were allocating s->cluster_data to 32*cluster_size + 512 (possibly over 64 megabytes, for an image with 2M clusters). Let's check out the history:

However, the qcow2 spec permits an all-ones sector count, plus a full sector containing the initial offset, for a maximum read of 2 full clusters. Thanks to the way decompression works, even though such a value is too large for the actual compressed data (for all but 512-byte clusters), it really doesn't matter if we read too much (gzip ignores slop, once it has decoded a full cluster). So it's easier to just allocate cluster_data to be as large as we can ever possibly see; even if it still wastes up to 2M on any image created by qcow2, that's still an improvment of

Quoted 40 lines

60M less waste than pre-patch.

Signed-off-by Eric Blake <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

---

+++ b/block/qcow2-cluster.c

block/qcow2-cluster.c +14 -9

@@ -1599,20 +1599,29 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
          sector_offset = coffset & 511;
          csize = nb_csectors * 512 - sector_offset;
 
-        /* Allocate buffers on first decompress operation, most images are
-         * uncompressed and the memory overhead can be avoided.  The buffers
-         * are freed in .bdrv_close().
+        /* Allocate buffers on the first decompress operation; most
+         * images are uncompressed and the memory overhead can be
+         * avoided.  The buffers are freed in .bdrv_close().  qemu
+         * never writes an inflated cluster, and gzip itself never
+         * inflates a problematic cluster by more than 0.015%, but the
+         * qcow2 format allows up to 1 byte short of 2 full clusters
+         * when including the sector containing offset.  gzip ignores
+         * trailing slop, so just allocate that much up front rather
+         * than reject third-party images with overlarge csize.
           */
+        assert(!!s->cluster_data == !!s->cluster_cache);
+        assert(csize <= 2 * s->cluster_size);
          if (!s->cluster_data) {
-            /* one more sector for decompressed data alignment */
-            s->cluster_data = qemu_try_blockalign(bs->file->bs,
-                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
+            s->cluster_data = g_try_malloc(2 * s->cluster_size);
              if (!s->cluster_data) {
                  return -ENOMEM;
              }
-        }
-        if (!s->cluster_cache) {
-            s->cluster_cache = g_malloc(s->cluster_size);
+            s->cluster_cache = g_try_malloc(s->cluster_size);
+            if (!s->cluster_cache) {

block/qcow2-cluster.c int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) · line 1625

Quoted 5 lines

block/qcow2-cluster.c +4 -0

+                g_free(s->cluster_data);
+                s->cluster_data = NULL;
+                return -ENOMEM;
+            }
          }

I wonder how much of a difference s->cluster_cache really makes. I wouldn't expect guests to access the same cluster twice in a row.

Maybe the easiest solution would be switching to temporary buffers that would have the exact size we need and would be freed at the end of the request?

Quoted 15 lines

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 14 lines

When reading a compressed image, we were allocating s->cluster_data to 32*cluster_size + 512 (possibly over 64 megabytes, for an image with 2M clusters). Let's check out the history:

However, the qcow2 spec permits an all-ones sector count, plus a full sector containing the initial offset, for a maximum read of 2 full clusters. Thanks to the way decompression works, even though such a value is too large for the actual compressed data (for all but 512-byte clusters), it really doesn't matter if we read too much (gzip ignores slop, once it has decoded a full cluster). So it's easier to just allocate cluster_data to be as large as we can ever possibly see; even if it still wastes up to 2M on any image created by qcow2, that's still an improvment of

s/improvment/improvement/

Quoted 40 lines

Quoted 40 lines

60M less waste than pre-patch.

Signed-off-by Eric Blake <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

---

+++ b/block/qcow2-cluster.c

block/qcow2-cluster.c +14 -9

@@ -1599,20 +1599,29 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
          sector_offset = coffset & 511;
          csize = nb_csectors * 512 - sector_offset;
 
-        /* Allocate buffers on first decompress operation, most images are
-         * uncompressed and the memory overhead can be avoided.  The buffers
-         * are freed in .bdrv_close().
+        /* Allocate buffers on the first decompress operation; most
+         * images are uncompressed and the memory overhead can be
+         * avoided.  The buffers are freed in .bdrv_close().  qemu
+         * never writes an inflated cluster, and gzip itself never
+         * inflates a problematic cluster by more than 0.015%, but the
+         * qcow2 format allows up to 1 byte short of 2 full clusters
+         * when including the sector containing offset.  gzip ignores
+         * trailing slop, so just allocate that much up front rather
+         * than reject third-party images with overlarge csize.
           */
+        assert(!!s->cluster_data == !!s->cluster_cache);
+        assert(csize <= 2 * s->cluster_size);
          if (!s->cluster_data) {
-            /* one more sector for decompressed data alignment */
-            s->cluster_data = qemu_try_blockalign(bs->file->bs,
-                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
+            s->cluster_data = g_try_malloc(2 * s->cluster_size);
              if (!s->cluster_data) {
                  return -ENOMEM;
              }
-        }
-        if (!s->cluster_cache) {
-            s->cluster_cache = g_malloc(s->cluster_size);
+            s->cluster_cache = g_try_malloc(s->cluster_size);
+            if (!s->cluster_cache) {

Quoted 8 lines

block/qcow2-cluster.c int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) · line 1625

Quoted 5 lines

block/qcow2-cluster.c +4 -0

+                g_free(s->cluster_data);
+                s->cluster_data = NULL;
+                return -ENOMEM;
+            }
          }

I wonder how much of a difference s->cluster_cache really makes. I wouldn't expect guests to access the same cluster twice in a row.

I don't know if it makes a difference. But my patch is not even touching that - ALL I'm doing is changing a 64M allocation into a 4M allocation, with otherwise no change to the frequency of allocation (which is once per image).

Quoted 1 line

Quoted 3 lines

Maybe the easiest solution would be switching to temporary buffers that would have the exact size we need and would be freed at the end of the request?

The exact size for a qemu-produced image would be at most 2M instead of 4M - but doing the change you propose WILL cause more frequent allocations (once per cluster, rather than once per image). We'd have to benchmark if it makes sense.

But that would be a separate patch from this one.

Am 29.06.2018 um 17:16 hat Eric Blake geschrieben:

Quoted 74 lines · Kevin Wolf

Quoted 15 lines

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 14 lines

When reading a compressed image, we were allocating s->cluster_data to 32*cluster_size + 512 (possibly over 64 megabytes, for an image with 2M clusters). Let's check out the history:

However, the qcow2 spec permits an all-ones sector count, plus a full sector containing the initial offset, for a maximum read of 2 full clusters. Thanks to the way decompression works, even though such a value is too large for the actual compressed data (for all but 512-byte clusters), it really doesn't matter if we read too much (gzip ignores slop, once it has decoded a full cluster). So it's easier to just allocate cluster_data to be as large as we can ever possibly see; even if it still wastes up to 2M on any image created by qcow2, that's still an improvment of

s/improvment/improvement/

Quoted 40 lines

Quoted 40 lines

60M less waste than pre-patch.

Signed-off-by Eric Blake <[email protected]>

Reviewed-by Alberto Garcia <[email protected]>

---

+++ b/block/qcow2-cluster.c

block/qcow2-cluster.c +14 -9

@@ -1599,20 +1599,29 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
          sector_offset = coffset & 511;
          csize = nb_csectors * 512 - sector_offset;
 
-        /* Allocate buffers on first decompress operation, most images are
-         * uncompressed and the memory overhead can be avoided.  The buffers
-         * are freed in .bdrv_close().
+        /* Allocate buffers on the first decompress operation; most
+         * images are uncompressed and the memory overhead can be
+         * avoided.  The buffers are freed in .bdrv_close().  qemu
+         * never writes an inflated cluster, and gzip itself never
+         * inflates a problematic cluster by more than 0.015%, but the
+         * qcow2 format allows up to 1 byte short of 2 full clusters
+         * when including the sector containing offset.  gzip ignores
+         * trailing slop, so just allocate that much up front rather
+         * than reject third-party images with overlarge csize.
           */
+        assert(!!s->cluster_data == !!s->cluster_cache);
+        assert(csize <= 2 * s->cluster_size);
          if (!s->cluster_data) {
-            /* one more sector for decompressed data alignment */
-            s->cluster_data = qemu_try_blockalign(bs->file->bs,
-                    QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
+            s->cluster_data = g_try_malloc(2 * s->cluster_size);
              if (!s->cluster_data) {
                  return -ENOMEM;
              }
-        }
-        if (!s->cluster_cache) {
-            s->cluster_cache = g_malloc(s->cluster_size);
+            s->cluster_cache = g_try_malloc(s->cluster_size);
+            if (!s->cluster_cache) {

Quoted 8 lines

block/qcow2-cluster.c int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) · line 1625

Quoted 5 lines

block/qcow2-cluster.c +4 -0

+                g_free(s->cluster_data);
+                s->cluster_data = NULL;
+                return -ENOMEM;
+            }
          }

I wonder how much of a difference s->cluster_cache really makes. I wouldn't expect guests to access the same cluster twice in a row.

I don't know if it makes a difference. But my patch is not even touching that - ALL I'm doing is changing a 64M allocation into a 4M allocation, with otherwise no change to the frequency of allocation (which is once per image).

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Maybe the easiest solution would be switching to temporary buffers that would have the exact size we need and would be freed at the end of the request?

The exact size for a qemu-produced image would be at most 2M instead of 4M - but doing the change you propose WILL cause more frequent allocations (once per cluster, rather than once per image). We'd have to benchmark if it makes sense.

I wouldn't expect that another allocation makes a big difference when you already have to decompress the whole cluster. In fact, it could speed things up because we could then parallelise this.

Hmm... Wasn't there a series for using a worker thread for decompression recently? It might actually already make that change, I don't remember.

Am 29.06.2018 um 17:16 hat Eric Blake geschrieben:

Quoted 31 lines · Kevin Wolf

Quoted 15 lines

Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:

Quoted 14 lines

When reading a compressed image, we were allocating s->cluster_data to 32*cluster_size + 512 (possibly over 64 megabytes, for an image with 2M clusters). Let's check out the history:

However, the qcow2 spec permits an all-ones sector count, plus a full sector containing the initial offset, for a maximum read of 2 full clusters. Thanks to the way decompression works, even though such a value is too large for the actual compressed data (for all but 512-byte clusters), it really doesn't matter if we read too much (gzip ignores slop, once it has decoded a full cluster). So it's easier to just allocate cluster_data to be as large as we can ever possibly see; even if it still wastes up to 2M on any image created by qcow2, that's still an improvment of

s/improvment/improvement/

Quoted 5 lines

Quoted 2 lines

60M less waste than pre-patch.

I wonder how much of a difference s->cluster_cache really makes. I wouldn't expect guests to access the same cluster twice in a row.

I don't know if it makes a difference. But my patch is not even touching that - ALL I'm doing is changing a 64M allocation into a 4M allocation, with otherwise no change to the frequency of allocation (which is once per image).

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Maybe the easiest solution would be switching to temporary buffers that would have the exact size we need and would be freed at the end of the request?

The exact size for a qemu-produced image would be at most 2M instead of 4M - but doing the change you propose WILL cause more frequent allocations (once per cluster, rather than once per image). We'd have to benchmark if it makes sense.

I wouldn't expect that another allocation makes a big difference when you already have to decompress the whole cluster. In fact, it could speed things up because we could then parallelise this.

Hmm... Wasn't there a series for using a worker thread for decompression recently? It might actually already make that change, I don't remember.

Quoted 1 line

But that would be a separate patch from this one.

Yes, just a thought I had while reviewing your patch.