[pve-devel] [PATCH-SERIES v2 qemu] vma/alloc-track improvements

[pve-devel] [PATCH-SERIES v2 qemu] vma/alloc-track improvements

[Fabian Ebner]

Most important is the first patch, as it fixes backing up templates
with efitype=4m EFI disk on file storages (or other 64-KiB-unaligned
disk) to non-PBS. Also reported in the community forum, most recently:
https://forum.proxmox.com/threads/104740/post-479298

Second vma-related patch is to be more graceful in error scenario upon
creation. alloc-track patch is to avoid a bogus break, but might even
be dead code in practice.

vma-related patches can be squashed into
0026-PVE-Backup-add-vma-backup-format-code.patch

alloca-track patch can be squashed into
0047-block-add-alloc-track-driver.patch

Fabian Ebner (3):
  vma: create: support 64KiB-unaligned input images
  vma: create: avoid triggering assertion in error case
  block: alloc-track: avoid premature break

 block/alloc-track.c |  3 ++-
 vma-writer.c        |  2 ++
 vma.c               | 12 ++++++++++--
 3 files changed, 14 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 qemu 1/3] vma: create: support 64KiB-unaligned input images

[Fabian Ebner]

which fixes backing up templates with such disks in PVE, for example
efitype=4m EFI disks on a file-based storage (size = 540672).

If there is not enough left to read, blk_co_preadv will return -EIO,
so limit the size in the last iteration.

For writing, an unaligned end is already handled correctly.

The call to memset is not strictly necessary, because writing also
checks that it doesn't write data beyond the end of the image. But
there are two reasons to do it:
1. It's cleaner that way.
2. It allows detecting when the final piece is all zeroes, which might
   not happen if the buffer still contains data from the previous
   iteration.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

Changes from v1:
    * Add assert just to be sure.

 vma.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/vma.c b/vma.c
index 21e765a469..6d02b29047 100644
--- a/vma.c
+++ b/vma.c
@@ -548,7 +548,7 @@ static void coroutine_fn backup_run(void *opaque)
     struct iovec iov;
     QEMUIOVector qiov;
 
-    int64_t start, end;
+    int64_t start, end, readlen;
     int ret = 0;
 
     unsigned char *buf = blk_blockalign(job->target, VMA_CLUSTER_SIZE);
@@ -562,8 +562,16 @@ static void coroutine_fn backup_run(void *opaque)
         iov.iov_len = VMA_CLUSTER_SIZE;
         qemu_iovec_init_external(&qiov, &iov, 1);
 
+        if (start + 1 == end) {
+            memset(buf, 0, VMA_CLUSTER_SIZE);
+            readlen = job->len - start * VMA_CLUSTER_SIZE;
+            assert(readlen > 0 && readlen <= VMA_CLUSTER_SIZE);
+        } else {
+            readlen = VMA_CLUSTER_SIZE;
+        }
+
         ret = blk_co_preadv(job->target, start * VMA_CLUSTER_SIZE,
-                            VMA_CLUSTER_SIZE, &qiov, 0);
+                            readlen, &qiov, 0);
         if (ret < 0) {
             vma_writer_set_error(job->vmaw, "read error", -1);
             goto out;
-- 
2.30.2

[pve-devel] [PATCH v2 qemu 2/3] vma: create: avoid triggering assertion in error case

[Fabian Ebner]

error_setg expects its argument to not be initialized yet.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 vma-writer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/vma-writer.c b/vma-writer.c
index 11d8321ffd..29567cba68 100644
--- a/vma-writer.c
+++ b/vma-writer.c
@@ -310,6 +310,8 @@ VmaWriter *vma_writer_create(const char *filename, uuid_t uuid, Error **errp)
         }
 
         if (vmaw->fd < 0) {
+            error_free(*errp);
+            *errp = NULL;
             error_setg(errp, "can't open file %s - %s\n", filename,
                        g_strerror(errno));
             goto err;
-- 
2.30.2

[pve-devel] [PATCH v2 qemu 3/3] block: alloc-track: avoid premature break

[Fabian Ebner]

While the bdrv_co_preadv() calls are expected to return 0 on success,
qemu_iovec_memset() will return the number of bytes set (will be
local_bytes, because the slice with that size was just initialized).

Don't break out of the loop after the branch with qemu_iovec_memset(),
because there might still be work to do. Additionally, ret is an int,
which on 64-bit platforms is too small to hold the size_t returned by
qemu_iovec_memset().

The branch seems to be difficult to reach in practice, because the
whole point of alloc-track is to be used with a backing device.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

New in v2.

 block/alloc-track.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/alloc-track.c b/block/alloc-track.c
index 6b50fbe537..c1160af04b 100644
--- a/block/alloc-track.c
+++ b/block/alloc-track.c
@@ -174,7 +174,8 @@ static int coroutine_fn track_co_preadv(BlockDriverState *bs,
             ret = bdrv_co_preadv(bs->backing, local_offset, local_bytes,
                                  &local_qiov, flags);
         } else {
-            ret = qemu_iovec_memset(&local_qiov, cur_offset, 0, local_bytes);
+            qemu_iovec_memset(&local_qiov, cur_offset, 0, local_bytes);
+            ret = 0;
         }
 
         if (ret != 0) {
-- 
2.30.2

[pve-devel] applied: [PATCH-SERIES v2 qemu] vma/alloc-track improvements

[Thomas Lamprecht]

Am 22/06/2022 um 10:45 schrieb Fabian Ebner:
> Most important is the first patch, as it fixes backing up templates
> with efitype=4m EFI disk on file storages (or other 64-KiB-unaligned
> disk) to non-PBS. Also reported in the community forum, most recently:
> https://forum.proxmox.com/threads/104740/post-479298
> 
> Second vma-related patch is to be more graceful in error scenario upon
> creation. alloc-track patch is to avoid a bogus break, but might even
> be dead code in practice.
> 
> vma-related patches can be squashed into
> 0026-PVE-Backup-add-vma-backup-format-code.patch
> 
> alloca-track patch can be squashed into
> 0047-block-add-alloc-track-driver.patch
> 
> Fabian Ebner (3):
>   vma: create: support 64KiB-unaligned input images
>   vma: create: avoid triggering assertion in error case
>   block: alloc-track: avoid premature break
> 
>  block/alloc-track.c |  3 ++-
>  vma-writer.c        |  2 ++
>  vma.c               | 12 ++++++++++--
>  3 files changed, 14 insertions(+), 3 deletions(-)
> 

applied, thanks!