From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id C89261FF13E for ; Fri, 23 Jan 2026 16:43:18 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DCFC51165E; Fri, 23 Jan 2026 16:43:38 +0100 (CET) From: Robert Obkircher To: pbs-devel@lists.proxmox.com Date: Fri, 23 Jan 2026 16:37:24 +0100 Message-ID: <20260123154147.222215-12-r.obkircher@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260123154147.222215-1-r.obkircher@proxmox.com> References: <20260123154147.222215-1-r.obkircher@proxmox.com> MIME-Version: 1.0 X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1769182955740 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.055 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: [pbs-devel] [PATCH v4 proxmox-backup 11/11] datastore: support writing fidx files on systems with larger page 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" A file can only be memory mapped at an offset which is a multiple of the page size. Mapping from the start avoids this issue and simplifies writes to the header. The content size is now written unconditionally on close, because the OS will write the entire page anyway to persist the checksum. Signed-off-by: Robert Obkircher --- pbs-datastore/src/fixed_index.rs | 121 ++++++++++++++----------------- 1 file changed, 54 insertions(+), 67 deletions(-) diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs index dbb24111..5fbb2d93 100644 --- a/pbs-datastore/src/fixed_index.rs +++ b/pbs-datastore/src/fixed_index.rs @@ -1,5 +1,4 @@ use std::fs::File; -use std::io::Write; use std::io::{Seek, SeekFrom}; use std::os::unix::io::AsRawFd; use std::path::{Path, PathBuf}; @@ -210,6 +209,18 @@ impl IndexFile for FixedIndexReader { } } +struct MmapPtr(NonNull); + +impl MmapPtr { + fn header(&self) -> NonNull { + self.0.cast::() + } + + fn index(&self) -> NonNull { + unsafe { self.0.byte_add(size_of::()).cast::() } + } +} + pub struct FixedIndexWriter { file: File, filename: PathBuf, @@ -218,11 +229,10 @@ pub struct FixedIndexWriter { size: u64, index_length: usize, index_capacity: usize, - index: *mut u8, + memory: Option, 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 @@ -255,7 +265,7 @@ impl FixedIndexWriter { let mut tmp_path = full_path.clone(); tmp_path.set_extension("tmp_fidx"); - let mut file = std::fs::OpenOptions::new() + let file = std::fs::OpenOptions::new() .create(true) .truncate(true) .read(true) @@ -274,19 +284,6 @@ impl FixedIndexWriter { let uuid = Uuid::generate(); - let buffer = vec![0u8; header_size]; - let header = unsafe { &mut *(buffer.as_ptr() as *mut FixedIndexHeader) }; - - header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0; - header.ctime = i64::to_le(ctime); - header.size = u64::to_le(size as u64); - header.chunk_size = u64::to_le(chunk_size as u64); - header.uuid = *uuid.as_bytes(); - - header.index_csum = [0u8; 32]; - - file.write_all(&buffer)?; - let (index_length, index_capacity) = match known_size { Some(s) => { let len = s.div_ceil(chunk_size).try_into()?; @@ -295,23 +292,25 @@ impl FixedIndexWriter { None => (0, Self::INITIAL_CAPACITY), }; - let index_size = index_capacity * 32; let file_size = Self::file_size(index_capacity)?; nix::unistd::ftruncate(&file, file_size)?; - let data = unsafe { + let memory = MmapPtr(unsafe { nix::sys::mman::mmap( None, - std::num::NonZeroUsize::new(index_size) - .ok_or_else(|| format_err!("invalid index size"))?, + std::num::NonZeroUsize::new(file_size as usize).expect("has header"), nix::sys::mman::ProtFlags::PROT_READ | nix::sys::mman::ProtFlags::PROT_WRITE, nix::sys::mman::MapFlags::MAP_SHARED, &file, - header_size as i64, + 0, ) - }? - .as_ptr() - .cast::(); + }?); + + let header = unsafe { memory.header().as_mut() }; + header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0; + header.ctime = i64::to_le(ctime); + header.chunk_size = u64::to_le(chunk_size); + header.uuid = *uuid.as_bytes(); Ok(Self { file, @@ -321,11 +320,10 @@ impl FixedIndexWriter { size, index_length, index_capacity, - index: data, + memory: Some(memory), ctime, uuid: *uuid.as_bytes(), growable_size: known_size.is_none(), - write_size_on_close: known_size.is_none(), }) } @@ -351,27 +349,26 @@ impl FixedIndexWriter { 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 = Self::file_size(new_capacity)?; + let old_size = Self::file_size(self.index_capacity)?; + let new_size = Self::file_size(new_capacity)?; - 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.") - })?; + let Some(MmapPtr(index_addr)) = self.memory else { + bail!("Can't resize unmapped FixedIndexWriter"); + }; - nix::unistd::ftruncate(&self.file, new_file_size)?; + nix::unistd::ftruncate(&self.file, new_size)?; let new_index = unsafe { nix::sys::mman::mremap( index_addr, - old_index_size, - new_index_size, + old_size as usize, + new_size as usize, nix::sys::mman::MRemapFlags::MREMAP_MAYMOVE, None, ) }?; - self.index = new_index.as_ptr().cast::(); + self.memory = Some(MmapPtr(new_index)); self.index_capacity = new_capacity; Ok(()) } @@ -384,7 +381,7 @@ impl FixedIndexWriter { "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:?}"); + assert!(self.memory.is_none(), "{message} {unmap_result:?}"); e.context(message) }) } @@ -430,30 +427,32 @@ impl FixedIndexWriter { } fn unmap(&mut self) -> Result<(), Error> { - let Some(index) = NonNull::new(self.index as *mut std::ffi::c_void) else { - return Ok(()); - }; - - 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); + if let Some(ptr) = self.memory.take() { + let len = Self::file_size(self.index_capacity).expect( + "this is the capacity that didn't cause an overflow when the memory was mapped", + ); + if let Err(err) = unsafe { nix::sys::mman::munmap(ptr.0, len as usize) } { + bail!("unmap file {:?} failed - {}", self.tmp_filename, err); + } } - - self.index = std::ptr::null_mut(); - Ok(()) } pub fn close(&mut self) -> Result<[u8; 32], Error> { - if self.index.is_null() { + let Some(ptr) = &self.memory else { bail!("cannot close already closed index file."); - } + }; let index_size = self.index_length * 32; - let data = unsafe { std::slice::from_raw_parts(self.index, index_size) }; + let data = unsafe { std::slice::from_raw_parts(ptr.index().as_ptr(), index_size) }; let index_csum = openssl::sha::sha256(data); + { + let header = unsafe { ptr.header().as_mut() }; + header.index_csum = index_csum; + header.size = self.size.to_le(); + } + self.unmap()?; if self.index_length < self.index_capacity { @@ -462,18 +461,6 @@ impl FixedIndexWriter { 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) { bail!("Atomic rename file {:?} failed - {}", self.filename, err); } @@ -518,13 +505,13 @@ impl FixedIndexWriter { ); } - if self.index.is_null() { + let Some(ptr) = &self.memory else { bail!("cannot write to closed index file."); - } + }; let index_pos = index * 32; unsafe { - let dst = self.index.add(index_pos); + let dst = ptr.index().as_ptr().add(index_pos); dst.copy_from_nonoverlapping(digest.as_ptr(), 32); } -- 2.47.3 _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel