Re: [PATCH proxmox-backup v2 1/3] server: pull: run blocking file operations on the blocking pool

[Christian Ebner <c.ebner@proxmox.com>]

One comment inline, with that addressed consider:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

On 8/21/26 1:18 PM, Jakob Klocker wrote:
> The atomic renames and the unreferenced-file cleanup are blocking
> operations that ran directly on the runtime's worker threads, stalling
> other tasks scheduled there for their duration.
> 
> Use the tokio counterpart for the renames and move the cleanup onto the
> blocking thread pool.
> 
> Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
> ---
>   src/server/pull.rs | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 4eb5bcf11..fd401ac85 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -600,7 +600,7 @@ async fn pull_single_archive<'a>(
>       } else {
>           tmp_path
>       };
> -    if let Err(err) = std::fs::rename(&source_path, &path) {
> +    if let Err(err) = tokio::fs::rename(&source_path, &path).await {

comment: there is an additional std::fs::remove_file() in the pull code, 
e.g. removing the temp file just above this (not seen in diff). All of 
these should be moved to tokio::fs::remove_file().

We need to be careful with performance when using tokio::fs over std::fs 
as stated in [0], but these should be fine to call on the blocking task 
pool. That this might have performance implications might be also noted 
in the commit message.

[0] https://docs.rs/tokio/latest/tokio/fs/index.html

>           bail!("{archive_prefix}: Atomic rename file {path:?} failed - {err}");
>       }
>   
> @@ -890,7 +890,7 @@ async fn pull_snapshot<'a>(
>           nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
>       }
>   
> -    if let Err(err) = std::fs::rename(&tmp_manifest_name, &manifest_name) {
> +    if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
>           bail!("{prefix}: Atomic rename file {manifest_name:?} failed - {err}");
>       }
>       if let DatastoreBackend::S3(s3_client) = backend {
> @@ -910,8 +910,9 @@ async fn pull_snapshot<'a>(
>   
>       fetch_log(crypt_config).await?;
>   
> -    snapshot
> -        .cleanup_unreferenced_files(&manifest)
> +    let snapshot = snapshot.clone();
> +    tokio::task::spawn_blocking(move || snapshot.cleanup_unreferenced_files(&manifest))
> +        .await?
>           .map_err(|err| format_err!("{prefix}: failed to cleanup unreferenced files - {err}"))?;
>   
>       Ok(Some(sync_stats))