[PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Kevin Wolf]

See patch 4 for the description of the bug fixed.

v3:
- Don't allow blocking the monitor for a zero write in block_resize
  (even though we can already blockfor other reasons there). This is
  mainly responsible for the increased complexity compared to v2.
  Personally, I think this is not an improvement over v2, but if this is
  what it takes to fix a corruption issue in 4.2... [Max]
- Don't use huge image files in the test case [Vladimir]

v2:
- Switched order of bs->total_sectors update and zero write [Vladimir]
- Fixed coding style [Vladimir]
- Changed the commit message to contain what was in the cover letter
- Test all preallocation modes
- Test allocation status with qemu-io 'map' [Vladimir]

Kevin Wolf (8):
  block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter
  block: Add no_fallback parameter to bdrv_co_truncate()
  qcow2: Declare BDRV_REQ_NO_FALLBACK supported
  block: truncate: Don't make backing file data visible
  iotests: Add qemu_io_log()
  iotests: Fix timeout in run_job()
  iotests: Support job-complete in run_job()
  iotests: Test committing to short backing file

 include/block/block.h          |   5 +-
 include/sysemu/block-backend.h |   2 +-
 block/block-backend.c          |   4 +-
 block/commit.c                 |   4 +-
 block/crypto.c                 |   4 +-
 block/io.c                     |  55 +++++++--
 block/mirror.c                 |   2 +-
 block/parallels.c              |   6 +-
 block/qcow.c                   |   4 +-
 block/qcow2-refcount.c         |   2 +-
 block/qcow2.c                  |  22 ++--
 block/qed.c                    |   2 +-
 block/raw-format.c             |   2 +-
 block/vdi.c                    |   2 +-
 block/vhdx-log.c               |   2 +-
 block/vhdx.c                   |   6 +-
 block/vmdk.c                   |  10 +-
 block/vpc.c                    |   2 +-
 blockdev.c                     |   2 +-
 qemu-img.c                     |   2 +-
 qemu-io-cmds.c                 |   2 +-
 tests/test-block-iothread.c    |   6 +-
 tests/qemu-iotests/274         | 152 ++++++++++++++++++++++++
 tests/qemu-iotests/274.out     | 203 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group       |   1 +
 tests/qemu-iotests/iotests.py  |  11 +-
 26 files changed, 463 insertions(+), 52 deletions(-)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

-- 
2.20.1

[PATCH v3 1/8] block: bdrv_co_do_pwrite_zeroes: 64 bit 'bytes' parameter

[Kevin Wolf]

bdrv_co_do_pwrite_zeroes() can already cope with maximum request sizes
by calling the driver in a loop until everything is done. Make the small
remaining change that is necessary to let it accept a 64 bit byte count.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/io.c b/block/io.c
index f75777f5ea..003f4ea38c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -42,7 +42,7 @@
 
 static void bdrv_parent_cb_resize(BlockDriverState *bs);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-    int64_t offset, int bytes, BdrvRequestFlags flags);
+    int64_t offset, int64_t bytes, BdrvRequestFlags flags);
 
 static void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore,
                                       bool ignore_bds_parents)
@@ -1730,7 +1730,7 @@ int coroutine_fn bdrv_co_preadv_part(BdrvChild *child,
 }
 
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
-    int64_t offset, int bytes, BdrvRequestFlags flags)
+    int64_t offset, int64_t bytes, BdrvRequestFlags flags)
 {
     BlockDriver *drv = bs->drv;
     QEMUIOVector qiov;
@@ -1760,7 +1760,7 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     assert(max_write_zeroes >= bs->bl.request_alignment);
 
     while (bytes > 0 && !ret) {
-        int num = bytes;
+        int num = MIN(bytes, BDRV_REQUEST_MAX_BYTES);
 
         /* Align request.  Block drivers can expect the "bulk" of the request
          * to be aligned, and that unaligned requests do not cross cluster
-- 
2.20.1

[PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()

[Kevin Wolf]

This adds a no_fallback parameter to bdrv_co_truncate(), bdrv_truncate()
and blk_truncate() in preparation for a fix that potentially needs to
zero-write the new area. no_fallback will use BDRV_REQ_NO_FALLBACK for
this operation and lets the truncate fail if an efficient zero write
isn't possible.

Only qmp_block_resize() passes true for this parameter because it is a
blocking monitor command, so we don't want to add more potentially slow
I/O operations to it than we already have.

All other users will accept even a slow fallback to avoid failure.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h          |  5 +++--
 include/sysemu/block-backend.h |  2 +-
 block/block-backend.c          |  4 ++--
 block/commit.c                 |  4 ++--
 block/crypto.c                 |  4 ++--
 block/io.c                     | 16 ++++++++++++----
 block/mirror.c                 |  2 +-
 block/parallels.c              |  6 +++---
 block/qcow.c                   |  4 ++--
 block/qcow2-refcount.c         |  2 +-
 block/qcow2.c                  | 19 +++++++++++--------
 block/qed.c                    |  2 +-
 block/raw-format.c             |  2 +-
 block/vdi.c                    |  2 +-
 block/vhdx-log.c               |  2 +-
 block/vhdx.c                   |  6 +++---
 block/vmdk.c                   | 10 ++++++----
 block/vpc.c                    |  2 +-
 blockdev.c                     |  2 +-
 qemu-img.c                     |  2 +-
 qemu-io-cmds.c                 |  2 +-
 tests/test-block-iothread.c    |  6 +++---
 22 files changed, 60 insertions(+), 46 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 1df9848e74..3e44677905 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -347,9 +347,10 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
 void bdrv_refresh_filename(BlockDriverState *bs);
 
 int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
-                                  PreallocMode prealloc, Error **errp);
+                                  PreallocMode prealloc, bool no_fallback,
+                                  Error **errp);
 int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
-                  PreallocMode prealloc, Error **errp);
+                  PreallocMode prealloc, bool no_fallback, Error **errp);
 
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b198deca0b..487b29d13e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -238,7 +238,7 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int bytes);
 int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
-                 PreallocMode prealloc, Error **errp);
+                 PreallocMode prealloc, bool no_fallback, Error **errp);
 int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
                      int64_t pos, int size);
diff --git a/block/block-backend.c b/block/block-backend.c
index 8b8f2a80a0..fcc9d60cdb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2073,14 +2073,14 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
 }
 
 int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
-                 PreallocMode prealloc, Error **errp)
+                 PreallocMode prealloc, bool no_fallback, Error **errp)
 {
     if (!blk_is_available(blk)) {
         error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
     }
 
-    return bdrv_truncate(blk->root, offset, exact, prealloc, errp);
+    return bdrv_truncate(blk->root, offset, exact, prealloc, no_fallback, errp);
 }
 
 static void blk_pdiscard_entry(void *opaque)
diff --git a/block/commit.c b/block/commit.c
index 23c90b3b91..f074181d83 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -155,7 +155,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
     }
 
     if (base_len < len) {
-        ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, NULL);
+        ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, false, NULL);
         if (ret) {
             goto out;
         }
@@ -472,7 +472,7 @@ int bdrv_commit(BlockDriverState *bs)
      * we must return an error */
     if (length > backing_length) {
         ret = blk_truncate(backing, length, false, PREALLOC_MODE_OFF,
-                           &local_err);
+                           false, &local_err);
         if (ret < 0) {
             error_report_err(local_err);
             goto ro_cleanup;
diff --git a/block/crypto.c b/block/crypto.c
index 24823835c1..0f28e8b4e1 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -114,7 +114,7 @@ static ssize_t block_crypto_init_func(QCryptoBlock *block,
      * which will be used by the crypto header
      */
     return blk_truncate(data->blk, data->size + headerlen, false,
-                        data->prealloc, errp);
+                        data->prealloc, false, errp);
 }
 
 
@@ -311,7 +311,7 @@ block_crypto_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
 
     offset += payload_offset;
 
-    return bdrv_co_truncate(bs->file, offset, exact, prealloc, errp);
+    return bdrv_co_truncate(bs->file, offset, exact, prealloc, false, errp);
 }
 
 static void block_crypto_close(BlockDriverState *bs)
diff --git a/block/io.c b/block/io.c
index 003f4ea38c..42e7558954 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3313,9 +3313,15 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
  * If 'exact' is true, the file must be resized to exactly the given
  * 'offset'.  Otherwise, it is sufficient for the node to be at least
  * 'offset' bytes in length.
+ *
+ * If 'no_fallback' is true, a possibly needed writte_zeroes operation to avoid
+ * making a longer backing file visible will use BDRV_REQ_NO_FALLBACK. If the
+ * zero write is necessary and this flag is set, bdrv_co_truncate() will fail
+ * if efficient zero writes cannot be provided.
  */
 int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
-                                  PreallocMode prealloc, Error **errp)
+                                  PreallocMode prealloc, bool no_fallback,
+                                  Error **errp)
 {
     BlockDriverState *bs = child->bs;
     BlockDriver *drv = bs->drv;
@@ -3372,7 +3378,8 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
     if (drv->bdrv_co_truncate) {
         ret = drv->bdrv_co_truncate(bs, offset, exact, prealloc, errp);
     } else if (bs->file && drv->is_filter) {
-        ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, errp);
+        ret = bdrv_co_truncate(bs->file, offset, exact, prealloc, no_fallback,
+                               errp);
     } else {
         error_setg(errp, "Image format driver does not support resize");
         ret = -ENOTSUP;
@@ -3405,6 +3412,7 @@ typedef struct TruncateCo {
     int64_t offset;
     bool exact;
     PreallocMode prealloc;
+    bool no_fallback;
     Error **errp;
     int ret;
 } TruncateCo;
@@ -3413,12 +3421,12 @@ static void coroutine_fn bdrv_truncate_co_entry(void *opaque)
 {
     TruncateCo *tco = opaque;
     tco->ret = bdrv_co_truncate(tco->child, tco->offset, tco->exact,
-                                tco->prealloc, tco->errp);
+                                tco->prealloc, tco->no_fallback, tco->errp);
     aio_wait_kick();
 }
 
 int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
-                  PreallocMode prealloc, Error **errp)
+                  PreallocMode prealloc, bool no_fallback, Error **errp)
 {
     Coroutine *co;
     TruncateCo tco = {
diff --git a/block/mirror.c b/block/mirror.c
index f0f2d9dff1..a333533a38 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -879,7 +879,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
 
         if (s->bdev_length > base_length) {
             ret = blk_truncate(s->target, s->bdev_length, false,
-                               PREALLOC_MODE_OFF, NULL);
+                               PREALLOC_MODE_OFF, false, NULL);
             if (ret < 0) {
                 goto immediate_exit;
             }
diff --git a/block/parallels.c b/block/parallels.c
index 7a01997659..859f3f4904 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -203,7 +203,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
         } else {
             ret = bdrv_truncate(bs->file,
                                 (s->data_end + space) << BDRV_SECTOR_BITS,
-                                false, PREALLOC_MODE_OFF, NULL);
+                                false, PREALLOC_MODE_OFF, false, NULL);
         }
         if (ret < 0) {
             return ret;
@@ -493,7 +493,7 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
              * That means we have to pass exact=true.
              */
             ret = bdrv_truncate(bs->file, res->image_end_offset, true,
-                                PREALLOC_MODE_OFF, &local_err);
+                                PREALLOC_MODE_OFF, false, &local_err);
             if (ret < 0) {
                 error_report_err(local_err);
                 res->check_errors++;
@@ -888,7 +888,7 @@ static void parallels_close(BlockDriverState *bs)
 
         /* errors are ignored, so we might as well pass exact=true */
         bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
-                      PREALLOC_MODE_OFF, NULL);
+                      PREALLOC_MODE_OFF, false, NULL);
     }
 
     g_free(s->bat_dirty_bmap);
diff --git a/block/qcow.c b/block/qcow.c
index fce8989868..f43cb59cc0 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -480,7 +480,7 @@ static int get_cluster_offset(BlockDriverState *bs,
                     return -E2BIG;
                 }
                 ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
-                                    false, PREALLOC_MODE_OFF, NULL);
+                                    false, PREALLOC_MODE_OFF, false, NULL);
                 if (ret < 0) {
                     return ret;
                 }
@@ -1034,7 +1034,7 @@ static int qcow_make_empty(BlockDriverState *bs)
             l1_length) < 0)
         return -1;
     ret = bdrv_truncate(bs->file, s->l1_table_offset + l1_length, false,
-                        PREALLOC_MODE_OFF, NULL);
+                        PREALLOC_MODE_OFF, false, NULL);
     if (ret < 0)
         return ret;
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index f67ac6b2d8..fdfdec336d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -2017,7 +2017,7 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
                 }
 
                 ret = bdrv_truncate(bs->file, offset + s->cluster_size, false,
-                                    PREALLOC_MODE_OFF, &local_err);
+                                    PREALLOC_MODE_OFF, false, &local_err);
                 if (ret < 0) {
                     error_report_err(local_err);
                     goto resize_fail;
diff --git a/block/qcow2.c b/block/qcow2.c
index 7c18721741..b201383c3d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3075,7 +3075,7 @@ static int coroutine_fn preallocate_co(BlockDriverState *bs, uint64_t offset,
             mode = PREALLOC_MODE_OFF;
         }
         ret = bdrv_co_truncate(s->data_file, host_offset + cur_bytes, false,
-                               mode, errp);
+                               mode, false, errp);
         if (ret < 0) {
             return ret;
         }
@@ -3490,7 +3490,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 
     /* Okay, now that we have a valid image, let's give it the right size */
     ret = blk_truncate(blk, qcow2_opts->size, false, qcow2_opts->preallocation,
-                       errp);
+                       false, errp);
     if (ret < 0) {
         error_prepend(errp, "Could not resize image: ");
         goto out;
@@ -4035,7 +4035,7 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
              * always fulfilled, so there is no need to pass it on.)
              */
             bdrv_co_truncate(bs->file, (last_cluster + 1) * s->cluster_size,
-                             false, PREALLOC_MODE_OFF, &local_err);
+                             false, PREALLOC_MODE_OFF, false, &local_err);
             if (local_err) {
                 warn_reportf_err(local_err,
                                  "Failed to truncate the tail of the image: ");
@@ -4057,7 +4057,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
              * file should be resized to the exact target size, too,
              * so we pass @exact here.
              */
-            ret = bdrv_co_truncate(s->data_file, offset, exact, prealloc, errp);
+            ret = bdrv_co_truncate(s->data_file, offset, exact, prealloc, false,
+                                   errp);
             if (ret < 0) {
                 goto fail;
             }
@@ -4143,7 +4144,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
         new_file_size = allocation_start +
                         nb_new_data_clusters * s->cluster_size;
         /* Image file grows, so @exact does not matter */
-        ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, errp);
+        ret = bdrv_co_truncate(bs->file, new_file_size, false, prealloc, false,
+                               errp);
         if (ret < 0) {
             error_prepend(errp, "Failed to resize underlying file: ");
             qcow2_free_clusters(bs, allocation_start,
@@ -4246,7 +4248,8 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
         if (len < 0) {
             return len;
         }
-        return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, NULL);
+        return bdrv_co_truncate(bs->file, len, false, PREALLOC_MODE_OFF, false,
+                                NULL);
     }
 
     if (offset_into_cluster(s, offset)) {
@@ -4484,7 +4487,7 @@ static int make_completely_empty(BlockDriverState *bs)
     }
 
     ret = bdrv_truncate(bs->file, (3 + l1_clusters) * s->cluster_size, false,
-                        PREALLOC_MODE_OFF, &local_err);
+                        PREALLOC_MODE_OFF, false, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         goto fail;
@@ -5327,7 +5330,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
          * Amending image options should ensure that the image has
          * exactly the given new values, so pass exact=true here.
          */
-        ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, errp);
+        ret = blk_truncate(blk, new_size, true, PREALLOC_MODE_OFF, false, errp);
         blk_unref(blk);
         if (ret < 0) {
             return ret;
diff --git a/block/qed.c b/block/qed.c
index d8c4e5fb1e..11e5cb37d9 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -677,7 +677,7 @@ static int coroutine_fn bdrv_qed_co_create(BlockdevCreateOptions *opts,
      * The QED format associates file length with allocation status,
      * so a new file (which is empty) must have a length of 0.
      */
-    ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate(blk, 0, true, PREALLOC_MODE_OFF, false, errp);
     if (ret < 0) {
         goto out;
     }
diff --git a/block/raw-format.c b/block/raw-format.c
index 3a76ec7dd2..950334cdb4 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -387,7 +387,7 @@ static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
 
     s->size = offset;
     offset += s->offset;
-    return bdrv_co_truncate(bs->file, offset, exact, prealloc, errp);
+    return bdrv_co_truncate(bs->file, offset, exact, prealloc, false, errp);
 }
 
 static void raw_eject(BlockDriverState *bs, bool eject_flag)
diff --git a/block/vdi.c b/block/vdi.c
index 0142da7233..2deb600614 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -875,7 +875,7 @@ static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
 
     if (image_type == VDI_TYPE_STATIC) {
         ret = blk_truncate(blk, offset + blocks * block_size, false,
-                           PREALLOC_MODE_OFF, errp);
+                           PREALLOC_MODE_OFF, false, errp);
         if (ret < 0) {
             error_prepend(errp, "Failed to statically allocate file");
             goto exit;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 13a49c2a33..4cd32a1231 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -558,7 +558,7 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
                     goto exit;
                 }
                 ret = bdrv_truncate(bs->file, new_file_size, false,
-                                    PREALLOC_MODE_OFF, NULL);
+                                    PREALLOC_MODE_OFF, false, NULL);
                 if (ret < 0) {
                     goto exit;
                 }
diff --git a/block/vhdx.c b/block/vhdx.c
index f02d2611be..a58b2e2768 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1264,7 +1264,7 @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
     }
 
     return bdrv_truncate(bs->file, *new_offset + s->block_size, false,
-                         PREALLOC_MODE_OFF, NULL);
+                         PREALLOC_MODE_OFF, false, NULL);
 }
 
 /*
@@ -1703,13 +1703,13 @@ static int vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
         /* All zeroes, so we can just extend the file - the end of the BAT
          * is the furthest thing we have written yet */
         ret = blk_truncate(blk, data_file_offset, false, PREALLOC_MODE_OFF,
-                           errp);
+                           false, errp);
         if (ret < 0) {
             goto exit;
         }
     } else if (type == VHDX_TYPE_FIXED) {
         ret = blk_truncate(blk, data_file_offset + image_size, false,
-                           PREALLOC_MODE_OFF, errp);
+                           PREALLOC_MODE_OFF, false, errp);
         if (ret < 0) {
             goto exit;
         }
diff --git a/block/vmdk.c b/block/vmdk.c
index 20e909d997..1fbfed45ce 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2077,7 +2077,7 @@ vmdk_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
             }
             length = QEMU_ALIGN_UP(length, BDRV_SECTOR_SIZE);
             ret = bdrv_truncate(s->extents[i].file, length, false,
-                                PREALLOC_MODE_OFF, NULL);
+                                PREALLOC_MODE_OFF, false, NULL);
             if (ret < 0) {
                 return ret;
             }
@@ -2118,7 +2118,8 @@ static int vmdk_init_extent(BlockBackend *blk,
     int gd_buf_size;
 
     if (flat) {
-        ret = blk_truncate(blk, filesize, false, PREALLOC_MODE_OFF, errp);
+        ret = blk_truncate(blk, filesize, false, PREALLOC_MODE_OFF, false,
+                           errp);
         goto exit;
     }
     magic = cpu_to_be32(VMDK4_MAGIC);
@@ -2182,7 +2183,7 @@ static int vmdk_init_extent(BlockBackend *blk,
     }
 
     ret = blk_truncate(blk, le64_to_cpu(header.grain_offset) << 9, false,
-                       PREALLOC_MODE_OFF, errp);
+                       PREALLOC_MODE_OFF, false, errp);
     if (ret < 0) {
         goto exit;
     }
@@ -2523,7 +2524,8 @@ static int coroutine_fn vmdk_co_do_create(int64_t size,
     /* bdrv_pwrite write padding zeros to align to sector, we don't need that
      * for description file */
     if (desc_offset == 0) {
-        ret = blk_truncate(blk, desc_len, false, PREALLOC_MODE_OFF, errp);
+        ret = blk_truncate(blk, desc_len, false, PREALLOC_MODE_OFF, false,
+                           errp);
         if (ret < 0) {
             goto exit;
         }
diff --git a/block/vpc.c b/block/vpc.c
index a65550298e..9202a517e1 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -898,7 +898,7 @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
     /* Add footer to total size */
     total_size += HEADER_SIZE;
 
-    ret = blk_truncate(blk, total_size, false, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate(blk, total_size, false, PREALLOC_MODE_OFF, false, errp);
     if (ret < 0) {
         return ret;
     }
diff --git a/blockdev.c b/blockdev.c
index 8e029e9c01..d7d3c250f6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3204,7 +3204,7 @@ void qmp_block_resize(bool has_device, const char *device,
     }
 
     bdrv_drained_begin(bs);
-    ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, errp);
+    ret = blk_truncate(blk, size, false, PREALLOC_MODE_OFF, true, errp);
     bdrv_drained_end(bs);
 
 out:
diff --git a/qemu-img.c b/qemu-img.c
index 95a24b9762..7ee450bc18 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3836,7 +3836,7 @@ static int img_resize(int argc, char **argv)
      * resizing, so pass @exact=true.  It is of no use to report
      * success when the image has not actually been resized.
      */
-    ret = blk_truncate(blk, total_size, true, prealloc, &err);
+    ret = blk_truncate(blk, total_size, true, prealloc, false, &err);
     if (!ret) {
         qprintf(quiet, "Image resized.\n");
     } else {
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 1b7e700020..08cce9d4cf 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1715,7 +1715,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
      * exact=true.  It is better to err on the "emit more errors" side
      * than to be overly permissive.
      */
-    ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, &local_err);
+    ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, false, &local_err);
     if (ret < 0) {
         error_report_err(local_err);
         return ret;
diff --git a/tests/test-block-iothread.c b/tests/test-block-iothread.c
index 0c861809f0..339dc2c9bd 100644
--- a/tests/test-block-iothread.c
+++ b/tests/test-block-iothread.c
@@ -185,18 +185,18 @@ static void test_sync_op_truncate(BdrvChild *c)
     int ret;
 
     /* Normal success path */
-    ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, NULL);
+    ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, false, NULL);
     g_assert_cmpint(ret, ==, 0);
 
     /* Early error: Negative offset */
-    ret = bdrv_truncate(c, -2, false, PREALLOC_MODE_OFF, NULL);
+    ret = bdrv_truncate(c, -2, false, PREALLOC_MODE_OFF, false, NULL);
     g_assert_cmpint(ret, ==, -EINVAL);
 
     /* Error: Read-only image */
     c->bs->read_only = true;
     c->bs->open_flags &= ~BDRV_O_RDWR;
 
-    ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, NULL);
+    ret = bdrv_truncate(c, 65536, false, PREALLOC_MODE_OFF, false, NULL);
     g_assert_cmpint(ret, ==, -EACCES);
 
     c->bs->read_only = false;
-- 
2.20.1

Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()

[Eric Blake]

On 11/22/19 10:05 AM, Kevin Wolf wrote:
> This adds a no_fallback parameter to bdrv_co_truncate(), bdrv_truncate()
> and blk_truncate() in preparation for a fix that potentially needs to
> zero-write the new area. no_fallback will use BDRV_REQ_NO_FALLBACK for
> this operation and lets the truncate fail if an efficient zero write
> isn't possible.
> 
> Only qmp_block_resize() passes true for this parameter because it is a
> blocking monitor command, so we don't want to add more potentially slow
> I/O operations to it than we already have.
> 
> All other users will accept even a slow fallback to avoid failure.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/include/block/block.h
> @@ -347,9 +347,10 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>   void bdrv_refresh_filename(BlockDriverState *bs);
>   
>   int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
> -                                  PreallocMode prealloc, Error **errp);
> +                                  PreallocMode prealloc, bool no_fallback,
> +                                  Error **errp);
>   int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
> -                  PreallocMode prealloc, Error **errp);
> +                  PreallocMode prealloc, bool no_fallback, Error **errp);
>   

New signature, most of the changes are mechanical to pass the new 
parameter...

> +++ b/block/io.c
> @@ -3313,9 +3313,15 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
>    * If 'exact' is true, the file must be resized to exactly the given
>    * 'offset'.  Otherwise, it is sufficient for the node to be at least
>    * 'offset' bytes in length.
> + *
> + * If 'no_fallback' is true, a possibly needed writte_zeroes operation to avoid

write

> + * making a longer backing file visible will use BDRV_REQ_NO_FALLBACK. If the
> + * zero write is necessary and this flag is set, bdrv_co_truncate() will fail
> + * if efficient zero writes cannot be provided.
>    */

