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

["Jakob Klocker" <j.klocker@proxmox.com>]

On Fri Aug 28, 2026 at 12:30 PM CEST, Christian Ebner wrote:
> 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:
>> [SNIP]
>> +    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
>

I'll make sure to replace all existing std::fs::remove_file() with
tokio's version and to adapt the commit message in the next revision.
Since I'm already at it, I'll do so for all occasions in the pull.rs 
file -- std::fs::remove_file is also called twice in
pull_single_archive().

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