Re: [PATCH v5 proxmox-backup 07/16] api: backup: make fixed index file size optional

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

On 2/2/26 2:19 PM, Robert Obkircher wrote:
> 
> On 2/2/26 12:38, Christian Ebner wrote:
>> On 1/30/26 5:45 PM, Robert Obkircher wrote:
>>> Pass the optional size to the FixedIndexWriter to make it grow as
>>> necessary.
>>>
>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>> ---
>>>    src/api2/backup/environment.rs | 21 ++++++++++++---------
>>>    src/api2/backup/mod.rs         |  8 +++-----
>>>    2 files changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/api2/backup/environment.rs
>>> b/src/api2/backup/environment.rs
>>> index bd9c5211..90a84ed2 100644
>>> --- a/src/api2/backup/environment.rs
>>> +++ b/src/api2/backup/environment.rs
>>> @@ -67,7 +67,7 @@ struct DynamicWriterState {
>>>    struct FixedWriterState {
>>>        name: String,
>>>        index: FixedIndexWriter,
>>> -    size: usize,
>>> +    size: Option<usize>,
>>>        chunk_size: u32,
>>>        chunk_count: u64,
>>>        small_chunk_count: usize, // allow 0..1 small chunks (last
>>> chunk may be smaller)
>>> @@ -349,7 +349,7 @@ impl BackupEnvironment {
>>>            &self,
>>>            index: FixedIndexWriter,
>>>            name: String,
>>> -        size: usize,
>>> +        size: Option<usize>,
>>>            chunk_size: u32,
>>>            incremental: bool,
>>>        ) -> Result<usize, Error> {
>>> @@ -443,6 +443,7 @@ impl BackupEnvironment {
>>>            }
>>>              let end = (offset as usize) + (size as usize);
>>> +        data.index.grow_to_size(end)?;
>>>            let idx = data.index.check_chunk_alignment(end, size as
>>> usize)?;
>>>              data.chunk_count += 1;
>>> @@ -624,13 +625,15 @@ impl BackupEnvironment {
>>>                    );
>>>                }
>>>    -            if size != (data.size as u64) {
>>> -                bail!(
>>> -                    "fixed writer '{}' close failed - unexpected
>>> file size ({} != {})",
>>> -                    data.name,
>>> -                    data.size,
>>> -                    size
>>> -                );
>>> +            if let Some(known_size) = data.size {
>>> +                if size != known_size as u64 {
>>> +                    bail!(
>>> +                        "fixed writer '{}' close failed -
>>> unexpected file size ({} != {})",
>>> +                        data.name,
>>> +                        known_size,
>>> +                        size,
>>> +                    );
>>> +                }
>>>                }
>>>            }
>>>    diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>>> index c625a2dc..c2822c18 100644
>>> --- a/src/api2/backup/mod.rs
>>> +++ b/src/api2/backup/mod.rs
>>> @@ -456,7 +456,7 @@ pub const API_METHOD_CREATE_FIXED_INDEX:
>>> ApiMethod = ApiMethod::new(
>>>                ("archive-name", false, &BACKUP_ARCHIVE_NAME_SCHEMA),
>>>                (
>>>                    "size",
>>> -                false,
>>> +                true,
>>>                    &IntegerSchema::new("File
>>> size.").minimum(1).schema()
>>>                ),
>>>                (
>>> @@ -480,7 +480,7 @@ fn create_fixed_index(
>>>        let env: &BackupEnvironment = rpcenv.as_ref();
>>>          let name = required_string_param(&param,
>>> "archive-name")?.to_owned();
>>> -    let size = required_integer_param(&param, "size")? as usize;
>>> +    let size =
>>> param["size"].as_u64().map(usize::try_from).transpose()?;
>>>        let reuse_csum = param["reuse-csum"].as_str();
>>
>> The fixed index writer does not allow for incremental backups,
>> failing if it is requested.
>>
>> question: should we allow to proceed the backup without failing by
>> simply switching to a non incremental backup for those index writers
>> where no size is specified, including a log warning that that a
>> non-incremental mode is used?
> I think reuse-csum without size could reasonably be supported by
> copying the entire file and taking the size from there. Now that
> add_chunk is a single operation it would even be possible to allow
> overwriting the partial chunk in the end to continue growing.

Yes, if this can be supported as well would be great. It will however 
only allow to skip re-uploads, chunking of the stream on the client side 
has to happen anyways. And it must be assured that the stream might 
shrink, not just grow.

>>
>> Should be better than failing the whole backup snapshot in case of
>> multiple archives with mixed inputs.
> As far as I can tell, "reuse-csum" is only set in one specific place
> in proxmox-backup-qemu and only if the checksum matches exactly. Is
> this actually relevant for anything else?

No, AFAIK the checksum assures that the index cannot be reused for an 
incremental backup if e.g. there was a backup to the same backup group 
from a different client, which could lead to inconsistencies. So the 
checksum stored by the client must match the one found on the server to 
proceed safely, otherwise fallback to non-incremental.

>>
>>>          let archive_name = name.clone();
>>> @@ -528,9 +528,7 @@ fn create_fixed_index(
>>>            reader = Some(index);
>>>        }
>>>    -    let mut writer = env
>>> -        .datastore
>>> -        .create_fixed_writer(&path, Some(size), chunk_size)?;
>>> +    let mut writer = env.datastore.create_fixed_writer(&path,
>>> size, chunk_size)?;
>>>          if let Some(reader) = reader {
>>>            writer.clone_data_from(&reader)?;
>>