> +++ b/qemu-img.c
> @@ -3836,7 +3836,7 @@ static int img_resize(int argc, char **argv)
>        * resizing, so pass @exact=true.  It is of no use to report
>        * success when the image has not actually been resized.
>        */
> -    ret = blk_truncate(blk, total_size, true, prealloc, &err);
> +    ret = blk_truncate(blk, total_size, true, prealloc, false, &err);
>       if (!ret) {
>           qprintf(quiet, "Image resized.\n");
>       } else {

Hmm - thought for a future patch (not this one): are there situations 
where it may be faster to perform bulk pre-zeroing of the tail of a file 
by performing two truncates (smaller and then larger) because we know 
that just-added bytes from a truncate will read as zero?  This may be 
true for some file systems (but is not true for block devices, nor for 
things like NBD that lack resize).  Anyway, unrelated to this patch.

With the typo fixed,

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

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

Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1840 bytes --]

On 22.11.19 17:05, Kevin Wolf wrote:
> This adds a no_fallback parameter to bdrv_co_truncate(), bdrv_truncate()
> and blk_truncate() in preparation for a fix that potentially needs to
> zero-write the new area. no_fallback will use BDRV_REQ_NO_FALLBACK for
> this operation and lets the truncate fail if an efficient zero write
> isn't possible.
> 
> Only qmp_block_resize() passes true for this parameter because it is a
> blocking monitor command, so we don't want to add more potentially slow
> I/O operations to it than we already have.
> 
> All other users will accept even a slow fallback to avoid failure.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block.h          |  5 +++--
>  include/sysemu/block-backend.h |  2 +-
>  block/block-backend.c          |  4 ++--
>  block/commit.c                 |  4 ++--
>  block/crypto.c                 |  4 ++--
>  block/io.c                     | 16 ++++++++++++----
>  block/mirror.c                 |  2 +-
>  block/parallels.c              |  6 +++---
>  block/qcow.c                   |  4 ++--
>  block/qcow2-refcount.c         |  2 +-
>  block/qcow2.c                  | 19 +++++++++++--------
>  block/qed.c                    |  2 +-
>  block/raw-format.c             |  2 +-
>  block/vdi.c                    |  2 +-
>  block/vhdx-log.c               |  2 +-
>  block/vhdx.c                   |  6 +++---
>  block/vmdk.c                   | 10 ++++++----
>  block/vpc.c                    |  2 +-
>  blockdev.c                     |  2 +-
>  qemu-img.c                     |  2 +-
>  qemu-io-cmds.c                 |  2 +-
>  tests/test-block-iothread.c    |  6 +++---
>  22 files changed, 60 insertions(+), 46 deletions(-)

With the typo pointed out by Eric fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()

[Alberto Garcia]

On Fri 22 Nov 2019 05:05:05 PM CET, Kevin Wolf wrote:

> @@ -3405,6 +3412,7 @@ typedef struct TruncateCo {
>      int64_t offset;
>      bool exact;
>      PreallocMode prealloc;
> +    bool no_fallback;
>      Error **errp;
>      int ret;
>  } TruncateCo;

You add the 'no_fallback' field here...

>  int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
> -                  PreallocMode prealloc, Error **errp)
> +                  PreallocMode prealloc, bool no_fallback, Error **errp)
>  {
>      Coroutine *co;
>      TruncateCo tco = {

...but then you don't use it when the structure is initialized.

Berto

Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()

[Kevin Wolf]

Am 25.11.2019 um 16:06 hat Alberto Garcia geschrieben:
> On Fri 22 Nov 2019 05:05:05 PM CET, Kevin Wolf wrote:
> 
> > @@ -3405,6 +3412,7 @@ typedef struct TruncateCo {
> >      int64_t offset;
> >      bool exact;
> >      PreallocMode prealloc;
> > +    bool no_fallback;
> >      Error **errp;
> >      int ret;
> >  } TruncateCo;
> 
> You add the 'no_fallback' field here...
> 
> >  int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
> > -                  PreallocMode prealloc, Error **errp)
> > +                  PreallocMode prealloc, bool no_fallback, Error **errp)
> >  {
> >      Coroutine *co;
> >      TruncateCo tco = {
> 
> ...but then you don't use it when the structure is initialized.

Oops. Another proof that a series like this is too much for -rc3.

Kevin

[PATCH v3 3/8] qcow2: Declare BDRV_REQ_NO_FALLBACK supported

[Kevin Wolf]

In the common case, qcow2_co_pwrite_zeroes() already only modifies
metadata case, so we're fine with or without BDRV_REQ_NO_FALLBACK set.

The only exception is when using an external data file, where the
request is passed down to the block driver of the external data file. We
are forwarding the BDRV_REQ_NO_FALLBACK flag there, though, so this is
fine, too.

Declare the flag supported therefore.

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 b201383c3d..3fa10bf807 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1722,7 +1722,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         }
     }
 
-    bs->supported_zero_flags = header.version >= 3 ? BDRV_REQ_MAY_UNMAP : 0;
+    bs->supported_zero_flags = header.version >= 3 ?
+                               BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0;
 
     /* Repair image if dirty */
     if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
-- 
2.20.1

Re: [PATCH v3 3/8] qcow2: Declare BDRV_REQ_NO_FALLBACK supported

[Eric Blake]

On 11/22/19 10:05 AM, Kevin Wolf wrote:
> In the common case, qcow2_co_pwrite_zeroes() already only modifies
> metadata case, so we're fine with or without BDRV_REQ_NO_FALLBACK set.
> 
> The only exception is when using an external data file, where the
> request is passed down to the block driver of the external data file. We
> are forwarding the BDRV_REQ_NO_FALLBACK flag there, though, so this is
> fine, too.
> 
> Declare the flag supported therefore.
> 
> 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>

> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b201383c3d..3fa10bf807 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1722,7 +1722,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>           }
>       }
>   
> -    bs->supported_zero_flags = header.version >= 3 ? BDRV_REQ_MAY_UNMAP : 0;
> +    bs->supported_zero_flags = header.version >= 3 ?
> +                               BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK : 0;
>   
>       /* Repair image if dirty */
>       if (!(flags & (BDRV_O_CHECK | BDRV_O_INACTIVE)) && !bs->read_only &&
> 

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

Re: [PATCH v3 3/8] qcow2: Declare BDRV_REQ_NO_FALLBACK supported

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 648 bytes --]

On 22.11.19 17:05, Kevin Wolf wrote:
> In the common case, qcow2_co_pwrite_zeroes() already only modifies
> metadata case, so we're fine with or without BDRV_REQ_NO_FALLBACK set.
> 
> The only exception is when using an external data file, where the
> request is passed down to the block driver of the external data file. We
> are forwarding the BDRV_REQ_NO_FALLBACK flag there, though, so this is
> fine, too.
> 
> Declare the flag supported therefore.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [PATCH v3 3/8] qcow2: Declare BDRV_REQ_NO_FALLBACK supported

[Alberto Garcia]

On Fri 22 Nov 2019 05:05:06 PM CET, Kevin Wolf wrote:
> In the common case, qcow2_co_pwrite_zeroes() already only modifies
> metadata case, so we're fine with or without BDRV_REQ_NO_FALLBACK set.
>
> The only exception is when using an external data file, where the
> request is passed down to the block driver of the external data file. We
> are forwarding the BDRV_REQ_NO_FALLBACK flag there, though, so this is
> fine, too.
>
> Declare the flag supported therefore.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

[PATCH v3 4/8] block: truncate: Don't make backing file data visible

[Kevin Wolf]

When extending the size of an image that has a backing file larger than
its old size, make sure that the backing file data doesn't become
visible in the guest, but the added area is properly zeroed out.

Consider the following scenario where the overlay is shorter than its
backing file:

    base.qcow2:     AAAAAAAA
    overlay.qcow2:  BBBB

When resizing (extending) overlay.qcow2, the new blocks should not stay
unallocated and make the additional As from base.qcow2 visible like
before this patch, but zeros should be read.

A similar case happens with the various variants of a commit job when an
intermediate file is short (- for unallocated):

    base.qcow2:     A-A-AAAA
    mid.qcow2:      BB-B
    top.qcow2:      C--C--C-

After commit top.qcow2 to mid.qcow2, the following happens:

    mid.qcow2:      CB-C00C0 (correct result)
    mid.qcow2:      CB-C--C- (before this fix)

Without the fix, blocks that previously read as zeros on top.qcow2
suddenly turn into A.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/block/io.c b/block/io.c
index 42e7558954..61a63d9dc2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3392,12 +3392,45 @@ int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
     ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
