[pbs-devel] [PATCH qemu] PVE: handle PBS write callback with big blocks correctly

[pbs-devel] [PATCH qemu] PVE: handle PBS write callback with big blocks correctly

[Stefan Reiter]

Under certain conditions QEMU will push more than the given blocksize
into the callback at once. Handle it like VMA does, by iterating the
data in PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE (or smaller, for last one)
sized blocks.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

As briefly tested by Fabian, it seems to fix the original issue.

 pve-backup.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/pve-backup.c b/pve-backup.c
index 77eb475563..4d423611e1 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -147,17 +147,29 @@ pvebackup_co_dump_pbs_cb(
         return -1;
     }
 
-    pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id, buf, start, size, &local_err);
-    qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
+    uint64_t transferred = 0;
+    uint64_t reused = 0;
+    while (transferred < size) {
+        uint64_t left = size - transferred;
+        uint64_t to_transfer = left < PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE ?
+            left : PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE;
 
-    if (pbs_res < 0) {
-        pvebackup_propagate_error(local_err);
-        return pbs_res;
-    } else {
-        size_t reused = (pbs_res == 0) ? size : 0;
-        pvebackup_add_transfered_bytes(size, !buf ? size : 0, reused);
+        pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id,
+            buf ? buf + transferred : NULL, start + transferred, to_transfer, &local_err);
+        transferred += to_transfer;
+
+        if (pbs_res < 0) {
+            pvebackup_propagate_error(local_err);
+            qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
+            return pbs_res;
+        }
+
+        reused += pbs_res == 0 ? to_transfer : 0;
     }
 
+    qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
+    pvebackup_add_transfered_bytes(size, !buf ? size : 0, reused);
+
     return size;
 }
 
-- 
2.20.1

Re: [pbs-devel] [PATCH qemu] PVE: handle PBS write callback with big blocks correctly

[Fabian Grünbichler]

On July 14, 2020 3:17 pm, Stefan Reiter wrote:
> Under certain conditions QEMU will push more than the given blocksize
> into the callback at once. Handle it like VMA does, by iterating the
> data in PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE (or smaller, for last one)
> sized blocks.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> As briefly tested by Fabian, it seems to fix the original issue.

Tested and it fixes the issue (patched smaller fixed chunk size + I/O 
pressure from within the guest), but it only works for the currently 
hard-coded case of chunk_size == PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE.

Given that we likely only trigger this when/if we make chunk size 
configurable by the caller, we either need to handle this inside 
libproxmox-backup-qemu (where we have the configured chunk size 
available), or expose the configured chunk size to Qemu throughout.

We can still apply it on the off-chance that it is also triggerable with 
the default 4M chunk size (haven't been able to yet), but then I'd add a 
comment here to make it clear that this constant is just because we 
don't have the actual one available (yet).

> 
>  pve-backup.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/pve-backup.c b/pve-backup.c
> index 77eb475563..4d423611e1 100644
> --- a/pve-backup.c
> +++ b/pve-backup.c
> @@ -147,17 +147,29 @@ pvebackup_co_dump_pbs_cb(
>          return -1;
>      }
>  
> -    pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id, buf, start, size, &local_err);
> -    qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
> +    uint64_t transferred = 0;
> +    uint64_t reused = 0;
> +    while (transferred < size) {
> +        uint64_t left = size - transferred;
> +        uint64_t to_transfer = left < PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE ?
> +            left : PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE;
>  
> -    if (pbs_res < 0) {
> -        pvebackup_propagate_error(local_err);
> -        return pbs_res;
> -    } else {
> -        size_t reused = (pbs_res == 0) ? size : 0;
> -        pvebackup_add_transfered_bytes(size, !buf ? size : 0, reused);
> +        pbs_res = proxmox_backup_co_write_data(backup_state.pbs, di->dev_id,
> +            buf ? buf + transferred : NULL, start + transferred, to_transfer, &local_err);
> +        transferred += to_transfer;
> +
> +        if (pbs_res < 0) {
> +            pvebackup_propagate_error(local_err);
> +            qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
> +            return pbs_res;
> +        }
> +
> +        reused += pbs_res == 0 ? to_transfer : 0;
>      }
>  
> +    qemu_co_mutex_unlock(&backup_state.dump_callback_mutex);
> +    pvebackup_add_transfered_bytes(size, !buf ? size : 0, reused);
> +
>      return size;
>  }
>  
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>