[PATCH v5 proxmox-backup 00/16] fix: #3847 pipe from STDIN to proxmox-backup-client

[PATCH v5 proxmox-backup 00/16] fix: #3847 pipe from STDIN to proxmox-backup-client

[Robert Obkircher]

Add support for commands like:
    ssh host cmd | proxmox-backup-client backup data.img:/dev/stdin
    proxmox-backup-client backup a.img:<(mysqldump) b.img:<(pgdump)

Changes since v4:
- reordered the ChunkStore removal commit and adjusted dynamic writer
- moved TempTestDir into separate file, use it for test_chunk_store1
- improved commit messages and comments
- changed u32 param to u64
- removed unnecessary casts reported by clippy
- added two commits at the end which restrict chunk_size to u32
  (I'd fixup commit 12/16 if we actually want to keep those changes)

An open question is if we should maybe fsync the data before renaming.
Or is that too much overhead for slightly better crash consistency?


Robert Obkircher (16):
  datastore: remove Arc<ChunkStore> from FixedIndexWriter
  datastore: remove Arc<ChunkStore> from DynamicIndexWriter
  datastore: add TempTestDir that is automatically deleted on drop
  datastore: use temporary directory for chunk store test
  datastore: support writing fidx files of unknown size
  datastore: test FixedIndexWriter
  api: backup: make fixed index file size optional
  api: verify fixed index writer size on close
  fix #3847: client: support fifo pipe inputs for images
  client: treat minus sign as stdin
  datastore: combine public FixedIndexWriter methods into add_chunk.
  datastore: use u64 instead of usize for fidx writer content size
  datastore: compute fidx file size with overflow checks
  datastore: support writing fidx files on systems with larger page size
  datastore: FixedIndexWriter: switch public chunk_size to u32
  datastore: FixedIndexWriter: switch internal chunk_size to u32

 pbs-client/src/backup_writer.rs    |  38 ++-
 pbs-datastore/src/chunk_store.rs   |   7 +-
 pbs-datastore/src/datastore.rs     |  19 +-
 pbs-datastore/src/dynamic_index.rs |   5 +-
 pbs-datastore/src/fixed_index.rs   | 435 +++++++++++++++++++++++++----
 pbs-datastore/src/lib.rs           |   3 +
 pbs-datastore/src/temp_test_dir.rs |  33 +++
 proxmox-backup-client/src/main.rs  |  37 ++-
 src/api2/backup/environment.rs     |  33 ++-
 src/api2/backup/mod.rs             |   6 +-
 src/server/push.rs                 |  11 +-
 11 files changed, 499 insertions(+), 128 deletions(-)
 create mode 100644 pbs-datastore/src/temp_test_dir.rs

-- 
2.47.3

[PATCH v5 proxmox-backup 01/16] datastore: remove Arc<ChunkStore> from FixedIndexWriter

[Robert Obkircher]

The ChunkStore reference can be safely removed, because it was only
needed for the base directory. As discussed previously in the context
of the DynamicIndexWriter [1], it is the callers responsibility to
ensure that the writer doesn't outlive any locks.

This simplifies testing the writer, which would otherwise require a
workaround to avoid creating 65k directories.

[1] https://lore.proxmox.com/pbs-devel/1761822027.sm41v71zmk.astroid@yuna.none/

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   | 10 ++--------
 pbs-datastore/src/fixed_index.rs |  7 ++-----
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 7ad3d917..1c631817 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -698,14 +698,8 @@ impl DataStore {
         size: usize,
         chunk_size: usize,
     ) -> Result<FixedIndexWriter, Error> {
-        let index = FixedIndexWriter::create(
-            self.inner.chunk_store.clone(),
-            filename.as_ref(),
-            size,
-            chunk_size,
-        )?;
-
-        Ok(index)
+        let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
+        FixedIndexWriter::create(full_path, size, chunk_size)
     }
 
     pub fn open_fixed_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 6c3be2d4..becbe3e3 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -4,14 +4,12 @@ use std::io::{Seek, SeekFrom};
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::ptr::NonNull;
-use std::sync::Arc;
 
 use anyhow::{bail, format_err, Error};
 
 use proxmox_io::ReadExt;
 use proxmox_uuid::Uuid;
 
-use crate::chunk_store::ChunkStore;
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
 
@@ -240,12 +238,11 @@ impl FixedIndexWriter {
     #[allow(clippy::cast_ptr_alignment)]
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
-        store: Arc<ChunkStore>,
-        path: &Path,
+        full_path: impl Into<PathBuf>,
         size: usize,
         chunk_size: usize,
     ) -> Result<Self, Error> {
-        let full_path = store.relative_path(path);
+        let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
         tmp_path.set_extension("tmp_fidx");
 
-- 
2.47.3

[PATCH v5 proxmox-backup 02/16] datastore: remove Arc<ChunkStore> from DynamicIndexWriter

[Robert Obkircher]

Remain consistent with the FixedIndexWriter and make it easier to add
tests in the future.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs     | 5 ++---
 pbs-datastore/src/dynamic_index.rs | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 1c631817..b023b0d3 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -718,9 +718,8 @@ impl DataStore {
         &self,
         filename: P,
     ) -> Result<DynamicIndexWriter, Error> {
-        let index = DynamicIndexWriter::create(self.inner.chunk_store.clone(), filename.as_ref())?;
-
-        Ok(index)
+        let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
+        DynamicIndexWriter::create(full_path)
     }
 
     pub fn open_dynamic_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index ad49cdf3..e2fe09a4 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -16,7 +16,6 @@ use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 
 use pbs_tools::lru_cache::LruCache;
 
-use crate::chunk_store::ChunkStore;
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
 use crate::read_chunk::ReadChunk;
@@ -289,8 +288,8 @@ impl Drop for DynamicIndexWriter {
 
 impl DynamicIndexWriter {
     // Requires obtaining a shared chunk store lock beforehand
-    pub fn create(store: Arc<ChunkStore>, path: &Path) -> Result<Self, Error> {
-        let full_path = store.relative_path(path);
+    pub fn create(full_path: impl Into<PathBuf>) -> Result<Self, Error> {
+        let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
         tmp_path.set_extension("tmp_didx");
 
-- 
2.47.3

[PATCH v5 proxmox-backup 03/16] datastore: add TempTestDir that is automatically deleted on drop

[Robert Obkircher]

This will be used to test the FixedIndexWriter and chunk store
creation.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/lib.rs           |  4 ++++
 pbs-datastore/src/temp_test_dir.rs | 33 ++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 pbs-datastore/src/temp_test_dir.rs

diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 1f7c54ae..b48ec93c 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -233,3 +233,7 @@ pub use local_chunk_reader::LocalChunkReader;
 
 mod local_datastore_lru_cache;
 pub use local_datastore_lru_cache::LocalDatastoreLruCache;
+
+#[cfg(test)]
+mod temp_test_dir;
+
diff --git a/pbs-datastore/src/temp_test_dir.rs b/pbs-datastore/src/temp_test_dir.rs
new file mode 100644
index 00000000..13e16c91
--- /dev/null
+++ b/pbs-datastore/src/temp_test_dir.rs
@@ -0,0 +1,33 @@
+use std::convert::From;
+use std::env;
+use std::path::{Path, PathBuf};
+
+/// A temporary directory for tests that is removed on drop.
+/// Deletion is best-effort and errors are silently ignored.
+pub struct TempTestDir(PathBuf);
+
+impl TempTestDir {
+    /// Create an empty directory.
+    pub fn new() -> Self {
+        Self(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
+    }
+}
+
+impl std::ops::Deref for TempTestDir {
+    type Target = Path;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl Drop for TempTestDir {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_dir_all(&self.0);
+    }
+}
+
+impl<'a> From<&'a TempTestDir> for PathBuf {
+    fn from(temp: &'a TempTestDir) -> Self {
+        temp.0.clone()
+    }
+}
-- 
2.47.3

[PATCH v5 proxmox-backup 04/16] datastore: use temporary directory for chunk store test

[Robert Obkircher]

Use the systems tmp directory instead of the package root, because the
latter might not always be writeable and because the created files
weren't even ignored by git.

This reduced the tests runtime from 3.2 to 0.6 seconds.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/chunk_store.rs | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index bd1dc353..a2763890 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -980,10 +980,7 @@ impl ChunkExt {
 
 #[test]
 fn test_chunk_store1() {
-    let mut path = std::fs::canonicalize(".").unwrap(); // we need absolute path
-    path.push(".testdir");
-
-    if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
+    let path = crate::temp_test_dir::TempTestDir::new();
 
     let chunk_store = ChunkStore::open("test", &path, DatastoreFSyncLevel::None);
     assert!(chunk_store.is_err());
@@ -1007,6 +1004,4 @@ fn test_chunk_store1() {
     let chunk_store =
         ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
     assert!(chunk_store.is_err());
-
-    if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
 }
-- 
2.47.3

[PATCH v5 proxmox-backup 05/16] datastore: support writing fidx files of unknown size

[Robert Obkircher]

Use mremap and ftruncate to support growable FixedIndexWriters. Grow
exponentially from a small initial index size and 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 | 127 ++++++++++++++++++++++++++++++-
 src/api2/backup/mod.rs           |   4 +-
 3 files changed, 127 insertions(+), 6 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index b023b0d3..694a5dd0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -695,7 +695,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 full_path = self.inner.chunk_store.relative_path(filename.as_ref());
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index becbe3e3..bcb0cf94 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -217,9 +217,12 @@ pub struct FixedIndexWriter {
     chunk_size: usize,
     size: usize,
     index_length: usize,
+    index_capacity: usize,
     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
@@ -235,11 +238,21 @@ 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.
+    ///
+    /// On systems with 4 KiB page size this value ensures that the
+    /// mapped length is a multiple of the page size, but this is not
+    /// strictly necessary.
+    const INITIAL_CAPACITY: usize = 4096 / 32;
+
     #[allow(clippy::cast_ptr_alignment)]
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
         full_path: impl Into<PathBuf>,
-        size: usize,
+        known_size: Option<usize>,
         chunk_size: usize,
     ) -> Result<Self, Error> {
         let full_path = full_path.into();
@@ -261,6 +274,7 @@ impl FixedIndexWriter {
         }
 
         let ctime = proxmox_time::epoch_i64();
+        let size = known_size.unwrap_or(0);
 
         let uuid = Uuid::generate();
 
@@ -277,8 +291,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 {
@@ -302,12 +320,90 @@ 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 {
+                let new_capacity = new_len
+                    .checked_next_power_of_two()
+                    .ok_or_else(|| format_err!("capacity overflow"))?;
+                self.set_index_capacity_or_unmap(new_capacity)?;
+            }
+            assert!(new_len <= self.index_capacity);
+            self.index_length = new_len;
+            self.size = requested_size;
+        }
+        Ok(())
+    }
+
+    /// The current length of the index.
     pub fn index_length(&self) -> usize {
         self.index_length
     }
@@ -317,7 +413,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);
@@ -339,9 +435,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) {
@@ -404,6 +515,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");
         }
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 3e6b7a95..c625a2dc 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -528,7 +528,9 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
+    let mut writer = env
+        .datastore
+        .create_fixed_writer(&path, Some(size), chunk_size)?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
-- 
2.47.3

[PATCH v5 proxmox-backup 06/16] datastore: test FixedIndexWriter

[Robert Obkircher]

Write fixed and dynamically sized fidx files to a temporary directory.
Compare the resulting files directly (ignoring uuid and ctime bytes)
and also read them back using the reader.

The chunk hashes are just dummy values that don't actually exist in a
chunk store.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 170 +++++++++++++++++++++++++++++++
 pbs-datastore/src/lib.rs         |   1 -
 2 files changed, 170 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index bcb0cf94..0f87bf15 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -534,3 +534,173 @@ impl FixedIndexWriter {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::fs;
+
+    use super::*;
+    use crate::temp_test_dir::TempTestDir;
+
+    const CS: usize = 4096;
+
+    #[test]
+    fn test_empty() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_empty");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        assert!(w.add_digest(0, &[1u8; 32]).is_err(), "out of bounds");
+
+        assert_eq!(0, w.size);
+        assert_eq!(0, w.index_length(), "returns length, not capacity");
+        assert_eq!(FixedIndexWriter::INITIAL_CAPACITY, w.index_capacity);
+
+        assert!(w.close().is_err(), "should refuse to create empty file");
+
+        drop(w);
+        assert!(!fs::exists(path).unwrap());
+    }
+
+    #[test]
+    fn test_single_partial_chunk() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_single_partial_chunk");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        let size = CS - 1;
+        let expected = test_data(size);
+        w.grow_to_size(size).unwrap();
+        expected[0].add_to(&mut w);
+
+        w.close().unwrap();
+        drop(w);
+
+        check_with_reader(&path, size, &expected);
+        compare_to_known_size_writer(&path, size, &expected);
+    }
+
+    #[test]
+    fn test_grow_to_multiples_of_chunk_size() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_grow_to_multiples_of_chunk_size");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        let initial = FixedIndexWriter::INITIAL_CAPACITY;
+        let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
+        let expected = test_data(steps.last().unwrap() * CS);
+
+        let mut begin = 0;
+        for chunk_count in steps {
+            let last = &expected[chunk_count - 1];
+            w.grow_to_size(last.end).unwrap();
+            assert_eq!(last.index + 1, w.index_length());
+            assert!(w.add_digest(last.index + 1, &[1u8; 32]).is_err());
+
+            for c in expected[begin..chunk_count].iter().rev() {
+                c.add_to(&mut w);
+            }
+            begin = chunk_count;
+        }
+        w.close().unwrap();
+        drop(w);
+
+        let size = expected.len() * CS;
+        check_with_reader(&path, size, &expected);
+        compare_to_known_size_writer(&path, size, &expected);
+    }
+
+    #[test]
+    fn test_grow_to_misaligned_size() {
+        let dir = TempTestDir::new();
+        let path = dir.join("test_grow_to_misaligned_size");
+        let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
+
+        let size = (FixedIndexWriter::INITIAL_CAPACITY + 42) * CS - 1; // last is not full
+        let expected = test_data(size);
+
+        w.grow_to_size(size).unwrap();
+        assert!(w.grow_to_size(size + 1).is_err(), "size must be fixed now");
+        assert_eq!(expected.len(), w.index_length());
+        assert!(w.add_digest(expected.len(), &[1u8; 32]).is_err());
+
+        for c in expected.iter().rev() {
+            c.add_to(&mut w);
+        }
+
+        w.close().unwrap();
+        drop(w);
+
+        check_with_reader(&path, size, &expected);
+        compare_to_known_size_writer(&path, size, &expected);
+    }
+
+    struct TestChunk {
+        digest: [u8; 32],
+        index: usize,
+        size: usize,
+        end: usize,
+    }
+
+    impl TestChunk {
+        fn add_to(&self, w: &mut FixedIndexWriter) {
+            assert_eq!(
+                self.index,
+                w.check_chunk_alignment(self.end, self.size).unwrap()
+            );
+            w.add_digest(self.index, &self.digest).unwrap();
+        }
+    }
+
+    fn test_data(size: usize) -> Vec<TestChunk> {
+        (0..size.div_ceil(CS))
+            .map(|index| {
+                let mut digest = [0u8; 32];
+                let i = &(index as u64).to_le_bytes();
+                for c in digest.chunks_mut(i.len()) {
+                    c.copy_from_slice(i);
+                }
+                let size = if ((index + 1) * CS) <= size {
+                    CS
+                } else {
+                    size % CS
+                };
+                TestChunk {
+                    digest,
+                    index,
+                    size,
+                    end: index * CS + size,
+                }
+            })
+            .collect()
+    }
+
+    fn check_with_reader(path: &Path, size: usize, chunks: &[TestChunk]) {
+        let reader = FixedIndexReader::open(path).unwrap();
+        assert_eq!(size as u64, reader.index_bytes());
+        assert_eq!(chunks.len(), reader.index_count());
+        for c in chunks {
+            assert_eq!(&c.digest, reader.index_digest(c.index).unwrap());
+        }
+    }
+
+    fn compare_to_known_size_writer(file: &Path, size: usize, chunks: &[TestChunk]) {
+        let mut path = file.to_path_buf();
+        path.set_extension("reference");
+        let mut w = FixedIndexWriter::create(&path, Some(size), CS).unwrap();
+        for c in chunks {
+            c.add_to(&mut w);
+        }
+        w.close().unwrap();
+        drop(w);
+
+        let mut reference = fs::read(file).unwrap();
+        let mut tested = fs::read(path).unwrap();
+
+        // ignore uuid and ctime
+        reference[8..32].fill(0);
+        tested[8..32].fill(0);
+
+        assert_eq!(reference, tested);
+    }
+}
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index b48ec93c..23a17d96 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -236,4 +236,3 @@ pub use local_datastore_lru_cache::LocalDatastoreLruCache;
 
 #[cfg(test)]
 mod temp_test_dir;
-
-- 
2.47.3

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

[Robert Obkircher]

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();
 
     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)?;
-- 
2.47.3

[PATCH v5 proxmox-backup 08/16] api: verify fixed index writer size on close

[Robert Obkircher]

Compare it to the value submitted by the client to check that the
resizing worked as expected.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 5 +++++
 src/api2/backup/environment.rs   | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 0f87bf15..c4fe899d 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -408,6 +408,11 @@ impl FixedIndexWriter {
         self.index_length
     }
 
+    /// The current total size of the referenced content.
+    pub fn size(&self) -> u64 {
+        self.size as u64
+    }
+
     fn unmap(&mut self) -> Result<(), Error> {
         let Some(index) = NonNull::new(self.index as *mut std::ffi::c_void) else {
             return Ok(());
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 90a84ed2..dc4f9bf5 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -637,6 +637,14 @@ impl BackupEnvironment {
             }
         }
 
+        let writer_size = data.index.size();
+        if size != writer_size {
+            bail!(
+                "fixed writer '{}' close failed - unexpected size ({size} != {writer_size})",
+                data.name,
+            );
+        }
+
         let expected_csum = data.index.close()?;
         data.closed = true;
 
-- 
2.47.3

[PATCH v5 proxmox-backup 09/16] fix #3847: client: support fifo pipe inputs for images

[Robert Obkircher]

Accept fifo files as inputs for image backups. The unknown size is
represented in the UploadOptions using a new IndexType enum where the
total size for the Fixed variant is now optional.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-client/src/backup_writer.rs   | 38 ++++++++++++++++++++++---------
 proxmox-backup-client/src/main.rs | 30 +++++++++++++-----------
 src/server/push.rs                | 11 +++++----
 3 files changed, 50 insertions(+), 29 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index dbd177d8..f33f1063 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -52,7 +52,17 @@ pub struct UploadOptions {
     pub previous_manifest: Option<Arc<BackupManifest>>,
     pub compress: bool,
     pub encrypt: bool,
-    pub fixed_size: Option<u64>,
+    pub index_type: IndexType,
+}
+
+/// Index type for upload options.
+#[derive(Default, Clone)]
+pub enum IndexType {
+    /// Dynamic chunking.
+    #[default]
+    Dynamic,
+    /// Fixed size chunking with optional image file size.
+    Fixed(Option<u64>),
 }
 
 struct ChunkUploadResponse {
@@ -292,11 +302,14 @@ impl BackupWriter {
         options: UploadOptions,
     ) -> Result<BackupStats, Error> {
         let mut param = json!({ "archive-name": archive_name });
-        let prefix = if let Some(size) = options.fixed_size {
-            param["size"] = size.into();
-            "fixed"
-        } else {
-            "dynamic"
+        let prefix = match options.index_type {
+            IndexType::Fixed(image_file_size) => {
+                if let Some(size) = image_file_size {
+                    param["size"] = size.into();
+                }
+                "fixed"
+            }
+            IndexType::Dynamic => "dynamic",
         };
 
         if options.encrypt && self.crypt_config.is_none() {
@@ -387,11 +400,14 @@ impl BackupWriter {
         let known_chunks = Arc::new(Mutex::new(HashSet::new()));
 
         let mut param = json!({ "archive-name": archive_name });
-        let prefix = if let Some(size) = options.fixed_size {
-            param["size"] = size.into();
-            "fixed"
-        } else {
-            "dynamic"
+        let prefix = match options.index_type {
+            IndexType::Fixed(image_file_size) => {
+                if let Some(size) = image_file_size {
+                    param["size"] = size.into();
+                }
+                "fixed"
+            }
+            IndexType::Dynamic => "dynamic",
         };
 
         if options.encrypt && self.crypt_config.is_none() {
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 999e5020..7fc711fd 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -46,7 +46,7 @@ use pbs_client::tools::{
 use pbs_client::{
     delete_ticket_info, parse_backup_specification, view_task_result, BackupDetectionMode,
     BackupReader, BackupRepository, BackupSpecificationType, BackupStats, BackupWriter,
-    BackupWriterOptions, ChunkStream, FixedChunkStream, HttpClient, InjectionData,
+    BackupWriterOptions, ChunkStream, FixedChunkStream, HttpClient, IndexType, InjectionData,
     PxarBackupStream, RemoteChunkReader, UploadOptions, BACKUP_SOURCE_SCHEMA,
 };
 use pbs_datastore::catalog::{BackupCatalogWriter, CatalogReader, CatalogWriter};
@@ -205,7 +205,7 @@ async fn backup_directory<P: AsRef<Path>>(
     pxar_create_options: pbs_client::pxar::PxarCreateOptions,
     upload_options: UploadOptions,
 ) -> Result<(BackupStats, Option<BackupStats>), Error> {
-    if upload_options.fixed_size.is_some() {
+    if let IndexType::Fixed(_) = upload_options.index_type {
         bail!("cannot backup directory with fixed chunk size!");
     }
 
@@ -295,7 +295,7 @@ async fn backup_image<P: AsRef<Path>>(
 
     let stream = FixedChunkStream::new(stream, chunk_size.unwrap_or(4 * 1024 * 1024));
 
-    if upload_options.fixed_size.is_none() {
+    if let IndexType::Dynamic = upload_options.index_type {
         bail!("cannot backup image with dynamic chunk size!");
     }
 
@@ -859,15 +859,17 @@ async fn create_backup(
                 upload_list.push((BackupSpecificationType::PXAR, filename, target, "didx", 0));
             }
             BackupSpecificationType::IMAGE => {
-                if !(file_type.is_file() || file_type.is_block_device()) {
-                    bail!("got unexpected file type (expected file or block device)");
-                }
-
-                let size = image_size(&PathBuf::from(&filename))?;
-
-                if size == 0 {
-                    bail!("got zero-sized file '{}'", filename);
-                }
+                let size = if file_type.is_file() || file_type.is_block_device() {
+                    let size = image_size(&PathBuf::from(&filename))?;
+                    if size == 0 {
+                        bail!("got zero-sized file '{}'", filename);
+                    }
+                    size
+                } else if file_type.is_fifo() {
+                    0
+                } else {
+                    bail!("got unexpected file type (expected file, block device, or fifo");
+                };
 
                 upload_list.push((
                     BackupSpecificationType::IMAGE,
@@ -1191,9 +1193,11 @@ async fn create_backup(
             (BackupSpecificationType::IMAGE, false) => {
                 log_file("image", &filename, target.as_ref());
 
+                // 0 means fifo pipe with unknown size
+                let image_file_size = (size != 0).then_some(size);
                 let upload_options = UploadOptions {
                     previous_manifest: previous_manifest.clone(),
-                    fixed_size: Some(size),
+                    index_type: IndexType::Fixed(image_file_size),
                     compress: true,
                     encrypt: crypto.mode == CryptMode::Encrypt,
                 };
diff --git a/src/server/push.rs b/src/server/push.rs
index d7884fce..b1b41297 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -17,7 +17,8 @@ use pbs_api_types::{
     PRIV_REMOTE_DATASTORE_MODIFY, PRIV_REMOTE_DATASTORE_PRUNE,
 };
 use pbs_client::{
-    BackupRepository, BackupWriter, BackupWriterOptions, HttpClient, MergedChunkInfo, UploadOptions,
+    BackupRepository, BackupWriter, BackupWriterOptions, HttpClient, IndexType, MergedChunkInfo,
+    UploadOptions,
 };
 use pbs_config::CachedUserInfo;
 use pbs_datastore::data_blob::ChunkInfo;
@@ -917,7 +918,7 @@ pub(crate) async fn push_snapshot(
                         index,
                         chunk_reader,
                         &backup_writer,
-                        None,
+                        IndexType::Dynamic,
                         known_chunks.clone(),
                     )
                     .await?;
@@ -944,7 +945,7 @@ pub(crate) async fn push_snapshot(
                         index,
                         chunk_reader,
                         &backup_writer,
-                        Some(size),
+                        IndexType::Fixed(Some(size)),
                         known_chunks.clone(),
                     )
                     .await?;
@@ -1002,7 +1003,7 @@ async fn push_index(
     index: impl IndexFile + Send + 'static,
     chunk_reader: Arc<dyn AsyncReadChunk>,
     backup_writer: &BackupWriter,
-    size: Option<u64>,
+    index_type: IndexType,
     known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
 ) -> Result<SyncStats, Error> {
     let (upload_channel_tx, upload_channel_rx) = mpsc::channel(20);
@@ -1048,7 +1049,7 @@ async fn push_index(
     let upload_options = UploadOptions {
         compress: true,
         encrypt: false,
-        fixed_size: size,
+        index_type,
         ..UploadOptions::default()
     };
 
-- 
2.47.3

[PATCH v5 proxmox-backup 10/16] client: treat minus sign as stdin

[Robert Obkircher]

Treat "-" as an alias for "/dev/stdin". If there is an actual file
with that name it can still be read via "./-".

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-backup-client/src/main.rs | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 7fc711fd..37878b01 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -845,6 +845,13 @@ async fn create_backup(
         }
         target_set.insert(target.clone());
 
+        // one can still use ./- to refer to an actual file with that name
+        let filename = if filename == "-" {
+            String::from("/dev/stdin")
+        } else {
+            filename
+        };
+
         use std::os::unix::fs::FileTypeExt;
 
         let metadata = std::fs::metadata(&filename)
-- 
2.47.3

[PATCH v5 proxmox-backup 11/16] datastore: combine public FixedIndexWriter methods into add_chunk.

[Robert Obkircher]

These operations were always performed together anyway, so it makes
sense to simplify the public api.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 24 +++++++++++++++++++++---
 src/api2/backup/environment.rs   |  6 +-----
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index c4fe899d..f50f4517 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -380,7 +380,7 @@ impl FixedIndexWriter {
     /// 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> {
+    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);
@@ -467,7 +467,7 @@ impl FixedIndexWriter {
         Ok(index_csum)
     }
 
-    pub fn check_chunk_alignment(&self, offset: usize, chunk_len: usize) -> Result<usize, Error> {
+    fn check_chunk_alignment(&self, offset: usize, chunk_len: usize) -> Result<usize, Error> {
         if offset < chunk_len {
             bail!("got chunk with small offset ({} < {}", offset, chunk_len);
         }
@@ -497,7 +497,7 @@ impl FixedIndexWriter {
         Ok(pos / self.chunk_size)
     }
 
-    pub fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
+    fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
         if index >= self.index_length {
             bail!(
                 "add digest failed - index out of range ({} >= {})",
@@ -519,6 +519,24 @@ impl FixedIndexWriter {
         Ok(())
     }
 
+    /// Write the digest of a chunk into this index file.
+    ///
+    /// The `start` and `size` parameters encode the range of
+    /// content that is backed up. It is verified that `start` is
+    /// aligned and that only the last chunk may be smaller.
+    ///
+    /// If this writer has been created without a fixed size, the
+    /// index capacity and content size are increased automatically
+    /// until an incomplete chunk is encountered.
+    pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
+        let Some(end) = start.checked_add(size.into()) else {
+            bail!("add_chunk: start and size are too large: {start}+{size}");
+        };
+        self.grow_to_size(end as usize)?;
+        let idx = self.check_chunk_alignment(end as usize, size as usize)?;
+        self.add_digest(idx, digest)
+    }
+
     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");
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index dc4f9bf5..04c5bf84 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -442,14 +442,10 @@ 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.index.add_chunk(offset, size, digest)?;
 
         data.chunk_count += 1;
 
-        data.index.add_digest(idx, digest)?;
-
         Ok(())
     }
 
-- 
2.47.3

[PATCH v5 proxmox-backup 12/16] datastore: use u64 instead of usize for fidx writer content size

[Robert Obkircher]

The file format is independent of the systems pointer size, so it
makes sense to use a fixed size type.

The chunk size is also a u64 to avoid casts and because that is what
is stored in the header anyway. Most other places limit it to u32, but
widening shouldn't be an issue.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   |  4 +-
 pbs-datastore/src/fixed_index.rs | 71 ++++++++++++++++----------------
 src/api2/backup/environment.rs   |  8 ++--
 src/api2/backup/mod.rs           |  8 ++--
 4 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 694a5dd0..69bde917 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -695,8 +695,8 @@ impl DataStore {
     pub fn create_fixed_writer<P: AsRef<Path>>(
         &self,
         filename: P,
-        size: Option<usize>,
-        chunk_size: usize,
+        size: Option<u64>,
+        chunk_size: u64,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
         FixedIndexWriter::create(full_path, size, chunk_size)
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index f50f4517..753dd5a5 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -214,8 +214,8 @@ pub struct FixedIndexWriter {
     file: File,
     filename: PathBuf,
     tmp_filename: PathBuf,
-    chunk_size: usize,
-    size: usize,
+    chunk_size: u64,
+    size: u64,
     index_length: usize,
     index_capacity: usize,
     index: *mut u8,
@@ -252,8 +252,8 @@ impl FixedIndexWriter {
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
         full_path: impl Into<PathBuf>,
-        known_size: Option<usize>,
-        chunk_size: usize,
+        known_size: Option<u64>,
+        chunk_size: u64,
     ) -> Result<Self, Error> {
         let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
@@ -291,10 +291,13 @@ impl FixedIndexWriter {
 
         file.write_all(&buffer)?;
 
-        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_length, index_capacity) = match known_size {
+            Some(s) => {
+                let len = s.div_ceil(chunk_size).try_into()?;
+                (len, len)
+            }
+            None => (0, Self::INITIAL_CAPACITY),
+        };
 
         let index_size = index_capacity * 32;
         nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
@@ -380,13 +383,13 @@ impl FixedIndexWriter {
     /// 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.
-    fn grow_to_size(&mut self, requested_size: usize) -> Result<(), Error> {
+    fn grow_to_size(&mut self, requested_size: u64) -> 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 {
+            let new_len = requested_size.div_ceil(self.chunk_size).try_into()?;
+            if new_len as u64 * 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)?;
@@ -410,7 +413,7 @@ impl FixedIndexWriter {
 
     /// The current total size of the referenced content.
     pub fn size(&self) -> u64 {
-        self.size as u64
+        self.size
     }
 
     fn unmap(&mut self) -> Result<(), Error> {
@@ -467,12 +470,10 @@ impl FixedIndexWriter {
         Ok(index_csum)
     }
 
-    fn check_chunk_alignment(&self, offset: usize, chunk_len: usize) -> Result<usize, Error> {
-        if offset < chunk_len {
+    fn check_chunk_alignment(&self, offset: u64, chunk_len: u64) -> Result<usize, Error> {
+        let Some(pos) = offset.checked_sub(chunk_len) else {
             bail!("got chunk with small offset ({} < {}", offset, chunk_len);
-        }
-
-        let pos = offset - chunk_len;
+        };
 
         if offset > self.size {
             bail!("chunk data exceeds size ({} >= {})", offset, self.size);
@@ -494,7 +495,7 @@ impl FixedIndexWriter {
             bail!("got unaligned chunk (pos = {})", pos);
         }
 
-        Ok(pos / self.chunk_size)
+        Ok((pos / self.chunk_size) as usize)
     }
 
     fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
@@ -528,12 +529,12 @@ impl FixedIndexWriter {
     /// If this writer has been created without a fixed size, the
     /// index capacity and content size are increased automatically
     /// until an incomplete chunk is encountered.
-    pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
-        let Some(end) = start.checked_add(size.into()) else {
+    pub fn add_chunk(&mut self, start: u64, size: u64, digest: &[u8; 32]) -> Result<(), Error> {
+        let Some(end) = start.checked_add(size) else {
             bail!("add_chunk: start and size are too large: {start}+{size}");
         };
-        self.grow_to_size(end as usize)?;
-        let idx = self.check_chunk_alignment(end as usize, size as usize)?;
+        self.grow_to_size(end)?;
+        let idx = self.check_chunk_alignment(end, size)?;
         self.add_digest(idx, digest)
     }
 
@@ -542,7 +543,7 @@ impl FixedIndexWriter {
             bail!("reusing the index is only supported with known input size");
         }
 
-        if self.chunk_size != reader.chunk_size {
+        if Ok(self.chunk_size) != reader.chunk_size.try_into() {
             bail!("can't reuse file with different chunk size");
         }
 
@@ -565,7 +566,7 @@ mod tests {
     use super::*;
     use crate::temp_test_dir::TempTestDir;
 
-    const CS: usize = 4096;
+    const CS: u64 = 4096;
 
     #[test]
     fn test_empty() {
@@ -611,7 +612,7 @@ mod tests {
 
         let initial = FixedIndexWriter::INITIAL_CAPACITY;
         let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
-        let expected = test_data(steps.last().unwrap() * CS);
+        let expected = test_data(*steps.last().unwrap() as u64 * CS);
 
         let mut begin = 0;
         for chunk_count in steps {
@@ -628,7 +629,7 @@ mod tests {
         w.close().unwrap();
         drop(w);
 
-        let size = expected.len() * CS;
+        let size = expected.len() as u64 * CS;
         check_with_reader(&path, size, &expected);
         compare_to_known_size_writer(&path, size, &expected);
     }
@@ -639,7 +640,7 @@ mod tests {
         let path = dir.join("test_grow_to_misaligned_size");
         let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
 
-        let size = (FixedIndexWriter::INITIAL_CAPACITY + 42) * CS - 1; // last is not full
+        let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 42) * CS - 1; // last is not full
         let expected = test_data(size);
 
         w.grow_to_size(size).unwrap();
@@ -661,8 +662,8 @@ mod tests {
     struct TestChunk {
         digest: [u8; 32],
         index: usize,
-        size: usize,
-        end: usize,
+        size: u64,
+        end: u64,
     }
 
     impl TestChunk {
@@ -675,7 +676,7 @@ mod tests {
         }
     }
 
-    fn test_data(size: usize) -> Vec<TestChunk> {
+    fn test_data(size: u64) -> Vec<TestChunk> {
         (0..size.div_ceil(CS))
             .map(|index| {
                 let mut digest = [0u8; 32];
@@ -690,24 +691,24 @@ mod tests {
                 };
                 TestChunk {
                     digest,
-                    index,
+                    index: index as usize,
                     size,
-                    end: index * CS + size,
+                    end: index as u64 * CS + size,
                 }
             })
             .collect()
     }
 
-    fn check_with_reader(path: &Path, size: usize, chunks: &[TestChunk]) {
+    fn check_with_reader(path: &Path, size: u64, chunks: &[TestChunk]) {
         let reader = FixedIndexReader::open(path).unwrap();
-        assert_eq!(size as u64, reader.index_bytes());
+        assert_eq!(size, reader.index_bytes());
         assert_eq!(chunks.len(), reader.index_count());
         for c in chunks {
             assert_eq!(&c.digest, reader.index_digest(c.index).unwrap());
         }
     }
 
-    fn compare_to_known_size_writer(file: &Path, size: usize, chunks: &[TestChunk]) {
+    fn compare_to_known_size_writer(file: &Path, size: u64, chunks: &[TestChunk]) {
         let mut path = file.to_path_buf();
         path.set_extension("reference");
         let mut w = FixedIndexWriter::create(&path, Some(size), CS).unwrap();
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 04c5bf84..9773a87a 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: Option<usize>,
+    size: Option<u64>,
     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: Option<usize>,
+        size: Option<u64>,
         chunk_size: u32,
         incremental: bool,
     ) -> Result<usize, Error> {
@@ -442,7 +442,7 @@ impl BackupEnvironment {
             );
         }
 
-        data.index.add_chunk(offset, size, digest)?;
+        data.index.add_chunk(offset, size.into(), digest)?;
 
         data.chunk_count += 1;
 
@@ -622,7 +622,7 @@ impl BackupEnvironment {
             }
 
             if let Some(known_size) = data.size {
-                if size != known_size as u64 {
+                if size != known_size {
                     bail!(
                         "fixed writer '{}' close failed - unexpected file size ({} != {})",
                         data.name,
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index c2822c18..55207e6c 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -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 = param["size"].as_u64().map(usize::try_from).transpose()?;
+    let size = param["size"].as_u64();
     let reuse_csum = param["reuse-csum"].as_str();
 
     let archive_name = name.clone();
@@ -528,13 +528,15 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
+    let mut writer = env
+        .datastore
+        .create_fixed_writer(&path, size, chunk_size.into())?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
     }
 
-    let wid = env.register_fixed_writer(writer, name, size, chunk_size as u32, incremental)?;
+    let wid = env.register_fixed_writer(writer, name, size, chunk_size, incremental)?;
 
     env.log(format!("created new fixed index {wid} ({path:?})"));
 
-- 
2.47.3

[PATCH v5 proxmox-backup 13/16] datastore: compute fidx file size with overflow checks

[Robert Obkircher]

Handle overflows and prevent potential inconsistencies by performing
the calcualtion in one place. This also serves as a preparation for
mmapping the entire file.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 753dd5a5..008339bb 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -300,7 +300,8 @@ impl FixedIndexWriter {
         };
 
         let index_size = index_capacity * 32;
-        nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
+        let file_size = Self::file_size(index_capacity)?;
+        nix::unistd::ftruncate(&file, file_size)?;
 
         let data = unsafe {
             nix::sys::mman::mmap(
@@ -332,6 +333,22 @@ impl FixedIndexWriter {
         })
     }
 
+    /// Computes the size of a fidx file containing `index_length`
+    /// chunk digests.
+    ///
+    /// Guarantees that the size fits into usize, isize and i64.
+    fn file_size(index_length: usize) -> Result<i64, Error> {
+        if index_length == 0 {
+            bail!("fidx file must have at least one chunk");
+        }
+        index_length
+            .checked_mul(32)
+            .and_then(|s| s.checked_add(size_of::<FixedIndexHeader>()))
+            .filter(|s| *s <= isize::MAX as usize)
+            .and_then(|s| i64::try_from(s).ok())
+            .ok_or_else(|| format_err!("fidx file size overflow for {index_length} chunks"))
+    }
+
     /// 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> {
@@ -340,7 +357,7 @@ impl FixedIndexWriter {
         }
         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 new_file_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.")
@@ -443,11 +460,9 @@ 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)?;
+        if self.index_length < self.index_capacity {
+            let file_size = Self::file_size(self.index_length)?;
+            nix::unistd::ftruncate(&self.file, file_size)?;
             self.index_capacity = self.index_length;
         }
 
-- 
2.47.3

[PATCH v5 proxmox-backup 14/16] datastore: support writing fidx files on systems with larger page size

[Robert Obkircher]

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 writes the entire page anyway to persist the checksum.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 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 008339bb..e8c14e5c 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<std::ffi::c_void>);
+
+impl MmapPtr {
+    fn header(&self) -> NonNull<FixedIndexHeader> {
+        self.0.cast::<FixedIndexHeader>()
+    }
+
+    fn index(&self) -> NonNull<u8> {
+        unsafe { self.0.byte_add(size_of::<FixedIndexHeader>()).cast::<u8>() }
+    }
+}
+
 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<MmapPtr>,
     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
@@ -259,7 +269,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)
@@ -278,19 +288,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()?;
@@ -299,23 +296,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::<u8>();
+        }?);
+
+        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,
@@ -325,11 +324,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(),
         })
     }
 
@@ -355,27 +353,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::<u8>();
+        self.memory = Some(MmapPtr(new_index));
         self.index_capacity = new_capacity;
         Ok(())
     }
@@ -388,7 +385,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)
         })
     }
@@ -434,30 +431,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 {
@@ -466,18 +465,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);
         }
@@ -522,13 +509,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

[PATCH v5 proxmox-backup 15/16] datastore: FixedIndexWriter: switch public chunk_size to u32

[Robert Obkircher]

I'm not sure if this patch should be applied.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   | 4 ++--
 pbs-datastore/src/fixed_index.rs | 6 +++---
 src/api2/backup/environment.rs   | 2 +-
 src/api2/backup/mod.rs           | 4 +---
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 69bde917..98eab18d 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -696,10 +696,10 @@ impl DataStore {
         &self,
         filename: P,
         size: Option<u64>,
-        chunk_size: u64,
+        chunk_size: u32,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
-        FixedIndexWriter::create(full_path, size, chunk_size)
+        FixedIndexWriter::create(full_path, size, chunk_size.into())
     }
 
     pub fn open_fixed_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index e8c14e5c..3fa146f8 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -531,12 +531,12 @@ impl FixedIndexWriter {
     /// If this writer has been created without a fixed size, the
     /// index capacity and content size are increased automatically
     /// until an incomplete chunk is encountered.
-    pub fn add_chunk(&mut self, start: u64, size: u64, digest: &[u8; 32]) -> Result<(), Error> {
-        let Some(end) = start.checked_add(size) else {
+    pub fn add_chunk(&mut self, start: u64, size: u32, digest: &[u8; 32]) -> Result<(), Error> {
+        let Some(end) = start.checked_add(size.into()) else {
             bail!("add_chunk: start and size are too large: {start}+{size}");
         };
         self.grow_to_size(end)?;
-        let idx = self.check_chunk_alignment(end, size)?;
+        let idx = self.check_chunk_alignment(end, size.into())?;
         self.add_digest(idx, digest)
     }
 
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 9773a87a..9645f6de 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -442,7 +442,7 @@ impl BackupEnvironment {
             );
         }
 
-        data.index.add_chunk(offset, size.into(), digest)?;
+        data.index.add_chunk(offset, size, digest)?;
 
         data.chunk_count += 1;
 
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 55207e6c..4d0846ca 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -528,9 +528,7 @@ fn create_fixed_index(
         reader = Some(index);
     }
 
-    let mut writer = env
-        .datastore
-        .create_fixed_writer(&path, size, chunk_size.into())?;
+    let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
 
     if let Some(reader) = reader {
         writer.clone_data_from(&reader)?;
-- 
2.47.3

[PATCH v5 proxmox-backup 16/16] datastore: FixedIndexWriter: switch internal chunk_size to u32

[Robert Obkircher]

I don't really like this change becasue it introduces quite a few
conversions.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs   |  2 +-
 pbs-datastore/src/fixed_index.rs | 43 ++++++++++++++++----------------
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 98eab18d..631bdc30 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -699,7 +699,7 @@ impl DataStore {
         chunk_size: u32,
     ) -> Result<FixedIndexWriter, Error> {
         let full_path = self.inner.chunk_store.relative_path(filename.as_ref());
-        FixedIndexWriter::create(full_path, size, chunk_size.into())
+        FixedIndexWriter::create(full_path, size, chunk_size)
     }
 
     pub fn open_fixed_reader<P: AsRef<Path>>(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 3fa146f8..ba6874fe 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -225,7 +225,7 @@ pub struct FixedIndexWriter {
     file: File,
     filename: PathBuf,
     tmp_filename: PathBuf,
-    chunk_size: u64,
+    chunk_size: u32,
     size: u64,
     index_length: usize,
     index_capacity: usize,
@@ -263,7 +263,7 @@ impl FixedIndexWriter {
     pub fn create(
         full_path: impl Into<PathBuf>,
         known_size: Option<u64>,
-        chunk_size: u64,
+        chunk_size: u32,
     ) -> Result<Self, Error> {
         let full_path = full_path.into();
         let mut tmp_path = full_path.clone();
@@ -290,7 +290,7 @@ impl FixedIndexWriter {
 
         let (index_length, index_capacity) = match known_size {
             Some(s) => {
-                let len = s.div_ceil(chunk_size).try_into()?;
+                let len = s.div_ceil(chunk_size.into()).try_into()?;
                 (len, len)
             }
             None => (0, Self::INITIAL_CAPACITY),
@@ -313,7 +313,7 @@ impl FixedIndexWriter {
         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.chunk_size = u64::to_le(chunk_size.into());
         header.uuid = *uuid.as_bytes();
 
         Ok(Self {
@@ -402,8 +402,9 @@ impl FixedIndexWriter {
             if !self.growable_size {
                 bail!("refusing to resize from {} to {requested_size}", self.size);
             }
-            let new_len = requested_size.div_ceil(self.chunk_size).try_into()?;
-            if new_len as u64 * self.chunk_size != requested_size {
+            let cs = u64::from(self.chunk_size);
+            let new_len = requested_size.div_ceil(cs).try_into()?;
+            if new_len as u64 * cs != 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)?;
@@ -472,8 +473,8 @@ impl FixedIndexWriter {
         Ok(index_csum)
     }
 
-    fn check_chunk_alignment(&self, offset: u64, chunk_len: u64) -> Result<usize, Error> {
-        let Some(pos) = offset.checked_sub(chunk_len) else {
+    fn check_chunk_alignment(&self, offset: u64, chunk_len: u32) -> Result<usize, Error> {
+        let Some(pos) = offset.checked_sub(chunk_len.into()) else {
             bail!("got chunk with small offset ({} < {}", offset, chunk_len);
         };
 
@@ -493,11 +494,11 @@ impl FixedIndexWriter {
             );
         }
 
-        if pos & (self.chunk_size - 1) != 0 {
+        if pos & (u64::from(self.chunk_size) - 1) != 0 {
             bail!("got unaligned chunk (pos = {})", pos);
         }
 
-        Ok((pos / self.chunk_size) as usize)
+        Ok((pos / u64::from(self.chunk_size)) as usize)
     }
 
     fn add_digest(&mut self, index: usize, digest: &[u8; 32]) -> Result<(), Error> {
@@ -536,7 +537,7 @@ impl FixedIndexWriter {
             bail!("add_chunk: start and size are too large: {start}+{size}");
         };
         self.grow_to_size(end)?;
-        let idx = self.check_chunk_alignment(end, size.into())?;
+        let idx = self.check_chunk_alignment(end, size)?;
         self.add_digest(idx, digest)
     }
 
@@ -568,7 +569,7 @@ mod tests {
     use super::*;
     use crate::temp_test_dir::TempTestDir;
 
-    const CS: u64 = 4096;
+    const CS: u32 = 4096;
 
     #[test]
     fn test_empty() {
@@ -594,7 +595,7 @@ mod tests {
         let path = dir.join("test_single_partial_chunk");
         let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
 
-        let size = CS - 1;
+        let size = CS as u64 - 1;
         let expected = test_data(size);
         w.grow_to_size(size).unwrap();
         expected[0].add_to(&mut w);
@@ -614,7 +615,7 @@ mod tests {
 
         let initial = FixedIndexWriter::INITIAL_CAPACITY;
         let steps = [1, 2, initial, initial + 1, 5 * initial, 10 * initial + 1];
-        let expected = test_data(*steps.last().unwrap() as u64 * CS);
+        let expected = test_data(*steps.last().unwrap() as u64 * CS as u64);
 
         let mut begin = 0;
         for chunk_count in steps {
@@ -631,7 +632,7 @@ mod tests {
         w.close().unwrap();
         drop(w);
 
-        let size = expected.len() as u64 * CS;
+        let size = expected.len() as u64 * CS as u64;
         check_with_reader(&path, size, &expected);
         compare_to_known_size_writer(&path, size, &expected);
     }
@@ -642,7 +643,7 @@ mod tests {
         let path = dir.join("test_grow_to_misaligned_size");
         let mut w = FixedIndexWriter::create(&path, None, CS).unwrap();
 
-        let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 42) * CS - 1; // last is not full
+        let size = (FixedIndexWriter::INITIAL_CAPACITY as u64 + 42) * CS as u64 - 1; // last is not full
         let expected = test_data(size);
 
         w.grow_to_size(size).unwrap();
@@ -664,7 +665,7 @@ mod tests {
     struct TestChunk {
         digest: [u8; 32],
         index: usize,
-        size: u64,
+        size: u32,
         end: u64,
     }
 
@@ -679,23 +680,23 @@ mod tests {
     }
 
     fn test_data(size: u64) -> Vec<TestChunk> {
-        (0..size.div_ceil(CS))
+        (0..size.div_ceil(CS.into()))
             .map(|index| {
                 let mut digest = [0u8; 32];
                 let i = &(index as u64).to_le_bytes();
                 for c in digest.chunks_mut(i.len()) {
                     c.copy_from_slice(i);
                 }
-                let size = if ((index + 1) * CS) <= size {
+                let size = if ((index + 1) * CS as u64) <= size {
                     CS
                 } else {
-                    size % CS
+                    (size % CS as u64) as u32
                 };
                 TestChunk {
                     digest,
                     index: index as usize,
                     size,
-                    end: index as u64 * CS + size,
+                    end: index as u64 * CS as u64 + size as u64,
                 }
             })
             .collect()
-- 
2.47.3