+        goto fail_refresh_total_sectors;
     } else {
         offset = bs->total_sectors * BDRV_SECTOR_SIZE;
     }
+
+    /*
+     * If the image has a backing file that is large enough that it would
+     * provide data for the new area, we cannot leave it unallocated because
+     * then the backing file content would become visible. Instead, zero-fill
+     * the area where backing file and new area overlap.
+     *
+     * Note that if the image has a backing file, but was opened without the
+     * backing file, taking care of keeping things consistent with that backing
+     * file is the user's responsibility.
+     */
+    if (new_bytes && bs->backing && prealloc == PREALLOC_MODE_OFF) {
+        int64_t backing_len;
+
+        backing_len = bdrv_getlength(backing_bs(bs));
+        if (backing_len < 0) {
+            ret = backing_len;
+            goto fail_refresh_total_sectors;
+        }
+
+        if (backing_len > old_size) {
+            ret = bdrv_co_do_pwrite_zeroes(
+                    bs, old_size, MIN(new_bytes, backing_len - old_size),
+                    BDRV_REQ_ZERO_WRITE | BDRV_REQ_MAY_UNMAP |
+                    (no_fallback ? BDRV_REQ_NO_FALLBACK : 0));
+            if (ret < 0) {
+                goto fail_refresh_total_sectors;
+            }
+        }
+    }
+
     /* It's possible that truncation succeeded but refresh_total_sectors
      * failed, but the latter doesn't affect how we should finish the request.
      * Pass 0 as the last parameter so that dirty bitmaps etc. are handled. */
+fail_refresh_total_sectors:
     bdrv_co_write_req_finish(child, offset - new_bytes, new_bytes, &req, 0);
 
 out:
-- 
2.20.1

Re: [PATCH v3 4/8] block: truncate: Don't make backing file data visible

[Eric Blake]

On 11/22/19 10:05 AM, Kevin Wolf wrote:
> When extending the size of an image that has a backing file larger than
> its old size, make sure that the backing file data doesn't become
> visible in the guest, but the added area is properly zeroed out.
> 
> Consider the following scenario where the overlay is shorter than its
> backing file:
> 
>      base.qcow2:     AAAAAAAA
>      overlay.qcow2:  BBBB
> 
> When resizing (extending) overlay.qcow2, the new blocks should not stay
> unallocated and make the additional As from base.qcow2 visible like
> before this patch, but zeros should be read.
> 
> A similar case happens with the various variants of a commit job when an
> intermediate file is short (- for unallocated):
> 
>      base.qcow2:     A-A-AAAA
>      mid.qcow2:      BB-B
>      top.qcow2:      C--C--C-
> 
> After commit top.qcow2 to mid.qcow2, the following happens:
> 
>      mid.qcow2:      CB-C00C0 (correct result)
>      mid.qcow2:      CB-C--C- (before this fix)
> 
> Without the fix, blocks that previously read as zeros on top.qcow2
> suddenly turn into A.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)

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

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

Re: [PATCH v3 4/8] block: truncate: Don't make backing file data visible

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1300 bytes --]

On 22.11.19 17:05, Kevin Wolf wrote:
> When extending the size of an image that has a backing file larger than
> its old size, make sure that the backing file data doesn't become
> visible in the guest, but the added area is properly zeroed out.
> 
> Consider the following scenario where the overlay is shorter than its
> backing file:
> 
>     base.qcow2:     AAAAAAAA
>     overlay.qcow2:  BBBB
> 
> When resizing (extending) overlay.qcow2, the new blocks should not stay
> unallocated and make the additional As from base.qcow2 visible like
> before this patch, but zeros should be read.
> 
> A similar case happens with the various variants of a commit job when an
> intermediate file is short (- for unallocated):
> 
>     base.qcow2:     A-A-AAAA
>     mid.qcow2:      BB-B
>     top.qcow2:      C--C--C-
> 
> After commit top.qcow2 to mid.qcow2, the following happens:
> 
>     mid.qcow2:      CB-C00C0 (correct result)
>     mid.qcow2:      CB-C--C- (before this fix)
> 
> Without the fix, blocks that previously read as zeros on top.qcow2
> suddenly turn into A.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

[PATCH v3 5/8] iotests: Add qemu_io_log()

[Kevin Wolf]

Add a function that runs qemu-io and logs the output with the
appropriate filters applied.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/iotests.py | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index df0708923d..fc78852ae5 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -162,6 +162,11 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
+def qemu_io_log(*args):
+    result = qemu_io(*args)
+    log(result, filters=[filter_testfiles, filter_qemu_io])
+    return result
+
 def qemu_io_silent(*args):
     '''Run qemu-io and return the exit code, suppressing stdout'''
     args = qemu_io_args + list(args)
-- 
2.20.1

[PATCH v3 6/8] iotests: Fix timeout in run_job()

[Kevin Wolf]

run_job() accepts a wait parameter for a timeout, but it doesn't
actually use it. The only thing that is missing is passing it to
events_wait(), so do that now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index fc78852ae5..0ac3ad4b04 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -609,7 +609,7 @@ class VM(qtest.QEMUQtestMachine):
         ]
         error = None
         while True:
-            ev = filter_qmp_event(self.events_wait(events))
+            ev = filter_qmp_event(self.events_wait(events, timeout=wait))
             if ev['event'] != 'JOB_STATUS_CHANGE':
                 if use_log:
                     log(ev)
-- 
2.20.1

[PATCH v3 7/8] iotests: Support job-complete in run_job()

[Kevin Wolf]

Automatically complete jobs that have a 'ready' state and need an
explicit job-complete. Without this, run_job() would hang for such
jobs.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/iotests.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 0ac3ad4b04..b46d298766 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -622,6 +622,8 @@ class VM(qtest.QEMUQtestMachine):
                         error = j['error']
                         if use_log:
                             log('Job failed: %s' % (j['error']))
+            elif status == 'ready':
+                self.qmp_log('job-complete', id=job)
             elif status == 'pending' and not auto_finalize:
                 if pre_finalize:
                     pre_finalize()
-- 
2.20.1

[PATCH v3 8/8] iotests: Test committing to short backing file

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/274        | 152 +++++++++++++++++++++++++
 tests/qemu-iotests/274.out    | 203 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |   2 +-
 4 files changed, 357 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/274
 create mode 100644 tests/qemu-iotests/274.out

