Re: [pbs-devel] [PATCH v3 proxmox-backup 1/5] fix #3847: datastore: support writing fidx files of unknown size

[Robert Obkircher <r.obkircher@proxmox.com>]

On 1/14/26 14:13, Fabian Grünbichler wrote:
> On January 9, 2026 6:35 pm, Robert Obkircher wrote:
>> Use mremap and ftruncate to support growable FixedIndexWriters. Grow
>> exponentially from a small initial index size for efficiency. Truncate
>> excessive capacity after encountering a non-full block or on close.
>>
>> Signed-off-by: Robert Obkircher<r.obkircher@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs   |   2 +-
>>   pbs-datastore/src/fixed_index.rs | 120 +++++++++++++++++++++++++++++--
>>   2 files changed, 117 insertions(+), 5 deletions(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 9c57aaac..af712726 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -591,7 +591,7 @@ impl DataStore {
>>       pub fn create_fixed_writer<P: AsRef<Path>>(
>>           &self,
>>           filename: P,
>> -        size: usize,
>> +        size: Option<usize>,
>>           chunk_size: usize,
>>       ) -> Result<FixedIndexWriter, Error> {
>>           let index = FixedIndexWriter::create(
>> diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
>> index 6c3be2d4..8036a519 100644
>> --- a/pbs-datastore/src/fixed_index.rs
>> +++ b/pbs-datastore/src/fixed_index.rs
>> @@ -219,9 +219,12 @@ pub struct FixedIndexWriter {
>>       chunk_size: usize,
>>       size: usize,
>>       index_length: usize,
>> +    index_capacity: usize,
> chunk_size and size effectively already give you index_length (as in,
> how many "slots" are allowed to be used), so index length and index
> capacity can be a single field. if you want to rename it to signify that
> it might now be bigger than ceil(size/chunk_size), that's fine, but I
> don't think we need to duplicate it and complicate the code below..
By that logic (chunk_size, size, growable_size) also give me the capacity,
so both fields could be removed.

I kept them as separate fields because that is what the original code and
the Reader do, supposedly to avoid the division for the bounds check.

This also makes it much more explicit what is going on and since the
index_length has to be computed anyway while resizing, I don't see how
storing that value for later is more complicated than recomputing it.

>>       index: *mut u8,
>>       pub uuid: [u8; 16],
>>       pub ctime: i64,
>> +    growable_size: bool,
>> +    write_size_on_close: bool,
>>   }
>>   
>>   // `index` is mmap()ed which cannot be thread-local so should be sendable
>> @@ -237,12 +240,18 @@ impl Drop for FixedIndexWriter {
>>   }
>>   
>>   impl FixedIndexWriter {
>> +    /// The initial capacity, if the total size is unknown.
>> +    ///
>> +    /// This capacity takes up the same amount of space as the header
>> +    /// and can refer to 128 Blocks * 4 MiB/Block = 512 MiB of content.
>> +    const INITIAL_CAPACITY: usize = 4096 / 32;
> might make sense to make this more explicit - we can only map using page
> granularity, the header is one page, so the first part needs to be a
> page as well, and if then always double it when resizing we stay aligned
> to page boundaries.

There is actually no strict requirement for the mapped length to be
a multiple of the page size, so I didn't want to guarantee too much.
However, the offset into the file must be aligned, so a page size
larger than the header would break anyway.

I think it would make sense to just map the file from the start, because
that would simplify my other fix [1] and the size/checksum update in
close().

[1] 
https://lore.proxmox.com/pbs-devel/20260109175945.309913-3-r.obkircher@proxmox.com/

>> +
>>       #[allow(clippy::cast_ptr_alignment)]
>>       // Requires obtaining a shared chunk store lock beforehand
>>       pub fn create(
>>           store: Arc<ChunkStore>,
>>           path: &Path,
>> -        size: usize,
>> +        known_size: Option<usize>,
>>           chunk_size: usize,
>>       ) -> Result<Self, Error> {
>>           let full_path = store.relative_path(path);
>> @@ -264,6 +273,7 @@ impl FixedIndexWriter {
>>           }
>>   
>>           let ctime = proxmox_time::epoch_i64();
>> +        let size = known_size.unwrap_or(0);
>>   
>>           let uuid = Uuid::generate();
>>   
>> @@ -280,8 +290,12 @@ impl FixedIndexWriter {
>>   
>>           file.write_all(&buffer)?;
>>   
>> -        let index_length = size.div_ceil(chunk_size);
>> -        let index_size = index_length * 32;
>> +        let (index_length, index_capacity) = known_size
>> +            .map(|s| s.div_ceil(chunk_size))
>> +            .map(|len| (len, len))
>> +            .unwrap_or((0, Self::INITIAL_CAPACITY));
>> +
>> +        let index_size = index_capacity * 32;
>>           nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
>>   
>>           let data = unsafe {
>> @@ -305,12 +319,87 @@ impl FixedIndexWriter {
>>               chunk_size,
>>               size,
>>               index_length,
>> +            index_capacity,
>>               index: data,
>>               ctime,
>>               uuid: *uuid.as_bytes(),
>> +            growable_size: known_size.is_none(),
>> +            write_size_on_close: known_size.is_none(),
>> +        })
>> +    }
>> +
>> +    /// If this returns an error, the sizes may be out of sync,
>> +    /// which is especially bad if the capacity was reduced.
>> +    fn set_index_capacity(&mut self, new_capacity: usize) -> Result<(), Error> {
>> +        if new_capacity == self.index_capacity {
>> +            return Ok(());
>> +        }
>> +        let old_index_size = self.index_capacity * 32;
>> +        let new_index_size = new_capacity * 32;
>> +        let new_file_size = (size_of::<FixedIndexHeader>() + new_index_size) as i64;
>> +
>> +        let index_addr = NonNull::new(self.index as *mut std::ffi::c_void).ok_or_else(|| {
>> +            format_err!("Can't resize FixedIndexWriter index because the index pointer is null.")
>> +        })?;
>> +
>> +        nix::unistd::ftruncate(&self.file, new_file_size)?;
>> +
>> +        let new_index = unsafe {
>> +            nix::sys::mman::mremap(
>> +                index_addr,
>> +                old_index_size,
>> +                new_index_size,
>> +                nix::sys::mman::MRemapFlags::MREMAP_MAYMOVE,
>> +                None,
>> +            )
>> +        }?;
>> +
>> +        self.index = new_index.as_ptr().cast::<u8>();
>> +        self.index_capacity = new_capacity;
>> +        Ok(())
>> +    }
>> +
>> +    /// Unmapping ensures future add and close operations fail.
>> +    fn set_index_capacity_or_unmap(&mut self, new_capacity: usize) -> Result<(), Error> {
>> +        self.set_index_capacity(new_capacity).map_err(|e| {
>> +            let unmap_result = self.unmap();
>> +            let message = format!(
>> +                "failed to resize index capacity from {} to {new_capacity} with backing file: {:?}",
>> +                self.index_capacity, self.tmp_filename
>> +            );
>> +            assert!(self.index.is_null(), "{message} {unmap_result:?}");
>> +            e.context(message)
>>           })
>>       }
>>   
>> +    /// Increase the content size to be at least `requested_size` and
>> +    /// ensure there is enough capacity.
>> +    ///
>> +    /// Only writers that were created without a known size can grow.
>> +    /// The size also becomes fixed as soon as it is no longer divisible
>> +    /// by the block size, to ensure that only the last block can be
>> +    /// smaller.
>> +    pub fn grow_to_size(&mut self, requested_size: usize) -> Result<(), Error> {
>> +        if self.size < requested_size {
>> +            if !self.growable_size {
>> +                bail!("refusing to resize from {} to {requested_size}", self.size);
>> +            }
>> +            let new_len = requested_size.div_ceil(self.chunk_size);
>> +            if new_len * self.chunk_size != requested_size {
>> +                // not a full chunk, so this must be the last one
>> +                self.growable_size = false;
>> +                self.set_index_capacity_or_unmap(new_len)?;
>> +            } else if new_len > self.index_capacity {
>> +                self.set_index_capacity_or_unmap(new_len.next_power_of_two())?;
>> +            };
>> +            assert!(new_len <= self.index_capacity);
>> +            self.index_length = new_len;
>> +            self.size = requested_size;
>> +        }
> should we handle the else part here? i.e., error out if shrinking is
> requested?

The else part is expected to succeed. I considered renaming the method to
something like grow_if_smaller or ensure_capacity to make this clear, but
I'm not quite happy with those names either.

>> +        Ok(())
>> +    }
>> +
>> +    /// The current length of the index. This may be increased with [`grow_to_size`].
>>       pub fn index_length(&self) -> usize {
>>           self.index_length
>>       }
>> @@ -320,7 +409,7 @@ impl FixedIndexWriter {
>>               return Ok(());
>>           };
>>   
>> -        let index_size = self.index_length * 32;
>> +        let index_size = self.index_capacity * 32;
>>   
>>           if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
>>               bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
>> @@ -342,9 +431,24 @@ impl FixedIndexWriter {
>>   
>>           self.unmap()?;
>>   
>> +        if self.index_length == 0 {
>> +            bail!("refusing to close empty fidx file {:?}", self.tmp_filename);
>> +        } else if self.index_length < self.index_capacity {
>> +            let file_size = size_of::<FixedIndexHeader>() + index_size;
>> +            nix::unistd::ftruncate(&self.file, file_size as i64)?;
>> +            self.index_capacity = self.index_length;
>> +        }
>> +
>>           let csum_offset = std::mem::offset_of!(FixedIndexHeader, index_csum);
>>           self.file.seek(SeekFrom::Start(csum_offset as u64))?;
>>           self.file.write_all(&index_csum)?;
>> +
>> +        if self.write_size_on_close {
>> +            let size_offset = std::mem::offset_of!(FixedIndexHeader, size);
>> +            self.file.seek(SeekFrom::Start(size_offset as u64))?;
>> +            self.file.write_all(&(self.size as u64).to_le_bytes())?;
>> +        }
>> +
>>           self.file.flush()?;
>>   
>>           if let Err(err) = std::fs::rename(&self.tmp_filename, &self.filename) {
>> @@ -407,6 +511,14 @@ impl FixedIndexWriter {
>>       }
>>   
>>       pub fn clone_data_from(&mut self, reader: &FixedIndexReader) -> Result<(), Error> {
>> +        if self.growable_size {
>> +            bail!("reusing the index is only supported with known input size");
>> +        }
>> +
>> +        if self.chunk_size != reader.chunk_size {
>> +            bail!("can't reuse file with different chunk size");
>> +        }
>> +
>>           if self.index_length != reader.index_count() {
>>               bail!("clone_data_from failed - index sizes not equal");
>>           }
>> -- 
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pbs-devel mailing list
>> pbs-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>>
>>
>>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel