From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 9BA361FF13F for ; Wed, 14 Jan 2026 14:14:00 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CB6C312741; Wed, 14 Jan 2026 14:14:04 +0100 (CET) Date: Wed, 14 Jan 2026 14:13:58 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20260109173548.301653-1-r.obkircher@proxmox.com> <20260109173548.301653-2-r.obkircher@proxmox.com> In-Reply-To: <20260109173548.301653-2-r.obkircher@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1768390129.t2gtpb8x7e.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1768396395544 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v3 proxmox-backup 1/5] fix #3847: datastore: support writing fidx files of unknown size X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" 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 > --- > 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>( > &self, > filename: P, > - size: usize, > + size: Option, > chunk_size: usize, > ) -> Result { > 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.. > 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. > + > #[allow(clippy::cast_ptr_alignment)] > // Requires obtaining a shared chunk store lock beforehand > pub fn create( > store: Arc, > path: &Path, > - size: usize, > + known_size: Option, > chunk_size: usize, > ) -> Result { > 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::() + 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::(); > + 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? > + 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::() + 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