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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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
> 
> 
>