[Qemu-devel] [PATCH 0/4] qcow2: Preallocation fixes

[Qemu-devel] [PATCH 0/4] qcow2: Preallocation fixes

[Kevin Wolf]

Kevin Wolf (4):
  qcow2: Avoid COW during metadata preallocation
  qcow2: Fix preallocation bdrv_pwrite to wrong file
  qcow2: Add errp to preallocate_co()
  qcow2: Fix full preallocation with external data file

 block/qcow2.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 0/4] qcow2: Preallocation fixes

[Kevin Wolf]

Kevin Wolf (4):
  qcow2: Avoid COW during metadata preallocation
  qcow2: Fix preallocation bdrv_pwrite to wrong file
  qcow2: Add errp to preallocate_co()
  qcow2: Fix full preallocation with external data file

 block/qcow2.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/4] qcow2: Avoid COW during metadata preallocation

[Kevin Wolf]

Limiting the allocation to INT_MAX bytes isn't particularly clever
because it means that the final cluster will be a partial cluster which
will be completed through a COW operation. This results in unnecessary
data read and write requests which lead to an unwanted non-sparse
filesystem block for metadata preallocation.

Align the maximum allocation size down to the cluster size to avoid this
situation.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d507ee0686..c8400e9712 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2723,6 +2723,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
 static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
                                        uint64_t new_length)
 {
+    BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
     uint64_t host_offset = 0;
     unsigned int cur_bytes;
@@ -2733,7 +2734,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
     bytes = new_length - offset;
 
     while (bytes) {
-        cur_bytes = MIN(bytes, INT_MAX);
+        cur_bytes = MIN(bytes, QEMU_ALIGN_DOWN(INT_MAX, s->cluster_size));
         ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
                                          &host_offset, &meta);
         if (ret < 0) {
-- 
2.20.1

[Qemu-devel] [PATCH 1/4] qcow2: Avoid COW during metadata preallocation

[Kevin Wolf]

Limiting the allocation to INT_MAX bytes isn't particularly clever
because it means that the final cluster will be a partial cluster which
will be completed through a COW operation. This results in unnecessary
data read and write requests which lead to an unwanted non-sparse
filesystem block for metadata preallocation.

Align the maximum allocation size down to the cluster size to avoid this
situation.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d507ee0686..c8400e9712 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2723,6 +2723,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
 static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
                                        uint64_t new_length)
 {
+    BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
     uint64_t host_offset = 0;
     unsigned int cur_bytes;
@@ -2733,7 +2734,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
     bytes = new_length - offset;
 
     while (bytes) {
-        cur_bytes = MIN(bytes, INT_MAX);
+        cur_bytes = MIN(bytes, QEMU_ALIGN_DOWN(INT_MAX, s->cluster_size));
         ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
                                          &host_offset, &meta);
         if (ret < 0) {
-- 
2.20.1

[Qemu-devel] [PATCH for-4.0? 2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file

[Kevin Wolf]

With an external data file, preallocate_co() must write the final byte
to the external data file, not to the qcow2 image file.

This is harmless for preallocation of newly created images (only the
qcow2 file size is increased to the virtual disk size while it should be
much smaller), but with preallocated resize, it could in theory cause
visible corruption if the metadata of the image is larger than the data
(e.g. lots of bitmaps).

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c8400e9712..dfac74c264 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2772,7 +2772,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
      */
     if (host_offset != 0) {
         uint8_t data = 0;
-        ret = bdrv_pwrite(bs->file, (host_offset + cur_bytes) - 1,
+        ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
                           &data, 1);
         if (ret < 0) {
             return ret;
-- 
2.20.1

[Qemu-devel] [PATCH for-4.0? 2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file

[Kevin Wolf]

With an external data file, preallocate_co() must write the final byte
to the external data file, not to the qcow2 image file.

This is harmless for preallocation of newly created images (only the
qcow2 file size is increased to the virtual disk size while it should be
much smaller), but with preallocated resize, it could in theory cause
visible corruption if the metadata of the image is larger than the data
(e.g. lots of bitmaps).

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c8400e9712..dfac74c264 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2772,7 +2772,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
      */
     if (host_offset != 0) {
         uint8_t data = 0;
-        ret = bdrv_pwrite(bs->file, (host_offset + cur_bytes) - 1,
+        ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
                           &data, 1);
         if (ret < 0) {
             return ret;
-- 
2.20.1

[Qemu-devel] [PATCH 3/4] qcow2: Add errp to preallocate_co()

[Kevin Wolf]

We'll add a bdrv_co_truncate() call in the next patch which can return
an Error that we don't want to discard. So add an errp parameter to
preallocate_co().

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index dfac74c264..b4f9f5a240 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2721,7 +2721,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
  * Returns: 0 on success, -errno on failure.
  */
 static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
-                                       uint64_t new_length)
+                                       uint64_t new_length, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
@@ -2738,6 +2738,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
         ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
                                          &host_offset, &meta);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Allocating clusters failed");
             return ret;
         }
 
@@ -2746,6 +2747,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
 
             ret = qcow2_alloc_cluster_link_l2(bs, meta);
             if (ret < 0) {
+                error_setg_errno(errp, -ret, "Mapping clusters failed");
                 qcow2_free_any_clusters(bs, meta->alloc_offset,
                                         meta->nb_clusters, QCOW2_DISCARD_NEVER);
                 return ret;
@@ -2775,6 +2777,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
         ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
                           &data, 1);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Writing to EOF failed");
             return ret;
         }
     }
@@ -3748,9 +3751,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         break;
 
     case PREALLOC_MODE_METADATA:
-        ret = preallocate_co(bs, old_length, offset);
+        ret = preallocate_co(bs, old_length, offset, errp);
         if (ret < 0) {
-            error_setg_errno(errp, -ret, "Preallocation failed");
             goto fail;
         }
         break;
@@ -3766,9 +3768,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         /* With a data file, preallocation means just allocating the metadata
          * and forwarding the truncate request to the data file */
         if (has_data_file(bs)) {
-            ret = preallocate_co(bs, old_length, offset);
+            ret = preallocate_co(bs, old_length, offset, errp);
             if (ret < 0) {
-                error_setg_errno(errp, -ret, "Preallocation failed");
                 goto fail;
             }
             break;
-- 
2.20.1

[Qemu-devel] [PATCH 3/4] qcow2: Add errp to preallocate_co()

[Kevin Wolf]

We'll add a bdrv_co_truncate() call in the next patch which can return
an Error that we don't want to discard. So add an errp parameter to
preallocate_co().

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index dfac74c264..b4f9f5a240 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2721,7 +2721,7 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
  * Returns: 0 on success, -errno on failure.
  */
 static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
-                                       uint64_t new_length)
+                                       uint64_t new_length, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
@@ -2738,6 +2738,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
         ret = qcow2_alloc_cluster_offset(bs, offset, &cur_bytes,
                                          &host_offset, &meta);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Allocating clusters failed");
             return ret;
         }
 
@@ -2746,6 +2747,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
 
             ret = qcow2_alloc_cluster_link_l2(bs, meta);
             if (ret < 0) {
+                error_setg_errno(errp, -ret, "Mapping clusters failed");
                 qcow2_free_any_clusters(bs, meta->alloc_offset,
                                         meta->nb_clusters, QCOW2_DISCARD_NEVER);
                 return ret;
@@ -2775,6 +2777,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
         ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
                           &data, 1);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Writing to EOF failed");
             return ret;
         }
     }
@@ -3748,9 +3751,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         break;
 
     case PREALLOC_MODE_METADATA:
-        ret = preallocate_co(bs, old_length, offset);
+        ret = preallocate_co(bs, old_length, offset, errp);
         if (ret < 0) {
-            error_setg_errno(errp, -ret, "Preallocation failed");
             goto fail;
         }
         break;
@@ -3766,9 +3768,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         /* With a data file, preallocation means just allocating the metadata
          * and forwarding the truncate request to the data file */
         if (has_data_file(bs)) {
-            ret = preallocate_co(bs, old_length, offset);
+            ret = preallocate_co(bs, old_length, offset, errp);
             if (ret < 0) {
-                error_setg_errno(errp, -ret, "Preallocation failed");
                 goto fail;
             }
             break;
-- 
2.20.1

[Qemu-devel] [PATCH 4/4] qcow2: Fix full preallocation with external data file

[Kevin Wolf]

preallocate_co() already gave the data file the full size without
forwarding the requested preallocation mode to the protocol. When
bdrv_co_truncate() was called later with the preallocation mode, the
file didn't actually grow any more, so the data file stayed unallocated
even if full preallocation was requested.

Pass the right preallocation mode to preallocate_co() and remove the
second bdrv_co_truncate() to fix this. As a side effect, the ugly
one-byte write in preallocate_co() is replaced with a truncate call,
now leaving the last block unallocated on the protocol level as it
should be.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b4f9f5a240..7fbef97aab 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2721,11 +2721,13 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
  * Returns: 0 on success, -errno on failure.
  */
 static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
-                                       uint64_t new_length, Error **errp)
+                                       uint64_t new_length, PreallocMode mode,
+                                       Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
     uint64_t host_offset = 0;
+    int64_t file_length;
     unsigned int cur_bytes;
     int ret;
     QCowL2Meta *meta;
@@ -2772,12 +2774,19 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
      * all of the allocated clusters (otherwise we get failing reads after
      * EOF). Extend the image to the last allocated sector.
      */
-    if (host_offset != 0) {
-        uint8_t data = 0;
-        ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
-                          &data, 1);
+    file_length = bdrv_getlength(s->data_file->bs);
+    if (file_length < 0) {
+        error_setg_errno(errp, -file_length, "Could not get file size");
+        return file_length;
+    }
+
+    if (host_offset + cur_bytes > file_length) {
+        if (mode == PREALLOC_MODE_METADATA) {
+            mode = PREALLOC_MODE_OFF;
+        }
+        ret = bdrv_co_truncate(s->data_file, host_offset + cur_bytes, mode,
+                               errp);
         if (ret < 0) {
-            error_setg_errno(errp, -ret, "Writing to EOF failed");
             return ret;
         }
     }
@@ -3748,10 +3757,16 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 
     switch (prealloc) {
     case PREALLOC_MODE_OFF:
+        if (has_data_file(bs)) {
+            ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
+            if (ret < 0) {
+                goto fail;
+            }
+        }
         break;
 
     case PREALLOC_MODE_METADATA:
-        ret = preallocate_co(bs, old_length, offset, errp);
+        ret = preallocate_co(bs, old_length, offset, prealloc, errp);
         if (ret < 0) {
             goto fail;
         }
@@ -3768,7 +3783,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         /* With a data file, preallocation means just allocating the metadata
          * and forwarding the truncate request to the data file */
         if (has_data_file(bs)) {
-            ret = preallocate_co(bs, old_length, offset, errp);
+            ret = preallocate_co(bs, old_length, offset, prealloc, errp);
             if (ret < 0) {
                 goto fail;
             }
@@ -3883,16 +3898,6 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 
     bs->total_sectors = offset / BDRV_SECTOR_SIZE;
 
-    if (has_data_file(bs)) {
-        if (prealloc == PREALLOC_MODE_METADATA) {
-            prealloc = PREALLOC_MODE_OFF;
-        }
-        ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
-        if (ret < 0) {
-            goto fail;
-        }
-    }
-
     /* write updated header.size */
     offset = cpu_to_be64(offset);
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
-- 
2.20.1

[Qemu-devel] [PATCH 4/4] qcow2: Fix full preallocation with external data file

[Kevin Wolf]

preallocate_co() already gave the data file the full size without
forwarding the requested preallocation mode to the protocol. When
bdrv_co_truncate() was called later with the preallocation mode, the
file didn't actually grow any more, so the data file stayed unallocated
even if full preallocation was requested.

Pass the right preallocation mode to preallocate_co() and remove the
second bdrv_co_truncate() to fix this. As a side effect, the ugly
one-byte write in preallocate_co() is replaced with a truncate call,
now leaving the last block unallocated on the protocol level as it
should be.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b4f9f5a240..7fbef97aab 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2721,11 +2721,13 @@ static int qcow2_set_up_encryption(BlockDriverState *bs,
  * Returns: 0 on success, -errno on failure.
  */
 static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
-                                       uint64_t new_length, Error **errp)
+                                       uint64_t new_length, PreallocMode mode,
+                                       Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t bytes;
     uint64_t host_offset = 0;
+    int64_t file_length;
     unsigned int cur_bytes;
     int ret;
     QCowL2Meta *meta;
@@ -2772,12 +2774,19 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
      * all of the allocated clusters (otherwise we get failing reads after
      * EOF). Extend the image to the last allocated sector.
      */
-    if (host_offset != 0) {
-        uint8_t data = 0;
-        ret = bdrv_pwrite(s->data_file, (host_offset + cur_bytes) - 1,
-                          &data, 1);
+    file_length = bdrv_getlength(s->data_file->bs);
+    if (file_length < 0) {
+        error_setg_errno(errp, -file_length, "Could not get file size");
+        return file_length;
+    }
+
+    if (host_offset + cur_bytes > file_length) {
+        if (mode == PREALLOC_MODE_METADATA) {
+            mode = PREALLOC_MODE_OFF;
+        }
+        ret = bdrv_co_truncate(s->data_file, host_offset + cur_bytes, mode,
+                               errp);
         if (ret < 0) {
-            error_setg_errno(errp, -ret, "Writing to EOF failed");
             return ret;
         }
     }
@@ -3748,10 +3757,16 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 
     switch (prealloc) {
     case PREALLOC_MODE_OFF:
+        if (has_data_file(bs)) {
+            ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
+            if (ret < 0) {
+                goto fail;
+            }
+        }
         break;
 
     case PREALLOC_MODE_METADATA:
-        ret = preallocate_co(bs, old_length, offset, errp);
+        ret = preallocate_co(bs, old_length, offset, prealloc, errp);
         if (ret < 0) {
             goto fail;
         }
@@ -3768,7 +3783,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         /* With a data file, preallocation means just allocating the metadata
          * and forwarding the truncate request to the data file */
         if (has_data_file(bs)) {
-            ret = preallocate_co(bs, old_length, offset, errp);
+            ret = preallocate_co(bs, old_length, offset, prealloc, errp);
             if (ret < 0) {
                 goto fail;
             }
@@ -3883,16 +3898,6 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
 
     bs->total_sectors = offset / BDRV_SECTOR_SIZE;
 
-    if (has_data_file(bs)) {
-        if (prealloc == PREALLOC_MODE_METADATA) {
-            prealloc = PREALLOC_MODE_OFF;
-        }
-        ret = bdrv_co_truncate(s->data_file, offset, prealloc, errp);
-        if (ret < 0) {
-            goto fail;
-        }
-    }
-
     /* write updated header.size */
     offset = cpu_to_be64(offset);
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
-- 
2.20.1

Re: [Qemu-devel] [PATCH 1/4] qcow2: Avoid COW during metadata preallocation

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> Limiting the allocation to INT_MAX bytes isn't particularly clever
> because it means that the final cluster will be a partial cluster which
> will be completed through a COW operation. This results in unnecessary
> data read and write requests which lead to an unwanted non-sparse
> filesystem block for metadata preallocation.
> 
> Align the maximum allocation size down to the cluster size to avoid this
> situation.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

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


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

Re: [Qemu-devel] [PATCH 1/4] qcow2: Avoid COW during metadata preallocation

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> Limiting the allocation to INT_MAX bytes isn't particularly clever
> because it means that the final cluster will be a partial cluster which
> will be completed through a COW operation. This results in unnecessary
> data read and write requests which lead to an unwanted non-sparse
> filesystem block for metadata preallocation.
> 
> Align the maximum allocation size down to the cluster size to avoid this
> situation.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

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


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

Re: [Qemu-devel] [PATCH for-4.0? 2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> With an external data file, preallocate_co() must write the final byte
> to the external data file, not to the qcow2 image file.
> 
> This is harmless for preallocation of newly created images (only the
> qcow2 file size is increased to the virtual disk size while it should be
> much smaller), but with preallocated resize, it could in theory cause
> visible corruption if the metadata of the image is larger than the data
> (e.g. lots of bitmaps).

Can we come up with such an image - maybe one with 512-byte cluster
sizing and only 1k in guest-visible length?  Since each bitmap is
cluster-aligned, it seems like you'd only need a couple of bitmaps to
easily reach that point.

We're awfully late for 4.0, but as we already have -rc4 coming due, and
as this is a data-corruption bug in a new feature, I can buy the
argument of getting this one into 4.0, particularly if you can design
the iotest along the lines of my ideas to prove that yes, indeed, we are
accidentally wiping out qcow2 metadata for visible image corruption.

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

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


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

Re: [Qemu-devel] [PATCH for-4.0? 2/4] qcow2: Fix preallocation bdrv_pwrite to wrong file

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> With an external data file, preallocate_co() must write the final byte
> to the external data file, not to the qcow2 image file.
> 
> This is harmless for preallocation of newly created images (only the
> qcow2 file size is increased to the virtual disk size while it should be
> much smaller), but with preallocated resize, it could in theory cause
> visible corruption if the metadata of the image is larger than the data
> (e.g. lots of bitmaps).

Can we come up with such an image - maybe one with 512-byte cluster
sizing and only 1k in guest-visible length?  Since each bitmap is
cluster-aligned, it seems like you'd only need a couple of bitmaps to
easily reach that point.

We're awfully late for 4.0, but as we already have -rc4 coming due, and
as this is a data-corruption bug in a new feature, I can buy the
argument of getting this one into 4.0, particularly if you can design
the iotest along the lines of my ideas to prove that yes, indeed, we are
accidentally wiping out qcow2 metadata for visible image corruption.

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

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


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

Re: [Qemu-devel] [PATCH 3/4] qcow2: Add errp to preallocate_co()

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> We'll add a bdrv_co_truncate() call in the next patch which can return
> an Error that we don't want to discard. So add an errp parameter to
> preallocate_co().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 3/4] qcow2: Add errp to preallocate_co()

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> We'll add a bdrv_co_truncate() call in the next patch which can return
> an Error that we don't want to discard. So add an errp parameter to
> preallocate_co().
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

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


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

Re: [Qemu-devel] [PATCH 4/4] qcow2: Fix full preallocation with external data file

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> preallocate_co() already gave the data file the full size without
> forwarding the requested preallocation mode to the protocol. When
> bdrv_co_truncate() was called later with the preallocation mode, the
> file didn't actually grow any more, so the data file stayed unallocated
> even if full preallocation was requested.
> 
> Pass the right preallocation mode to preallocate_co() and remove the
> second bdrv_co_truncate() to fix this. As a side effect, the ugly
> one-byte write in preallocate_co() is replaced with a truncate call,
> now leaving the last block unallocated on the protocol level as it
> should be.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 

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

Worth iotest coverage?

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


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

Re: [Qemu-devel] [PATCH 4/4] qcow2: Fix full preallocation with external data file

[Eric Blake]

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

On 4/15/19 10:54 AM, Kevin Wolf wrote:
> preallocate_co() already gave the data file the full size without
> forwarding the requested preallocation mode to the protocol. When
> bdrv_co_truncate() was called later with the preallocation mode, the
> file didn't actually grow any more, so the data file stayed unallocated
> even if full preallocation was requested.
> 
> Pass the right preallocation mode to preallocate_co() and remove the
> second bdrv_co_truncate() to fix this. As a side effect, the ugly
> one-byte write in preallocate_co() is replaced with a truncate call,
> now leaving the last block unallocated on the protocol level as it
> should be.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 

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

Worth iotest coverage?

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


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

Re: [Qemu-devel] [PATCH 0/4] qcow2: Preallocation fixes

[Kevin Wolf]

Am 15.04.2019 um 17:54 hat Kevin Wolf geschrieben:
> Kevin Wolf (4):
>   qcow2: Avoid COW during metadata preallocation
>   qcow2: Fix preallocation bdrv_pwrite to wrong file
>   qcow2: Add errp to preallocate_co()
>   qcow2: Fix full preallocation with external data file

Applied the remaining patches to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 0/4] qcow2: Preallocation fixes

[Kevin Wolf]

Am 15.04.2019 um 17:54 hat Kevin Wolf geschrieben:
> Kevin Wolf (4):
>   qcow2: Avoid COW during metadata preallocation
>   qcow2: Fix preallocation bdrv_pwrite to wrong file
>   qcow2: Add errp to preallocate_co()
>   qcow2: Fix full preallocation with external data file

Applied the remaining patches to the block branch.

Kevin