[PATCH proxmox{,-backup} 0/9] support index readers on larger page sizes

[PATCH proxmox{,-backup} 0/9] support index readers on larger page sizes

[Robert Obkircher]

Fix `proxmox-backup-client restore` and pbs in general on systems with
a larger page size.

Also ensure that `proxmox-backup-debug inspect` displays a consistent
size for fixed and dynamic index files, and include the total backup
size in the output.

proxmox:

Robert Obkircher (1):
  proxmox-sys: add len and as_non_null method to Mmap

 proxmox-sys/src/mmap.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)


proxmox-backup:

Robert Obkircher (8):
  datastore: limit scope of mutable reference in FixedIndexWriter::close
  datastore: add tests for fixed and dynamic index reader
  datastore: fix inconsistent implementation of index_size method
  bin: debug: print referenced backup size for index files
  datastore: check that index files are regular files
  datastore: simplify FixedIndexReader::chunk_info
  datastore: always access dynamic index reader slice through method
  fix #7244: datastore: always mmap index files from offset 0

 pbs-datastore/src/dynamic_index.rs      | 154 ++++++++++++-------
 pbs-datastore/src/fixed_index.rs        | 195 +++++++++++++-----------
 pbs-datastore/src/index.rs              |   5 +
 pbs-datastore/src/index_mmap.rs         | 171 +++++++++++++++++++++
 pbs-datastore/src/lib.rs                |   1 +
 src/bin/proxmox_backup_debug/inspect.rs |   4 +
 6 files changed, 387 insertions(+), 143 deletions(-)
 create mode 100644 pbs-datastore/src/index_mmap.rs


Summary over all repositories:
  7 files changed, 402 insertions(+), 143 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH proxmox 1/9] proxmox-sys: add len and as_non_null method to Mmap

[Robert Obkircher]

Allow access to the raw parts of the slice without materializing a
reference to it.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 proxmox-sys/src/mmap.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/proxmox-sys/src/mmap.rs b/proxmox-sys/src/mmap.rs
index e15ca1db..d54ed6e6 100644
--- a/proxmox-sys/src/mmap.rs
+++ b/proxmox-sys/src/mmap.rs
@@ -18,6 +18,7 @@ pub struct Mmap<T> {
     len: usize,
 }
 
+/// Memory can be used and unmapped on another thread.
 unsafe impl<T> Send for Mmap<T> where T: Send {}
 unsafe impl<T> Sync for Mmap<T> where T: Sync {}
 
@@ -68,6 +69,20 @@ impl<T> Mmap<T> {
         }
         .map_err(SysError::into_io_error)
     }