diff --git a/tests/qemu-iotests/274 b/tests/qemu-iotests/274
new file mode 100755
index 0000000000..7b238f41da
--- /dev/null
+++ b/tests/qemu-iotests/274
@@ -0,0 +1,152 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# Creator/Owner: Kevin Wolf <kwolf@redhat.com>
+#
+# Some tests for short backing files and short overlays
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+size_short = 1 * 1024 * 1024
+size_long = 2 * 1024 * 1024
+size_diff = size_long - size_short
+
+def create_chain():
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, base,
+                         str(size_long))
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, mid,
+                         str(size_short))
+    iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', mid, top,
+                         str(size_long))
+
+    iotests.qemu_io_log('-c', 'write -P 1 0 %d' % size_long, base)
+
+def create_vm():
+    vm = iotests.VM()
+    vm.add_blockdev('file,filename=%s,node-name=base-file' % (base))
+    vm.add_blockdev('%s,file=base-file,node-name=base' % (iotests.imgfmt))
+    vm.add_blockdev('file,filename=%s,node-name=mid-file' % (mid))
+    vm.add_blockdev('%s,file=mid-file,node-name=mid,backing=base' % (iotests.imgfmt))
+    vm.add_drive(top, 'backing=mid,node-name=top')
+    return vm
+
+with iotests.FilePath('base') as base, \
+     iotests.FilePath('mid') as mid, \
+     iotests.FilePath('top') as top:
+
+    iotests.log('== Commit tests ==')
+
+    create_chain()
+
+    iotests.log('=== Check visible data ===')
+
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, top)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), top)
+
+    iotests.log('=== Checking allocation status ===')
+
+    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+                        '-c', 'alloc %d %d' % (size_short, size_diff),
+                        base)
+
+    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+                        '-c', 'alloc %d %d' % (size_short, size_diff),
+                        mid)
+
+    iotests.qemu_io_log('-c', 'alloc 0 %d' % size_short,
+                        '-c', 'alloc %d %d' % (size_short, size_diff),
+                        top)
+
+    iotests.log('=== Checking map ===')
+
+    iotests.qemu_img_log('map', '--output=json', base)
+    iotests.qemu_img_log('map', '--output=human', base)
+    iotests.qemu_img_log('map', '--output=json', mid)
+    iotests.qemu_img_log('map', '--output=human', mid)
+    iotests.qemu_img_log('map', '--output=json', top)
+    iotests.qemu_img_log('map', '--output=human', top)
+
+    iotests.log('=== Testing qemu-img commit (top -> mid) ===')
+
+    iotests.qemu_img_log('commit', top)
+    iotests.img_info_log(mid)
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+    iotests.log('=== Testing HMP commit (top -> mid) ===')
+
+    create_chain()
+    with create_vm() as vm:
+        vm.launch()
+        vm.qmp_log('human-monitor-command', command_line='commit drive0')
+
+    iotests.img_info_log(mid)
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+    iotests.log('=== Testing QMP active commit (top -> mid) ===')
+
+    create_chain()
+    with create_vm() as vm:
+        vm.launch()
+        vm.qmp_log('block-commit', device='top', base_node='mid',
+                   job_id='job0', auto_dismiss=False)
+        vm.run_job('job0', wait=5)
+
+    iotests.img_info_log(mid)
+    iotests.qemu_io_log('-c', 'read -P 1 0 %d' % size_short, mid)
+    iotests.qemu_io_log('-c', 'read -P 0 %d %d' % (size_short, size_diff), mid)
+
+
+    iotests.log('== Resize tests ==')
+
+    # Use different sizes for different allocation modes:
+    #
+    # We want to have at least one test where 32 bit truncation in the size of
+    # the overlapping area becomes visible. This is covered by the
+    # prealloc='off' case (1G to 6G is an overlap of 5G).
+    #
+    # However, we can only do this for modes that don't preallocate data
+    # because otherwise we might run out of space on the test host.
+    for (prealloc, base_size, top_size_old, top_size_new, off)  in [
+            ('off',       '6G',  '1G',  '8G',  '5G'),
+            ('metadata', '32G', '30G', '33G', '31G'),
+            ('falloc',   '10M',  '5M', '15M',  '9M'),
+            ('full',     '16M',  '8M', '12M', '11M')]:
+
+        iotests.log('=== preallocation=%s ===' % prealloc)
+        iotests.qemu_img_log('create', '-f', iotests.imgfmt, base, base_size)
+        iotests.qemu_img_log('create', '-f', iotests.imgfmt, '-b', base, top,
+                             top_size_old)
+        iotests.qemu_io_log('-c', 'write -P 1 %s 64k' % off, base)
+
+        # After this, 0 to base_size should be allocated/zeroed
+        # base_size to top_size_new should be unallocated with
+        # preallocation=off and allocated with preallocation enabled
+        iotests.qemu_img_log('resize', '-f', iotests.imgfmt,
+                             '--preallocation', prealloc, top, top_size_new)
+        iotests.qemu_io_log('-c', 'read -P 0 %s 64k' % off, top)
+
+        # Metadata preallocation doesn't have a defined result on the file
+        # system level with respect to holes, so skip it here
+        iotests.qemu_io_log('-c', 'map', top)
+        if prealloc != 'metadata':
+            iotests.qemu_img_log('map', '--output=json', top)
diff --git a/tests/qemu-iotests/274.out b/tests/qemu-iotests/274.out
new file mode 100644
index 0000000000..b18a69fd42
--- /dev/null
+++ b/tests/qemu-iotests/274.out
@@ -0,0 +1,203 @@
+== Commit tests ==
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Check visible data ===
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Checking allocation status ===
+1048576/1048576 bytes allocated at offset 0 bytes
+1048576/1048576 bytes allocated at offset 1 MiB
+
+0/1048576 bytes allocated at offset 0 bytes
+0/0 bytes allocated at offset 1 MiB
+
+0/1048576 bytes allocated at offset 0 bytes
+0/1048576 bytes allocated at offset 1 MiB
+
+=== Checking map ===
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+
+Offset          Length          Mapped to       File
+0               0x200000        0x50000         TEST_DIR/PID-base
+
+[{ "start": 0, "length": 1048576, "depth": 1, "zero": false, "data": true, "offset": 327680}]
+
+Offset          Length          Mapped to       File
+0               0x100000        0x50000         TEST_DIR/PID-base
+
+[{ "start": 0, "length": 1048576, "depth": 2, "zero": false, "data": true, "offset": 327680},
+{ "start": 1048576, "length": 1048576, "depth": 0, "zero": true, "data": false}]
+
+Offset          Length          Mapped to       File
+0               0x100000        0x50000         TEST_DIR/PID-base
+
+=== Testing qemu-img commit (top -> mid) ===
+Image committed.
+
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing HMP commit (top -> mid) ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+{"execute": "human-monitor-command", "arguments": {"command-line": "commit drive0"}}
+{"return": ""}
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing QMP active commit (top -> mid) ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-mid', fmt=qcow2 size=1048576 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=2097152 backing_file=TEST_DIR/PID-mid cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+{"execute": "block-commit", "arguments": {"auto-dismiss": false, "base-node": "mid", "device": "top", "job-id": "job0"}}
+{"return": {}}
+{"execute": "job-complete", "arguments": {"id": "job0"}}
+{"return": {}}
+{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_READY", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"data": {"device": "job0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+{"execute": "job-dismiss", "arguments": {"id": "job0"}}
+{"return": {}}
+image: TEST_IMG
+file format: IMGFMT
+virtual size: 2 MiB (2097152 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/PID-base
+Format specific information:
+    compat: 1.1
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== Resize tests ==
+=== preallocation=off ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=6442450944 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=1073741824 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 65536/65536 bytes at offset 5368709120
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Image resized.
+
+read 65536/65536 bytes at offset 5368709120
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+1 GiB (0x40000000) bytes not allocated at offset 0 bytes (0x0)
+5 GiB (0x140000000) bytes     allocated at offset 1 GiB (0x40000000)
+2 GiB (0x80000000) bytes not allocated at offset 6 GiB (0x180000000)
+
+[{ "start": 0, "length": 1073741824, "depth": 1, "zero": true, "data": false},
+{ "start": 1073741824, "length": 7516192768, "depth": 0, "zero": true, "data": false}]
+
+=== preallocation=metadata ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=34359738368 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=32212254720 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 65536/65536 bytes at offset 33285996544
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Image resized.
+
+read 65536/65536 bytes at offset 33285996544
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+30 GiB (0x780000000) bytes not allocated at offset 0 bytes (0x0)
+3 GiB (0xc0000000) bytes     allocated at offset 30 GiB (0x780000000)
+
+=== preallocation=falloc ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=10485760 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=5242880 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 65536/65536 bytes at offset 9437184
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Image resized.
+
+read 65536/65536 bytes at offset 9437184
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+5 MiB (0x500000) bytes not allocated at offset 0 bytes (0x0)
+10 MiB (0xa00000) bytes     allocated at offset 5 MiB (0x500000)
+
+[{ "start": 0, "length": 5242880, "depth": 1, "zero": true, "data": false},
+{ "start": 5242880, "length": 10485760, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+
+=== preallocation=full ===
+Formatting 'TEST_DIR/PID-base', fmt=qcow2 size=16777216 cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+Formatting 'TEST_DIR/PID-top', fmt=qcow2 size=8388608 backing_file=TEST_DIR/PID-base cluster_size=65536 lazy_refcounts=off refcount_bits=16
+
+wrote 65536/65536 bytes at offset 11534336
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+Image resized.
+
+read 65536/65536 bytes at offset 11534336
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+8 MiB (0x800000) bytes not allocated at offset 0 bytes (0x0)
+4 MiB (0x400000) bytes     allocated at offset 8 MiB (0x800000)
+
+[{ "start": 0, "length": 8388608, "depth": 1, "zero": true, "data": false},
+{ "start": 8388608, "length": 4194304, "depth": 0, "zero": false, "data": true, "offset": 327680}]
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6b10a6a762..b377a17e78 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -285,4 +285,5 @@
 270 rw backing quick
 272 rw
 273 backing quick
+274 rw backing
 277 rw quick
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b46d298766..9135dd52b6 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -149,7 +149,7 @@ def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
     output = qemu_img_pipe(*args)
     if not filter_path:
         filter_path = filename
-    log(filter_img_info(output, filter_path))
+    log(filter_img_info(output, filter_path), filters=[filter_testfiles])
 
 def qemu_io(*args):
     '''Run qemu-io and return the stdout data'''
-- 
2.20.1

Re: [PATCH v3 8/8] iotests: Test committing to short backing file

[Eric Blake]

On 11/22/19 10:05 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/274        | 152 +++++++++++++++++++++++++
>   tests/qemu-iotests/274.out    | 203 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/group      |   1 +
>   tests/qemu-iotests/iotests.py |   2 +-
>   4 files changed, 357 insertions(+), 1 deletion(-)
>   create mode 100755 tests/qemu-iotests/274
>   create mode 100644 tests/qemu-iotests/274.out
> 

> +    iotests.log('== Resize tests ==')
> +
> +    # Use different sizes for different allocation modes:
> +    #
> +    # We want to have at least one test where 32 bit truncation in the size of
> +    # the overlapping area becomes visible. This is covered by the
> +    # prealloc='off' case (1G to 6G is an overlap of 5G).
> +    #
> +    # However, we can only do this for modes that don't preallocate data
> +    # because otherwise we might run out of space on the test host.
> +    for (prealloc, base_size, top_size_old, top_size_new, off)  in [
> +            ('off',       '6G',  '1G',  '8G',  '5G'),
> +            ('metadata', '32G', '30G', '33G', '31G'),
> +            ('falloc',   '10M',  '5M', '15M',  '9M'),
> +            ('full',     '16M',  '8M', '12M', '11M')]:

The changes since v3 make sense.

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

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

Re: [PATCH v3 8/8] iotests: Test committing to short backing file

[Vladimir Sementsov-Ogievskiy]

22.11.2019 19:05, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH v3 8/8] iotests: Test committing to short backing file

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1541 bytes --]

On 22.11.19 17:05, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/274        | 152 +++++++++++++++++++++++++
>  tests/qemu-iotests/274.out    | 203 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group      |   1 +
>  tests/qemu-iotests/iotests.py |   2 +-
>  4 files changed, 357 insertions(+), 1 deletion(-)
>  create mode 100755 tests/qemu-iotests/274
>  create mode 100644 tests/qemu-iotests/274.out

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index b46d298766..9135dd52b6 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -149,7 +149,7 @@ def img_info_log(filename, filter_path=None, imgopts=False, extra_args=[]):
>      output = qemu_img_pipe(*args)
>      if not filter_path:
>          filter_path = filename
> -    log(filter_img_info(output, filter_path))
> +    log(filter_img_info(output, filter_path), filters=[filter_testfiles])

This doesn’t work when the path contains $IMGFMT.  The test dir filter
must then run before the image format filter.  For example, I usually
run the tests in /tmp/test-${IMGFMT}-${IMGPROTO} (i.e.
/tmp/test-qcow2-file for -qcow2), and this test fails for me:

@@ -49,7 +49,7 @@
 file format: IMGFMT
 virtual size: 2 MiB (2097152 bytes)
 cluster_size: 65536
-backing file: TEST_DIR/PID-base
+backing file: /tmp/test-IMGFMT-file/40083-base
 Format specific information:
     compat: 1.1
     lazy refcounts: false

Max


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

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Peter Maydell]

On Fri, 22 Nov 2019 at 16:08, Kevin Wolf <kwolf@redhat.com> wrote:
>
> See patch 4 for the description of the bug fixed.

I guess my questions for trying to answer the "for-4.2?"
question in the subject are:
 1) is this a security (leaking data into the guest) bug ?
 2) is this a regression?
 3) is this something a lot of people are likely to run into?

