Re: [PATCH proxmox-backup v2 3/3] fix #6990: server: drop verify state on non-decrypt pull job

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

On Fri Aug 28, 2026 at 1:11 PM CEST, Christian Ebner wrote:
> Two smaller comments and considerations inline, rest looks good to me!
>
> On 8/21/26 1:18 PM, Jakob Klocker wrote:
>> On a non-decrypt pull the source manifest is written to the target
>> as-is, so the target inherits the source's verify_state flag instead of
>> being verified independently on its own storage. Because a snapshot
>> carrying a verify_state is skipped by verify jobs, the target's copy can
>> never be checked.
>> 
>> The decrypt path already drops verify_state; do the same on the
>> non-decrypt path when the snapshot is newly pulled or re-synced due to
>> corruption. Also factor the manifest blob encoding and writing into a
>> helper, shared by both paths.
>> 
>> Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990
>> Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
>> ---
>>   pbs-datastore/src/manifest.rs | 25 ++++++++++++++++++-
>>   src/server/pull.rs            | 47 +++++++++++++++++++++++------------
>>   2 files changed, 55 insertions(+), 17 deletions(-)
>> 
>> diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
>> index 11de1085b..c87797474 100644
>> --- a/pbs-datastore/src/manifest.rs
>> +++ b/pbs-datastore/src/manifest.rs
>> @@ -1,5 +1,8 @@
>> -use anyhow::{Error, bail, format_err};
>> +use std::io::Write;
>> +use std::os::fd::AsRawFd;
>> +use std::path::Path;
>>   
>> +use anyhow::{Context, Error, bail, format_err};
>>   use serde::{Deserialize, Serialize};
>>   use serde_json::{Value, json};
>>   
>> @@ -278,6 +281,26 @@ impl BackupManifest {
>>   
>>           Ok(Some(Deserialize::deserialize(value)?))
>>       }
>> +
>> +    /// Encode the manifest and write the raw blob data to `path`, fsync'ing it.
>
> comment: I would like for this comment to also include a note/warning 
> that this should only be used to write manifest files to temp file path 
> (or maybe we even want to check/encode that)? If this will be used to 
> write the mainfest directly, it would lead to concurrent readers to 
> potentially read incomplete and therefore corrupt manifests.
>
> Another option and probably even preferable would be for the rename to 
> be included in this helper as well and maybe pass both, tmp and target 
> path. From the diff below that should be doable and give us guarantees 
> that the manifest is persisted atomically.
>
>
> Also this whole content writing could be performed as async via 
> tokio::fs::File, but taking the performance considerations into account 
> [0] it probably is best kept sync for now. Major win would be that this 
> could then use `io-uring` for some operations if enabled in tokio in the 
> future without code changes, e.g. the OpenOptions used by File have 
> config and feature flags for this [1].
>
> [0] https://docs.rs/tokio/latest/tokio/fs/index.html
> [1] https://docs.rs/tokio/latest/src/tokio/fs/open_options.rs.html#122
>

I'll add the rename in the helper as well, since I can't think of a
case where this helper would be used to write the manifest without
renaming it afterwards. This would also make it more clear how this
should be used and cover the note/warning concern.

Thanks for linking and explaining parts of the tokio 
documentation, since I haven't used it much yet this information is
much appreciated!

>> +    ///
>> +    /// Returns the raw blob data.
>> +    pub fn write_to_path(&self, path: &Path) -> Result<Vec<u8>, Error> {
>
> This could take ownership of the manifest instead, so no further 
> modifications are to be made afterwards and we can get rid of the Arc 
> below, since it can be moved without issues into the spawn_blocking call
>

The reason I reached for Arc is that manifest is used again after the
write, in cleanup_unreferenced_files() -- so it can't just be moved
into spawn_blocking and dropped. Taking ownership in write_to_path()
alone doesn't resolve that; I'd still need a way to get the manifest
back or keep a copy. I see three options:

- Return the manifest from write_to_path() (e.g. Result<(Vec<u8>, Self)>) 
and rebind it for the cleanup call.
- Derive Clone on BackupManifest and clone before moving into
  spawn_blocking.
- Keep the Arc

I went with Arc as it felt like the least invasive option, but I'm
happy to switch.
Do you have a preference, or am I missing something here?

>> +        let raw_data = self.to_data_blob(None)?.raw_data().to_vec();
>> +
>> +        let mut file = std::fs::OpenOptions::new()
>> +            .write(true)
>> +            .create(true)
>> +            .truncate(true)
>> +            .open(path)
>> +            .with_context(|| format!("failed to open manifest {path:?}"))?;
>> +
>> +        file.write_all(&raw_data)?;
>> +        file.flush()?;
>> +        nix::unistd::fsync(file.as_raw_fd())?;
>> +
>> +        Ok(raw_data)
>> +    }
>>   }
>>   
>>
>>   [SNIP]