+
+    pub fn is_empty(&self) -> bool {
+        self.len == 0
+    }
+
+    /// Return the length (without creating a reference to the slice)
+    pub fn len(&self) -> usize {
+        self.len
+    }
+
+    /// Return the underlying pointer (without creating a reference to the slice)
+    pub fn as_non_null(&self) -> NonNull<T> {
+        self.data
+    }
 }
 
 impl<T> std::ops::Deref for Mmap<T> {
-- 
2.47.3

[PATCH proxmox-backup 2/9] datastore: limit scope of mutable reference in FixedIndexWriter::close

[Robert Obkircher]

Eliminate the variable to make it clearer that the reference won't be
accessed after the memory is unmapped.

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

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index b516bad62..2324fc3e0 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -464,8 +464,9 @@ impl FixedIndexWriter {
         };
 
         let index_size = self.index_length * 32;
-        let data = unsafe { std::slice::from_raw_parts(ptr.index().as_ptr(), index_size) };
-        let index_csum = openssl::sha::sha256(data);
+        let index_csum = openssl::sha::sha256(unsafe {
+            std::slice::from_raw_parts(ptr.index().as_ptr(), index_size)
+        });
 
         {
             let header = unsafe { ptr.header().as_mut() };
-- 
2.47.3

[PATCH proxmox-backup 3/9] datastore: add tests for fixed and dynamic index reader

[Robert Obkircher]

Focus on the error paths to make it more visible if they change during
refactoring.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/dynamic_index.rs | 77 ++++++++++++++++++++++++++
 pbs-datastore/src/fixed_index.rs   | 86 ++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index a0d9369a5..7ee8bdcab 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -586,3 +586,80 @@ impl<R: ReadChunk> ReadAt for LocalDynamicReadAt<R> {
         panic!("LocalDynamicReadAt::start_read_at returned Pending");
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::fs;
+
+    use openssl::sha::sha256;
+    use tempfile::TempDir;
+
+    use super::*;
+
+    #[track_caller]
+    fn check_error_contains<T>(result: Result<T, Error>, substring: &str) {
+        let error = result.map(|_| ()).unwrap_err().to_string();
+        assert!(
+            error.contains(substring),
+            "'{error}' does not contain '{substring}'"
+        );
+    }
+
+    #[test]
+    fn test_index_reader() {
+        let dir = TempDir::new().unwrap();
+
+        check_error_contains(
+            DynamicIndexReader::open(Path::new("/dev/stdin")),
+            "Illegal seek",
+        );
+        check_error_contains(
+            DynamicIndexReader::open(Path::new("/dev/zero")),
+            "index too small (0)",
+        );
+
+        let path = dir.path().join("file");
+        check_error_contains(DynamicIndexReader::open(&path), "No such file or directory");
+
+        fs::write(&path, []).unwrap();
+        check_error_contains(DynamicIndexReader::open(&path), "index too small (0)");
+
+        let mut data = vec![0; size_of::<DynamicIndexHeader>()];
+
+        fs::write(&path, &data).unwrap();
+        check_error_contains(DynamicIndexReader::open(&path), "got unknown magic number");
+
+        data[..8].copy_from_slice(&file_formats::DYNAMIC_SIZED_CHUNK_INDEX_1_0);
+        fs::write(&path, &data).unwrap();
+        check_error_contains(
+            DynamicIndexReader::open(&path),
+            "mapped length must not be zero",
+        );
+
+        data.extend_from_slice(&[0, 1, 2]);
+        fs::write(&path, &data).unwrap();
+        check_error_contains(DynamicIndexReader::open(&path), "got unexpected file size");
+
+        data.extend(3u8..80);
+        fs::write(&path, &data).unwrap();
+        let reader = DynamicIndexReader::open(&path).unwrap();
+        let size = u64::from_le_bytes(std::array::from_fn(|i| i as u8 + 40));
+        assert_eq!(reader.index_bytes(), size);
+        assert_eq!(reader.index_size(), 4096 + 80);
+        assert_eq!(reader.index_count(), 2);
+        assert_eq!(
+            reader.index_digest(0),
+            Some(&std::array::from_fn(|i| i as u8 + 8))
+        );
+        assert_eq!(
+            reader.index_digest(1),
+            Some(&std::array::from_fn(|i| i as u8 + 48))
+        );
+        assert_eq!(
+            reader.compute_csum(),
+            (sha256(&data[size_of::<DynamicIndexHeader>()..]), size,)
+        );
+
+        dir.close().unwrap();
+    }
+}
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 2324fc3e0..8104cbe72 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -585,12 +585,98 @@ impl FixedIndexWriter {
 #[cfg(test)]
 mod tests {
     use std::fs;
+
+    use openssl::sha::sha256;
     use tempfile::TempDir;
 
     use super::*;
 
     const CS: u32 = 4096;
 
+    #[track_caller]
+    fn check_error_contains<T>(result: Result<T, Error>, substring: &str) {
+        let error = result.map(|_| ()).unwrap_err().to_string();
+        assert!(
+            error.contains(substring),
+            "'{error}' does not contain '{substring}'"
+        );
+    }
+
+    #[test]
+    fn test_index_reader() {
+        let dir = TempDir::new().unwrap();
+
+        check_error_contains(
+            FixedIndexReader::open(Path::new("/dev/stdin")),
+            "Illegal seek",
+        );
+        check_error_contains(
+            FixedIndexReader::open(Path::new("/dev/zero")),
+            "index too small (0)",
+        );
+
+        let path = dir.path().join("file");
+        check_error_contains(FixedIndexReader::open(&path), "No such file or directory");
+
+        fs::write(&path, []).unwrap();
+        check_error_contains(FixedIndexReader::open(&path), "index too small (0)");
+
+        let mut data = vec![0; size_of::<FixedIndexHeader>()];
+
+        fs::write(&path, &data).unwrap();
+        check_error_contains(FixedIndexReader::open(&path), "got unknown magic number");
+
+        data[..8].copy_from_slice(&file_formats::FIXED_SIZED_CHUNK_INDEX_1_0);
+        fs::write(&path, &data).unwrap();
+        check_error_contains(
+            FixedIndexReader::open(&path),
+            "got non-power-of-two chunk size: 0",
+        );
+
+        let chunk_size = 4096 * 1024u64;
+        data[72..][..8].copy_from_slice(&chunk_size.to_le_bytes());
+        fs::write(&path, &data).unwrap();
+        check_error_contains(FixedIndexReader::open(&path), "invalid index size"); // 0
+
+        let size = chunk_size + 1;
+        data[64..][..8].copy_from_slice(&size.to_le_bytes());
+        data.extend_from_slice(&[0, 1, 2]);
+        fs::write(&path, &data).unwrap();
+        check_error_contains(
+            FixedIndexReader::open(&path),
+            "got unexpected file size (64 != 3)",
+        );
+
+        data.extend(3u8..32);
+        fs::write(&path, &data).unwrap();
+        check_error_contains(
+            FixedIndexReader::open(&path),
+            "got unexpected file size (64 != 32)",
+        );
+
+        data.extend(32u8..64);
+        fs::write(&path, &data).unwrap();
+        let reader = FixedIndexReader::open(&path).unwrap();
+        assert_eq!(reader.index_bytes(), size);
+        assert_eq!(reader.index_size(), size as usize);
+        assert_eq!(reader.chunk_size, chunk_size as usize);
+        assert_eq!(reader.index_count(), 2);
+        assert_eq!(
+            reader.index_digest(0),
+            Some(&std::array::from_fn(|i| i as u8))
+        );
+        assert_eq!(
+            reader.index_digest(1),
+            Some(&std::array::from_fn(|i| i as u8 + 32))
+        );
+        assert_eq!(
+            reader.compute_csum(),
+            (sha256(&data[size_of::<FixedIndexHeader>()..]), size)
+        );
+
+        dir.close().unwrap();
+    }
+
     #[test]
     fn test_empty() {
         let dir = TempDir::new().unwrap();
-- 
2.47.3

[PATCH proxmox-backup 4/9] datastore: fix inconsistent implementation of index_size method

[Robert Obkircher]

Return the file size of fixed index files, just like for dynamic ones.
Previously, the size of the referenced backup contents was returned,
which is still available via the index_bytes method.

The impact of this change should be fairly small, as this only seems
to be used by "proxmox-backup-debug inspect file". The original
intention was probably to show the file size there, because it also
does that for blobs.

Fixes: f0d23e537 ("add ctime and size function to IndexFile trait")
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/fixed_index.rs | 4 ++--
 pbs-datastore/src/index.rs       | 5 +++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 8104cbe72..0145f93d3 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -187,7 +187,7 @@ impl IndexFile for FixedIndexReader {
     }
 
     fn index_size(&self) -> usize {
-        self.size as usize
+        size_of::<FixedIndexHeader>() + self.index_length * 32
     }
 
     fn compute_csum(&self) -> ([u8; 32], u64) {
@@ -658,7 +658,7 @@ mod tests {
         fs::write(&path, &data).unwrap();
         let reader = FixedIndexReader::open(&path).unwrap();
         assert_eq!(reader.index_bytes(), size);
-        assert_eq!(reader.index_size(), size as usize);
+        assert_eq!(reader.index_size(), 4096 + 64);
         assert_eq!(reader.chunk_size, chunk_size as usize);
         assert_eq!(reader.index_count(), 2);
         assert_eq!(
diff --git a/pbs-datastore/src/index.rs b/pbs-datastore/src/index.rs
index b5d20ad91..4c0e9aefb 100644
--- a/pbs-datastore/src/index.rs
+++ b/pbs-datastore/src/index.rs
@@ -20,9 +20,14 @@ impl ChunkReadInfo {
 pub trait IndexFile {
     fn index_count(&self) -> usize;
     fn index_digest(&self, pos: usize) -> Option<&[u8; 32]>;
+
+    /// The total size of the referenced backup content.
     fn index_bytes(&self) -> u64;
+
     fn chunk_info(&self, pos: usize) -> Option<ChunkReadInfo>;
     fn index_ctime(&self) -> i64;
+
+    /// The size of the index file including the header.
     fn index_size(&self) -> usize;
 
     /// Get the chunk index and the relative offset within it for a byte offset
-- 
2.47.3

[PATCH proxmox-backup 5/9] bin: debug: print referenced backup size for index files

[Robert Obkircher]

This might be useful and it makes it clearer that "size" refers to the
index file itself.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/bin/proxmox_backup_debug/inspect.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bin/proxmox_backup_debug/inspect.rs b/src/bin/proxmox_backup_debug/inspect.rs
index 73b5b83a6..ccdc0bcee 100644
--- a/src/bin/proxmox_backup_debug/inspect.rs
+++ b/src/bin/proxmox_backup_debug/inspect.rs
@@ -304,6 +304,7 @@ fn inspect_file(
             json!({
                 "size": index.index_size(),
                 "ctime": ctime_str,
+                "referenced-bytes": index.index_bytes(),
                 "chunk-digests": chunk_digests
             })
         }
@@ -314,6 +315,9 @@ fn inspect_file(
 
     if output_format == "text" {
         println!("size: {}", val["size"]);
+        if let Some(count) = val["referenced-bytes"].as_number() {
+            println!("referenced bytes: {count}");
+        }
         if let Some(encryption) = val["encryption"].as_str() {
             println!("encryption: {encryption}");
         }
-- 
2.47.3

[PATCH proxmox-backup 6/9] datastore: check that index files are regular files

[Robert Obkircher]

Return an explicit error message when trying to mmap something other
than a regular file. Previously, this was caught implicitly by the
non-zero size check or by a failing seek.

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

diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index 7ee8bdcab..dd0b86f3f 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -8,6 +8,7 @@ use std::sync::{Arc, Mutex};
 use std::task::Context;
 
 use anyhow::{Error, bail, format_err};
+use nix::sys::stat::{SFlag, fstat};
 
 use proxmox_io::ReadExt;
 use proxmox_sys::mmap::Mmap;
@@ -96,17 +97,16 @@ impl DynamicIndexReader {
     }
 
     pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
+        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
+        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
+            bail!("not a regular file");
+        }
+
         // FIXME: This is NOT OUR job! Check the callers of this method and remove this!
         file.seek(SeekFrom::Start(0))?;
 
         let header_size = std::mem::size_of::<DynamicIndexHeader>();
 
-        let rawfd = file.as_raw_fd();
-        let stat = match nix::sys::stat::fstat(rawfd) {
-            Ok(stat) => stat,
-            Err(err) => bail!("fstat failed - {}", err),
-        };
-
         let size = stat.st_size as usize;
 
         if size < header_size {
@@ -611,11 +611,11 @@ mod tests {
 
         check_error_contains(
             DynamicIndexReader::open(Path::new("/dev/stdin")),
-            "Illegal seek",
+            "not a regular file",
         );
         check_error_contains(
             DynamicIndexReader::open(Path::new("/dev/zero")),
-            "index too small (0)",
+            "not a regular file",
         );
 
         let path = dir.path().join("file");
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 0145f93d3..1d3d0170a 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -5,6 +5,7 @@ use std::path::{Path, PathBuf};
 use std::ptr::NonNull;
 
 use anyhow::{Context, Error, bail, format_err};
+use nix::sys::stat::{SFlag, fstat};
 
 use proxmox_io::ReadExt;
 use proxmox_uuid::Uuid;
@@ -60,15 +61,15 @@ impl FixedIndexReader {
     }
 
     pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
+        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
+        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
+            bail!("not a regular file");
+        }
+
         file.seek(SeekFrom::Start(0))?;
 
         let header_size = std::mem::size_of::<FixedIndexHeader>();
 
-        let stat = match nix::sys::stat::fstat(file.as_raw_fd()) {
-            Ok(stat) => stat,
-            Err(err) => bail!("fstat failed - {}", err),
-        };
-
         let size = stat.st_size as usize;
 
         if size < header_size {
@@ -608,11 +609,11 @@ mod tests {
 
         check_error_contains(
             FixedIndexReader::open(Path::new("/dev/stdin")),
-            "Illegal seek",
+            "not a regular file",
         );
         check_error_contains(
             FixedIndexReader::open(Path::new("/dev/zero")),
-            "index too small (0)",
+            "not a regular file",
         );
 
         let path = dir.path().join("file");
-- 
2.47.3

[PATCH proxmox-backup 7/9] datastore: simplify FixedIndexReader::chunk_info

[Robert Obkircher]

This is mainly done so the index_length field can be removed.

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

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 1d3d0170a..29927aeba 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -165,18 +165,11 @@ impl IndexFile for FixedIndexReader {
     }
 
     fn chunk_info(&self, pos: usize) -> Option<ChunkReadInfo> {
-        if pos >= self.index_length {
-            return None;
-        }
+        let digest = self.index_digest(pos)?;
 
         let start = (pos * self.chunk_size) as u64;
-        let mut end = start + self.chunk_size as u64;
-
-        if end > self.size {
-            end = self.size;
-        }
+        let end = (start + self.chunk_size as u64).min(self.size);
 
-        let digest = self.index_digest(pos).unwrap();
         Some(ChunkReadInfo {
             range: start..end,
             digest: *digest,
-- 
2.47.3

[PATCH proxmox-backup 8/9] datastore: always access dynamic index reader slice through method

[Robert Obkircher]

Reduce the size of the diff in the upcoming mmap changes, where the
index field will be removed.

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

diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index dd0b86f3f..c03ab6c29 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -147,18 +147,18 @@ impl DynamicIndexReader {
 
     #[inline]
     pub fn chunk_end(&self, pos: usize) -> u64 {
-        if pos >= self.index.len() {
+        if pos >= self.index().len() {
             panic!("chunk index out of range");
         }
-        self.index[pos].end()
+        self.index()[pos].end()
     }
 
     #[inline]
     fn chunk_digest(&self, pos: usize) -> &[u8; 32] {
-        if pos >= self.index.len() {
+        if pos >= self.index().len() {
             panic!("chunk index out of range");
         }
-        &self.index[pos].digest
+        &self.index()[pos].digest
     }
 
     pub fn binary_search(
@@ -189,11 +189,11 @@ impl DynamicIndexReader {
 
 impl IndexFile for DynamicIndexReader {
     fn index_count(&self) -> usize {
-        self.index.len()
+        self.index().len()
     }
 
     fn index_digest(&self, pos: usize) -> Option<&[u8; 32]> {
-        if pos >= self.index.len() {
+        if pos >= self.index().len() {
             None
         } else {
             Some(self.chunk_digest(pos))
@@ -201,10 +201,10 @@ impl IndexFile for DynamicIndexReader {
     }
 
     fn index_bytes(&self) -> u64 {
-        if self.index.is_empty() {
+        if self.index().is_empty() {
             0
         } else {
-            self.chunk_end(self.index.len() - 1)
+            self.chunk_end(self.index().len() - 1)
         }
     }
 
@@ -222,20 +222,20 @@ impl IndexFile for DynamicIndexReader {
     }
 
     fn chunk_info(&self, pos: usize) -> Option<ChunkReadInfo> {
-        if pos >= self.index.len() {
+        if pos >= self.index().len() {
             return None;
         }
         let start = if pos == 0 {
             0
         } else {
-            self.index[pos - 1].end()
+            self.index()[pos - 1].end()
         };
 
-        let end = self.index[pos].end();
+        let end = self.index()[pos].end();
 
         Some(ChunkReadInfo {
             range: start..end,
-            digest: self.index[pos].digest,
+            digest: self.index()[pos].digest,
         })
     }
 
@@ -248,7 +248,7 @@ impl IndexFile for DynamicIndexReader {
     }
 
     fn chunk_from_offset(&self, offset: u64) -> Option<(usize, u64)> {
-        let end_idx = self.index.len() - 1;
+        let end_idx = self.index().len() - 1;
         let end = self.chunk_end(end_idx);
         let found_idx = self.binary_search(0, 0, end_idx, end, offset);
         let found_idx = match found_idx {
-- 
2.47.3

[PATCH proxmox-backup 9/9] fix #7244: datastore: always mmap index files from offset 0

[Robert Obkircher]

Always memory-map index files from the start, to allow running on
systems where the page size is larger than 4 kB. Previously, such
systems received an error because the offset needs to be a multiple of
the page size.

Since the header is now also mapped into memory, it is read that way
as well. This avoids the seek and read, but it may slightly increase
the risk of encountering SIGBUS on the first access.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/dynamic_index.rs |  63 +++--------
 pbs-datastore/src/fixed_index.rs   | 102 ++++-------------
 pbs-datastore/src/index_mmap.rs    | 171 +++++++++++++++++++++++++++++
 pbs-datastore/src/lib.rs           |   1 +
 4 files changed, 207 insertions(+), 130 deletions(-)
 create mode 100644 pbs-datastore/src/index_mmap.rs

diff --git a/pbs-datastore/src/dynamic_index.rs b/pbs-datastore/src/dynamic_index.rs
index c03ab6c29..0f609a635 100644
--- a/pbs-datastore/src/dynamic_index.rs
+++ b/pbs-datastore/src/dynamic_index.rs
@@ -1,17 +1,13 @@
 use std::fs::File;
 use std::io::{BufWriter, Seek, SeekFrom, Write};
 use std::ops::Range;
-use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::pin::Pin;
 use std::sync::{Arc, Mutex};
 use std::task::Context;
 
 use anyhow::{Error, bail, format_err};
-use nix::sys::stat::{SFlag, fstat};
 
-use proxmox_io::ReadExt;
-use proxmox_sys::mmap::Mmap;
 use proxmox_uuid::Uuid;
 use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
 
@@ -19,6 +15,7 @@ use pbs_tools::lru_cache::LruCache;
 
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
+use crate::index_mmap::IndexMmap;
 use crate::read_chunk::ReadChunk;
 
 /// Header format definition for dynamic index files (`.dixd`)
@@ -76,9 +73,8 @@ impl DynamicEntry {
 }
 
 pub struct DynamicIndexReader {
+    mmap: IndexMmap<DynamicIndexHeader, DynamicEntry>,
     _file: File,
-    pub size: usize,
-    index: Mmap<DynamicEntry>,
     pub uuid: [u8; 16],
     pub ctime: i64,
     pub index_csum: [u8; 32],
@@ -93,55 +89,28 @@ impl DynamicIndexReader {
     }
 
     pub fn index(&self) -> &[DynamicEntry] {
-        &self.index
+        self.mmap.index()
     }
 
-    pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
-        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
-        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
-            bail!("not a regular file");
-        }
-
-        // FIXME: This is NOT OUR job! Check the callers of this method and remove this!
-        file.seek(SeekFrom::Start(0))?;
-
-        let header_size = std::mem::size_of::<DynamicIndexHeader>();
-
-        let size = stat.st_size as usize;
+    pub fn new(file: File) -> Result<Self, Error> {
+        let mmap = unsafe { IndexMmap::map_read(&file)? };
 
-        if size < header_size {
-            bail!("index too small ({})", stat.st_size);
-        }
-
-        let header: Box<DynamicIndexHeader> = unsafe { file.read_host_value_boxed()? };
+        let header: &DynamicIndexHeader = mmap.header();
 
         if header.magic != file_formats::DYNAMIC_SIZED_CHUNK_INDEX_1_0 {
             bail!("got unknown magic number");
         }
 
-        let index_size = stat.st_size as usize - header_size;
-        let index_count = index_size / 40;
-        if index_count * 40 != index_size {
-            bail!("got unexpected file size");
+        if mmap.index().is_empty() {
+            bail!("index must not be empty");
         }
 
-        let index = unsafe {
-            Mmap::map_fd(
-                &file,
-                header_size as u64,
-                index_count,
-                nix::sys::mman::ProtFlags::PROT_READ,
-                nix::sys::mman::MapFlags::MAP_PRIVATE,
-            )?
-        };
-
         Ok(Self {
             _file: file,
-            size,
-            index,
             ctime: i64::from_le(header.ctime),
             uuid: header.uuid,
             index_csum: header.index_csum,
+            mmap,
         })
     }
 
@@ -244,7 +213,7 @@ impl IndexFile for DynamicIndexReader {
     }
 
     fn index_size(&self) -> usize {
-        self.size
+        self.mmap.size()
     }
 
     fn chunk_from_offset(&self, offset: u64) -> Option<(usize, u64)> {
@@ -622,7 +591,7 @@ mod tests {
         check_error_contains(DynamicIndexReader::open(&path), "No such file or directory");
 
         fs::write(&path, []).unwrap();
-        check_error_contains(DynamicIndexReader::open(&path), "index too small (0)");
+        check_error_contains(DynamicIndexReader::open(&path), "file too small (0)");
 
         let mut data = vec![0; size_of::<DynamicIndexHeader>()];
 
@@ -631,14 +600,14 @@ mod tests {
 
         data[..8].copy_from_slice(&file_formats::DYNAMIC_SIZED_CHUNK_INDEX_1_0);
         fs::write(&path, &data).unwrap();
-        check_error_contains(
-            DynamicIndexReader::open(&path),
-            "mapped length must not be zero",
-        );
+        check_error_contains(DynamicIndexReader::open(&path), "index must not be empty");
 
         data.extend_from_slice(&[0, 1, 2]);
         fs::write(&path, &data).unwrap();
-        check_error_contains(DynamicIndexReader::open(&path), "got unexpected file size");
+        check_error_contains(
+            DynamicIndexReader::open(&path),
+            "index size is not divisible by element size: 3",
+        );
 
         data.extend(3u8..80);
         fs::write(&path, &data).unwrap();
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 29927aeba..e8197fb01 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -1,17 +1,14 @@
 use std::fs::File;
-use std::io::{Seek, SeekFrom};
-use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
 use std::ptr::NonNull;
 
 use anyhow::{Context, Error, bail, format_err};
-use nix::sys::stat::{SFlag, fstat};
 
-use proxmox_io::ReadExt;
 use proxmox_uuid::Uuid;
 
 use crate::file_formats;
 use crate::index::{ChunkReadInfo, IndexFile};
+use crate::index_mmap::IndexMmap;
 
 /// Header format definition for fixed index files (`.fidx`)
 #[repr(C)]
@@ -30,28 +27,15 @@ proxmox_lang::static_assert_size!(FixedIndexHeader, 4096);
 // split image into fixed size chunks
 
 pub struct FixedIndexReader {
+    mmap: IndexMmap<FixedIndexHeader, [u8; 32]>,
     _file: File,
     pub chunk_size: usize,
     pub size: u64,
-    index_length: usize,
-    index: *mut u8,
     pub uuid: [u8; 16],
     pub ctime: i64,
     pub index_csum: [u8; 32],
 }
 
-// `index` is mmap()ed which cannot be thread-local so should be sendable
-unsafe impl Send for FixedIndexReader {}
-unsafe impl Sync for FixedIndexReader {}
-
-impl Drop for FixedIndexReader {
-    fn drop(&mut self) {
-        if let Err(err) = self.unmap() {
-            log::error!("Unable to unmap file - {}", err);
-        }
-    }
-}
-
 impl FixedIndexReader {
     pub fn open(path: &Path) -> Result<Self, Error> {
         File::open(path)
@@ -60,23 +44,10 @@ impl FixedIndexReader {
             .map_err(|err| format_err!("Unable to open fixed index {:?} - {}", path, err))
     }
 
-    pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
-        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
-        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
-            bail!("not a regular file");
-        }
-
-        file.seek(SeekFrom::Start(0))?;
-
-        let header_size = std::mem::size_of::<FixedIndexHeader>();
-
-        let size = stat.st_size as usize;
-
-        if size < header_size {
-            bail!("index too small ({})", stat.st_size);
-        }
+    pub fn new(file: File) -> Result<Self, Error> {
+        let mmap = unsafe { IndexMmap::map_read(&file)? };
 
-        let header: Box<FixedIndexHeader> = unsafe { file.read_host_value_boxed()? };
+        let header: &FixedIndexHeader = mmap.header();
 
         if header.magic != file_formats::FIXED_SIZED_CHUNK_INDEX_1_0 {
             bail!("got unknown magic number");
@@ -93,71 +64,36 @@ impl FixedIndexReader {
         let index_length = size.div_ceil(chunk_size) as usize;
         let index_size = index_length * 32;
 
-        let expected_index_size = (stat.st_size as usize) - header_size;
+        let expected_index_size = size_of_val(mmap.index());
         if index_size != expected_index_size {
-            bail!(
-                "got unexpected file size ({} != {})",
-                index_size,
-                expected_index_size
-            );
+            bail!("got unexpected index size ({index_size} != {expected_index_size})",);
         }
 
-        let chunk_size = usize::try_from(chunk_size)?;
+        if mmap.index().is_empty() {
+            bail!("index must not be empty");
+        }
 
-        let data = unsafe {
-            nix::sys::mman::mmap(
-                None,
-                std::num::NonZeroUsize::new(index_size)
-                    .ok_or_else(|| format_err!("invalid index size"))?,
-                nix::sys::mman::ProtFlags::PROT_READ,
-                nix::sys::mman::MapFlags::MAP_PRIVATE,
-                &file,
-                header_size as i64,
-            )
-        }?
-        .as_ptr()
-        .cast::<u8>();
+        let chunk_size = usize::try_from(chunk_size)?;
 
         Ok(Self {
             _file: file,
             chunk_size,
             size,
-            index_length,
-            index: data,
             ctime,
             uuid: header.uuid,
             index_csum: header.index_csum,
+            mmap,
         })
     }
-
-    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_length * 32;
-
-        if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
-            bail!("unmap file failed - {}", err);
-        }
-
-        self.index = std::ptr::null_mut();
-
-        Ok(())
-    }
 }
 
 impl IndexFile for FixedIndexReader {
     fn index_count(&self) -> usize {
-        self.index_length
+        self.mmap.index().len()
     }
 
     fn index_digest(&self, pos: usize) -> Option<&[u8; 32]> {
-        if pos >= self.index_length {
-            None
-        } else {
-            Some(unsafe { &*(self.index.add(pos * 32) as *const [u8; 32]) })
-        }
+        self.mmap.index().get(pos)
     }
 
     fn index_bytes(&self) -> u64 {
@@ -181,7 +117,7 @@ impl IndexFile for FixedIndexReader {
     }
 
     fn index_size(&self) -> usize {
-        size_of::<FixedIndexHeader>() + self.index_length * 32
+        self.mmap.size()
     }
 
     fn compute_csum(&self) -> ([u8; 32], u64) {
@@ -613,7 +549,7 @@ mod tests {
         check_error_contains(FixedIndexReader::open(&path), "No such file or directory");
 
         fs::write(&path, []).unwrap();
-        check_error_contains(FixedIndexReader::open(&path), "index too small (0)");
+        check_error_contains(FixedIndexReader::open(&path), "file too small (0)");
 
         let mut data = vec![0; size_of::<FixedIndexHeader>()];
 
@@ -630,7 +566,7 @@ mod tests {
         let chunk_size = 4096 * 1024u64;
         data[72..][..8].copy_from_slice(&chunk_size.to_le_bytes());
         fs::write(&path, &data).unwrap();
-        check_error_contains(FixedIndexReader::open(&path), "invalid index size"); // 0
+        check_error_contains(FixedIndexReader::open(&path), "index must not be empty");
 
         let size = chunk_size + 1;
         data[64..][..8].copy_from_slice(&size.to_le_bytes());
@@ -638,14 +574,14 @@ mod tests {
         fs::write(&path, &data).unwrap();
         check_error_contains(
             FixedIndexReader::open(&path),
-            "got unexpected file size (64 != 3)",
+            "index size is not divisible by element size: 3",
         );
 
         data.extend(3u8..32);
         fs::write(&path, &data).unwrap();
         check_error_contains(
             FixedIndexReader::open(&path),
-            "got unexpected file size (64 != 32)",
+            "got unexpected index size (64 != 32)",
         );
 
         data.extend(32u8..64);
diff --git a/pbs-datastore/src/index_mmap.rs b/pbs-datastore/src/index_mmap.rs
new file mode 100644
index 000000000..af5010458
--- /dev/null
+++ b/pbs-datastore/src/index_mmap.rs
@@ -0,0 +1,171 @@
+use std::fs::File;
+use std::marker::PhantomData;
+use std::os::unix::io::AsRawFd;
+use std::ptr::NonNull;
+
+use anyhow::{Error, bail, format_err};
+use nix::sys::mman::{MapFlags, ProtFlags};
+use nix::sys::stat::{SFlag, fstat};
+
+use proxmox_sys::mmap::Mmap;
+
+pub struct IndexMmap<H, T> {
+    _header: PhantomData<H>,
+    _index: PhantomData<[T]>,
+    /// used for the Drop, Send, and Sync impls
+    memory: Mmap<u8>,
+    index_len: usize,
+}
+
+impl<H, T> IndexMmap<H, T> {
+    /// Map a read-only file consisting of a header followed by an array into memory.
+    ///
+    /// SAFETY:
+    /// * `H` and `T` must be:
+    ///     * plain data types
+    ///     * of non-zero size
+    ///     * without interior mutability
+    ///     * where any bit pattern is valid.
+    /// * `file` *must not* be modified by anyone else!
+    ///     * modifying the bytes while a reference exists is UB
+    ///     * truncation may cause SIGBUS on access
+    pub unsafe fn map_read(file: &File) -> Result<Self, Error> {
+        let stat = fstat(file.as_raw_fd()).map_err(|e| format_err!("fstat failed - {e}"))?;
+        if (stat.st_mode & SFlag::S_IFMT.bits()) != SFlag::S_IFREG.bits() {
+            bail!("not a regular file");
+        }
+
+        let size = usize::try_from(stat.st_size)
+            .map_err(|_| format_err!("file too small ({})", stat.st_size))?;
+
+        let index_size = size
+            .checked_sub(size_of::<H>())
+            .ok_or_else(|| format_err!("file too small ({size})"))?;
+
+        let index_len = index_size
+            .checked_div(size_of::<T>())
+            .filter(|l| l * size_of::<T>() == index_size)
+            .ok_or_else(|| {
+                format_err!("index size is not divisible by element size: {index_size}")
+            })?;
+
+        let (prot, flags) = (ProtFlags::PROT_READ, MapFlags::MAP_PRIVATE);
+
+        let memory = unsafe { Mmap::map_fd(file, 0, size, prot, flags) }
+            .map_err(|e| format_err!("mmap failed - {e}"))?;
+
+        let mmap = Self {
+            _header: PhantomData,
+            _index: PhantomData,
+            memory,
+            index_len,
+        };
+        if !mmap.header_ptr().is_aligned() || !mmap.index_ptr().is_aligned() {
+            bail!("alignment error");
+        }
+        Ok(mmap)
+    }
+
+    fn header_ptr(&self) -> NonNull<H> {
+        self.memory.as_non_null().cast::<H>()
+    }
+
+    fn index_ptr(&self) -> NonNull<T> {
+        let ptr = self.memory.as_non_null();
+        unsafe { ptr.byte_add(size_of::<H>()).cast::<T>() }
+    }
+
+    pub fn header(&self) -> &H {
+        unsafe { self.header_ptr().as_ref() }
+    }
+
+    pub fn index(&self) -> &[T] {
+        unsafe { NonNull::slice_from_raw_parts(self.index_ptr(), self.index_len).as_ref() }
+    }
+
+    pub fn size(&self) -> usize {
+        self.memory.len()
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::fs;
+    use std::fs::File;
+    use std::path::Path;
+
+    use tempfile::TempDir;
+
+    use super::*;
+
+    #[track_caller]
+    fn check_error_contains<T>(result: Result<T, Error>, substring: &str) {
+        let error = result.map(|_| ()).unwrap_err().to_string();
+        assert!(
+            error.contains(substring),
+            "'{error}' does not contain '{substring}'"
+        );
+    }
+
+    fn map<H, T>(path: &Path) -> Result<IndexMmap<H, T>, Error> {
+        let file = File::open(path).unwrap();
+        unsafe { IndexMmap::<H, T>::map_read(&file) }
+    }
+
+    #[repr(align(8))]
+    struct A8(u64);
+
+    type A1 = u8;
+
+    #[test]
+    fn test_empty() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, &[]).unwrap();
+        check_error_contains(map::<A8, A8>(&path), "file too small (0)");
+        check_error_contains(map::<A1, A1>(&path), "file too small (0)");
+    }
+
+    #[test]
+    fn test_small() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, [1, 2, 3, 4, 5, 6, 7]).unwrap();
+        check_error_contains(map::<A8, A1>(&path), "file too small (7)");
+
+        let mmap = map::<A1, A1>(&path).unwrap();
+        assert_eq!(mmap.size(), 7);
+        assert_eq!(mmap.header(), &1);
+        assert_eq!(mmap.index(), &[2, 3, 4, 5, 6, 7]);
+    }
+
+    #[test]
+    fn test_divisibility_and_alignment() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, [1; 33]).unwrap();
+        check_error_contains(
+            map::<A8, A8>(&path),
+            "index size is not divisible by element size: 25",
+        );
+
+        // could be prevented statically, but it makes a good test for the sanity check
+        check_error_contains(map::<A1, A8>(&path), "alignment error");
+    }
+
+    #[test]
+    fn test_medium() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("file");
+
+        fs::write(&path, [1; 32]).unwrap();
+
+        let mmap = map::<A8, A8>(&path).unwrap();
+        assert_eq!(mmap.size(), 32);
+        assert_eq!(mmap.header().0, 0x0101010101010101);
+        assert_eq!(mmap.index().len(), 3);
+    }
+}
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index d9d7a4cb6..0da9c65fa 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -202,6 +202,7 @@ pub mod task_tracking;
 
 pub mod dynamic_index;
 pub mod fixed_index;
+mod index_mmap;
 
 pub use backup_info::{BackupDir, BackupGroup, BackupInfo};
 pub use checksum_reader::ChecksumReader;
-- 
2.47.3