Eyeballing of the diffstat plus the fact we're on v4 of
the patchset already makes me a little uneasy about
putting it into rc3, but if the bug we're fixing matters
enough we can do it.

thanks
-- PMM

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Eric Blake]

On 11/22/19 10:17 AM, Peter Maydell wrote:
> On Fri, 22 Nov 2019 at 16:08, Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> See patch 4 for the description of the bug fixed.
> 
> I guess my questions for trying to answer the "for-4.2?"
> question in the subject are:
>   1) is this a security (leaking data into the guest) bug ?
>   2) is this a regression?
>   3) is this something a lot of people are likely to run into?

My thoughts (although Kevin's may be more definitive):

1) yes, there is a security aspect: certain resize or commit actions can 
result in the guest seeing a revival of stale data that the guest may 
have thought that it previously scrubbed.  Similarly, the tail end of 
the series proves via iotests that we have an actual case of data 
corruption after a block commit without this patch

2) no, this is a long-standing bug, we've only recently noticed it

3) no, it is uncommon to have an overlay with a size shorter than its 
backing file (it's not even all that common to have an overlay longer 
than the backing file), so this is a corner case not many people will 
hit.  It's even less common to have the difference in overlay sizes also 
coincide with formats that introduce the speed penalty of a longer 
blocking due to the added zeroing.

> 
> Eyeballing of the diffstat plus the fact we're on v4 of
> the patchset already makes me a little uneasy about
> putting it into rc3, but if the bug we're fixing matters
> enough we can do it.

In terms of diffstat, the v3 series was much smaller in impact.  Both 
versions add robustness, where the difference between v3 and v4 is 
whether we introduce a speed penalty on an unlikely setup (v3) or reject 
any operation where it would require a speed penalty to avoid data 
problems (v4).  I think all the patches in v3 were reviewed, but I'll go 
ahead and review v4 as well.

Because of point 1, I am leaning towards some version of this patch 
series (whether 3 or 4) making -rc3; but point 2 (it is not a 4.2 
regression) also seems to be a reasonable justification for slipping 
this to 5.0.

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

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 858 bytes --]

On 22.11.19 17:41, Eric Blake wrote:
> On 11/22/19 10:17 AM, Peter Maydell wrote:

[...]

>> Eyeballing of the diffstat plus the fact we're on v4 of
>> the patchset already makes me a little uneasy about
>> putting it into rc3, but if the bug we're fixing matters
>> enough we can do it.
> 
> In terms of diffstat, the v3 series was much smaller in impact.  Both
> versions add robustness, where the difference between v3 and v4 is
> whether we introduce a speed penalty on an unlikely setup (v3) or reject
> any operation where it would require a speed penalty to avoid data
> problems (v4).

I’d just like to add that this isn’t just about a speed penalty, but
about the fact that the monitor is blocked while the operation is
running.  So the speed penalty has more impact than just some background
operation being slow.

Max


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

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1019 bytes --]

On 22.11.19 17:05, Kevin Wolf wrote:
> See patch 4 for the description of the bug fixed.
> 
> v3:
> - Don't allow blocking the monitor for a zero write in block_resize
>   (even though we can already blockfor other reasons there). This is
>   mainly responsible for the increased complexity compared to v2.
>   Personally, I think this is not an improvement over v2, but if this is
>   what it takes to fix a corruption issue in 4.2... [Max]

I don’t find it so bad because the added complexity is:

(1) A mainly mechanical change of code to add another parameter to
{blk,bdrv}(_co)?_truncate(),

(2) qcow2 providing BDRV_REQ_NO_FALLBACK, and

(3) passing BDRV_REQ_NO_FALLBACK in bdrv_co_truncate() if the new
parameter is true.

(1) sees the most LoC changed, but it isn’t a complex change.  (2) and
(3) are both basically one-line changes each.


OTOH, as I’ve said on IRC, I believe you have a sufficient number of
R-bs on v2 to take it without mine, so the choice is yours.

Max


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

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Kevin Wolf]

Am 22.11.2019 um 17:05 hat Kevin Wolf geschrieben:
> See patch 4 for the description of the bug fixed.

I'm applying patches 3 and 5-7 to the block branch because they make
sense on their own.

The real fix will need another approach because the error handling is
broken in this one: If zeroing out fails (either because of NO_FALLBACK
or because of some other I/O error), bdrv_co_truncate() will return
failure, but the image size has already been increased, with potentially
incorrect data in the new area.

To fix this, we need to make sure that zeros will be read before we
commit the new image size to the image file (e.g. qcow2 header) and to
bs->total_sectors. In other words, it must become the responsibility of
the block driver.

To this effect, I'm planning to introduce a PREALLOC_MODE_ZERO_INIT flag
that can be or'ed to the preallocation mode. This will fail by default
because it looks like just another unimplemented preallocation mode to
block drivers. It will be requested explicitly by commit jobs and
automatically added by bdrv_co_truncate() if the backing file would
become visible (like in this series, but now for all preallocation
modes). I'm planning to implement it for qcow2 and file-posix for now,
which should cover most interesting cases.

Does this make sense to you?

Kevin

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1449 bytes --]

On 10.12.19 18:46, Kevin Wolf wrote:
> Am 22.11.2019 um 17:05 hat Kevin Wolf geschrieben:
>> See patch 4 for the description of the bug fixed.
> 
> I'm applying patches 3 and 5-7 to the block branch because they make
> sense on their own.
> 
> The real fix will need another approach because the error handling is
> broken in this one: If zeroing out fails (either because of NO_FALLBACK
> or because of some other I/O error), bdrv_co_truncate() will return
> failure, but the image size has already been increased, with potentially
> incorrect data in the new area.
> 
> To fix this, we need to make sure that zeros will be read before we
> commit the new image size to the image file (e.g. qcow2 header) and to
> bs->total_sectors. In other words, it must become the responsibility of
> the block driver.
> 
> To this effect, I'm planning to introduce a PREALLOC_MODE_ZERO_INIT flag
> that can be or'ed to the preallocation mode. This will fail by default
> because it looks like just another unimplemented preallocation mode to
> block drivers. It will be requested explicitly by commit jobs and
> automatically added by bdrv_co_truncate() if the backing file would
> become visible (like in this series, but now for all preallocation
> modes). I'm planning to implement it for qcow2 and file-posix for now,
> which should cover most interesting cases.
> 
> Does this make sense to you?

Sounds good to me.

Max


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

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Vladimir Sementsov-Ogievskiy]

10.12.2019 20:46, Kevin Wolf wrote:
> Am 22.11.2019 um 17:05 hat Kevin Wolf geschrieben:
>> See patch 4 for the description of the bug fixed.
> 
> I'm applying patches 3 and 5-7 to the block branch because they make
> sense on their own.
> 
> The real fix will need another approach because the error handling is
> broken in this one: If zeroing out fails (either because of NO_FALLBACK
> or because of some other I/O error), bdrv_co_truncate() will return
> failure, but the image size has already been increased, with potentially
> incorrect data in the new area.
> 
> To fix this, we need to make sure that zeros will be read before we
> commit the new image size to the image file (e.g. qcow2 header) and to
> bs->total_sectors. In other words, it must become the responsibility of
> the block driver.
> 
> To this effect, I'm planning to introduce a PREALLOC_MODE_ZERO_INIT flag
> that can be or'ed to the preallocation mode. This will fail by default
> because it looks like just another unimplemented preallocation mode to
> block drivers. It will be requested explicitly by commit jobs and
> automatically added by bdrv_co_truncate() if the backing file would
> become visible (like in this series, but now for all preallocation
> modes). I'm planning to implement it for qcow2 and file-posix for now,
> which should cover most interesting cases.
> 
> Does this make sense to you?
> 

This should work. Do you still have this plan in a timeline?


-- 
Best regards,
Vladimir

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Kevin Wolf]

Am 19.12.2019 um 10:24 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 10.12.2019 20:46, Kevin Wolf wrote:
> > Am 22.11.2019 um 17:05 hat Kevin Wolf geschrieben:
> >> See patch 4 for the description of the bug fixed.
> > 
> > I'm applying patches 3 and 5-7 to the block branch because they make
> > sense on their own.
> > 
> > The real fix will need another approach because the error handling is
> > broken in this one: If zeroing out fails (either because of NO_FALLBACK
> > or because of some other I/O error), bdrv_co_truncate() will return
> > failure, but the image size has already been increased, with potentially
> > incorrect data in the new area.
> > 
> > To fix this, we need to make sure that zeros will be read before we
> > commit the new image size to the image file (e.g. qcow2 header) and to
> > bs->total_sectors. In other words, it must become the responsibility of
> > the block driver.
> > 
> > To this effect, I'm planning to introduce a PREALLOC_MODE_ZERO_INIT flag
> > that can be or'ed to the preallocation mode. This will fail by default
> > because it looks like just another unimplemented preallocation mode to
> > block drivers. It will be requested explicitly by commit jobs and
> > automatically added by bdrv_co_truncate() if the backing file would
> > become visible (like in this series, but now for all preallocation
> > modes). I'm planning to implement it for qcow2 and file-posix for now,
> > which should cover most interesting cases.
> > 
> > Does this make sense to you?
> 
> This should work. Do you still have this plan in a timeline?

Still planning to do this, but tomorrow is my last working day for this
year. So I guess I'll get to it sometime in January.

Kevin

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Vladimir Sementsov-Ogievskiy]

19.12.2019 13:13, Kevin Wolf wrote:
> Am 19.12.2019 um 10:24 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 10.12.2019 20:46, Kevin Wolf wrote:
>>> Am 22.11.2019 um 17:05 hat Kevin Wolf geschrieben:
>>>> See patch 4 for the description of the bug fixed.
>>>
>>> I'm applying patches 3 and 5-7 to the block branch because they make
>>> sense on their own.
>>>
>>> The real fix will need another approach because the error handling is
>>> broken in this one: If zeroing out fails (either because of NO_FALLBACK
>>> or because of some other I/O error), bdrv_co_truncate() will return
>>> failure, but the image size has already been increased, with potentially
>>> incorrect data in the new area.
>>>
>>> To fix this, we need to make sure that zeros will be read before we
>>> commit the new image size to the image file (e.g. qcow2 header) and to
>>> bs->total_sectors. In other words, it must become the responsibility of
>>> the block driver.
>>>
>>> To this effect, I'm planning to introduce a PREALLOC_MODE_ZERO_INIT flag
>>> that can be or'ed to the preallocation mode. This will fail by default
>>> because it looks like just another unimplemented preallocation mode to
>>> block drivers. It will be requested explicitly by commit jobs and
>>> automatically added by bdrv_co_truncate() if the backing file would
>>> become visible (like in this series, but now for all preallocation
>>> modes). I'm planning to implement it for qcow2 and file-posix for now,
>>> which should cover most interesting cases.
>>>
>>> Does this make sense to you?
>>
>> This should work. Do you still have this plan in a timeline?
> 
> Still planning to do this, but tomorrow is my last working day for this
> year. So I guess I'll get to it sometime in January.
> 

Good. Have a nice holiday!


-- 
Best regards,
Vladimir

Re: [PATCH for-4.2? v3 0/8] block: Fix resize (extending) of short overlays

[Vladimir Sementsov-Ogievskiy]

19.12.2019 13:20, Vladimir Sementsov-Ogievskiy wrote:
> 19.12.2019 13:13, Kevin Wolf wrote:
>> Am 19.12.2019 um 10:24 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>> 10.12.2019 20:46, Kevin Wolf wrote:
>>>> Am 22.11.2019 um 17:05 hat Kevin Wolf geschrieben:
>>>>> See patch 4 for the description of the bug fixed.
>>>>
>>>> I'm applying patches 3 and 5-7 to the block branch because they make
>>>> sense on their own.
>>>>
>>>> The real fix will need another approach because the error handling is
>>>> broken in this one: If zeroing out fails (either because of NO_FALLBACK
>>>> or because of some other I/O error), bdrv_co_truncate() will return
>>>> failure, but the image size has already been increased, with potentially
>>>> incorrect data in the new area.
>>>>
>>>> To fix this, we need to make sure that zeros will be read before we
>>>> commit the new image size to the image file (e.g. qcow2 header) and to
>>>> bs->total_sectors. In other words, it must become the responsibility of
>>>> the block driver.
>>>>
>>>> To this effect, I'm planning to introduce a PREALLOC_MODE_ZERO_INIT flag
>>>> that can be or'ed to the preallocation mode. This will fail by default
>>>> because it looks like just another unimplemented preallocation mode to
>>>> block drivers. It will be requested explicitly by commit jobs and
>>>> automatically added by bdrv_co_truncate() if the backing file would
>>>> become visible (like in this series, but now for all preallocation
>>>> modes). I'm planning to implement it for qcow2 and file-posix for now,
>>>> which should cover most interesting cases.
>>>>
>>>> Does this make sense to you?
>>>
>>> This should work. Do you still have this plan in a timeline?
>>
>> Still planning to do this, but tomorrow is my last working day for this
>> year. So I guess I'll get to it sometime in January.
>>
> 
> Good. Have a nice holiday!
> 
> 

Hi, didn't you forget? I just going to ping (or resend) my related
"[PATCH 0/4] fix & merge block_status_above and is_allocated_above", so,
pinging these patches too...

-- 
Best regards,
Vladimir