[pbs-devel] [PATCH v4 proxmox-backup 00/11] fix: #3847 pipe from STDIN to proxmox-backup-client

[pbs-devel] [PATCH v4 proxmox-backup 00/11] 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 v3:

From feedback:
- remove "fix" prefix from commits and ensure they all build
- preserve optional size in BackupEnvironment instead of updating it
- combine public methods of writer and remove Arc<ChunkStore>
- tests: use tmpdir, better names, additional asserts

New:
- use u64 instead of usize for content size
- centralize size computation and improve overflow checks
- mmap entire file to support larger page sizes
  - I'm working on separate patches to fix the Readers 
    (for #7244 - Support 16k page size)


Robert Obkircher (11):
  datastore: support writing fidx files of unknown size
  datastore: remove Arc<ChunkStore> from FixedIndexWriter
  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

 pbs-client/src/backup_writer.rs   |  38 ++-
 pbs-datastore/src/datastore.rs    |  14 +-
 pbs-datastore/src/fixed_index.rs  | 448 ++++++++++++++++++++++++++----
 proxmox-backup-client/src/main.rs |  37 ++-
 src/api2/backup/environment.rs    |  33 ++-
 src/api2/backup/mod.rs            |   4 +-
 src/server/push.rs                |  11 +-
 7 files changed, 471 insertions(+), 114 deletions(-)

-- 
2.47.3



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

[pbs-devel] [PATCH v4 proxmox-backup 01/11] 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 for efficiency. Truncate
excessive capacity after encountering a non-full block or on close.

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

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 7ad3d917..a7a7302f 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 index = FixedIndexWriter::create(
diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 6c3be2d4..c1b69763 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -219,9 +219,12 @@ pub struct FixedIndexWriter {
     chunk_size: usize,
     size: usize,
     index_length: usize,
+    index_capacity: usize,
     index: *mut u8,
     pub uuid: [u8; 16],
     pub ctime: i64,
+    growable_size: bool,
+    write_size_on_close: bool,
 }
 
 // `index` is mmap()ed which cannot be thread-local so should be sendable
@@ -237,12 +240,18 @@ impl Drop for FixedIndexWriter {
 }
 
 impl FixedIndexWriter {
+    /// The initial capacity, if the total size is unknown.
+    ///
+    /// This capacity takes up the same amount of space as the header
+    /// and can refer to 128 Blocks * 4 MiB/Block = 512 MiB of content.
+    const INITIAL_CAPACITY: usize = 4096 / 32;
+
     #[allow(clippy::cast_ptr_alignment)]
     // Requires obtaining a shared chunk store lock beforehand
     pub fn create(
         store: Arc<ChunkStore>,
         path: &Path,
-        size: usize,
+        known_size: Option<usize>,
         chunk_size: usize,
     ) -> Result<Self, Error> {
         let full_path = store.relative_path(path);
@@ -264,6 +273,7 @@ impl FixedIndexWriter {
         }
 
         let ctime = proxmox_time::epoch_i64();
+        let size = known_size.unwrap_or(0);
 
         let uuid = Uuid::generate();
 
@@ -280,8 +290,12 @@ impl FixedIndexWriter {
 
         file.write_all(&buffer)?;
 
-        let index_length = size.div_ceil(chunk_size);
-        let index_size = index_length * 32;
+        let (index_length, index_capacity) = known_size
+            .map(|s| s.div_ceil(chunk_size))
+            .map(|len| (len, len))
+            .unwrap_or((0, Self::INITIAL_CAPACITY));
+
+        let index_size = index_capacity * 32;
         nix::unistd::ftruncate(&file, (header_size + index_size) as i64)?;
 
         let data = unsafe {
@@ -305,12 +319,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. This may be increased with [`grow_to_size`].
     pub fn index_length(&self) -> usize {
         self.index_length
     }
@@ -320,7 +412,7 @@ impl FixedIndexWriter {
             return Ok(());
         };
 
-        let index_size = self.index_length * 32;
+        let index_size = self.index_capacity * 32;
 
         if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
             bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
@@ -342,9 +434,24 @@ impl FixedIndexWriter {
 
         self.unmap()?;
 
+        if self.index_length == 0 {
+            bail!("refusing to close empty fidx file {:?}", self.tmp_filename);
+        } else if self.index_length < self.index_capacity {
+            let file_size = size_of::<FixedIndexHeader>() + index_size;
+            nix::unistd::ftruncate(&self.file, file_size as i64)?;
+            self.index_capacity = self.index_length;
+        }
+
         let csum_offset = std::mem::offset_of!(FixedIndexHeader, index_csum);
         self.file.seek(SeekFrom::Start(csum_offset as u64))?;
         self.file.write_all(&index_csum)?;
+
+        if self.write_size_on_close {
+            let size_offset = std::mem::offset_of!(FixedIndexHeader, size);
+            self.file.seek(SeekFrom::Start(size_offset as u64))?;
+            self.file.write_all(&(self.size as u64).to_le_bytes())?;
+        }
+
         self.file.flush()?;
 
         if let Err(err) = std::fs::rename(&self.tmp_filename, &self.filename) {
@@ -407,6 +514,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



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

[pbs-devel] [PATCH v4 proxmox-backup 02/11] 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 taking 3 seconds to create 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 a7a7302f..56dfce6e 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -698,14 +698,8 @@ impl DataStore {
         size: Option<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 c1b69763..39c83ca8 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};
 
@@ -249,12 +247,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>,
         known_size: Option<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



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

[pbs-devel] [PATCH v4 proxmox-backup 03/11] 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 | 190 +++++++++++++++++++++++++++++++
 1 file changed, 190 insertions(+)

diff --git a/pbs-datastore/src/fixed_index.rs b/pbs-datastore/src/fixed_index.rs
index 39c83ca8..4290da08 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -530,3 +530,193 @@ impl FixedIndexWriter {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::env;
+    use std::fs;
+
+    const CS: usize = 4096;
+
+    #[test]
+    fn test_empty() {
+        let dir = TempDir::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 = TempDir::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 = TempDir::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 = TempDir::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 TempDir(PathBuf);
+
+    impl TempDir {
+        fn new() -> Self {
+            TempDir(proxmox_sys::fs::make_tmp_dir(env::temp_dir(), None).unwrap())
+        }
+    }
+
+    impl std::ops::Deref for TempDir {
+        type Target = Path;
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+
+    impl Drop for TempDir {
+        fn drop(&mut self) {
+            fs::remove_dir_all(&self.0).unwrap();
+        }
+    }
+
+    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);
+    }
+}
-- 
2.47.3



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

[pbs-devel] [PATCH v4 proxmox-backup 04/11] 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



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

[pbs-devel] [PATCH v4 proxmox-backup 05/11] 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 4290da08..ecfe9b28 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -404,6 +404,11 @@ impl FixedIndexWriter {
         self.index_length
     }
 
+    /// The total size of the referenced content. This may be increased with [`grow_to_size`].
+    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



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

[pbs-devel] [PATCH v4 proxmox-backup 06/11] 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 that
now stores an optional total size for the Fixed variant.

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



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

[pbs-devel] [PATCH v4 proxmox-backup 07/11] 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



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

[pbs-devel] [PATCH v4 proxmox-backup 08/11] datastore: combine public FixedIndexWriter methods into add_chunk.

[Robert Obkircher]

These operations were always perfomed 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 ecfe9b28..056ae07b 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -376,7 +376,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);
@@ -463,7 +463,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);
         }
@@ -493,7 +493,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 ({} >= {})",
@@ -515,6 +515,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



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

[pbs-devel] [PATCH v4 proxmox-backup 09/11] datastore: use u64 instead of usize for fidx writer content size

[Robert Obkircher]

This is closer to what the file format supports.

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

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 56dfce6e..8770d942 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -695,11 +695,11 @@ impl DataStore {
     pub fn create_fixed_writer<P: AsRef<Path>>(
         &self,
         filename: P,
-        size: Option<usize>,
-        chunk_size: usize,
+        size: Option<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 056ae07b..c2888372 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,
@@ -248,8 +248,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();
@@ -287,10 +287,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)?;
@@ -376,13 +379,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)?;
@@ -463,12 +466,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);
@@ -490,7 +491,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> {
@@ -524,12 +525,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)
     }
 
@@ -538,7 +539,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");
         }
 
@@ -560,7 +561,7 @@ mod tests {
     use std::env;
     use std::fs;
 
-    const CS: usize = 4096;
+    const CS: u64 = 4096;
 
     #[test]
     fn test_empty() {
@@ -606,7 +607,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 {
@@ -623,7 +624,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);
     }
@@ -634,7 +635,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();
@@ -677,8 +678,8 @@ mod tests {
     struct TestChunk {
         digest: [u8; 32],
         index: usize,
-        size: usize,
-        end: usize,
+        size: u64,
+        end: u64,
     }
 
     impl TestChunk {
@@ -691,7 +692,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];
@@ -706,24 +707,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..7d49d47c 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;
 
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index c2822c18..54445efa 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();
-- 
2.47.3



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

[pbs-devel] [PATCH v4 proxmox-backup 10/11] datastore: compute fidx file size with overflow checks

[Robert Obkircher]

This also serves as a preparation for mmapping the entire file to
support page sizes that are larger than the file header.

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 c2888372..dbb24111 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -296,7 +296,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(
@@ -328,6 +329,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> {
@@ -336,7 +353,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.")
@@ -439,11 +456,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



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

[pbs-devel] [PATCH v4 proxmox-backup 11/11] 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 will write 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 dbb24111..5fbb2d93 100644
--- a/pbs-datastore/src/fixed_index.rs
+++ b/pbs-datastore/src/fixed_index.rs
@@ -1,5 +1,4 @@
 use std::fs::File;
-use std::io::Write;
 use std::io::{Seek, SeekFrom};
 use std::os::unix::io::AsRawFd;
 use std::path::{Path, PathBuf};
@@ -210,6 +209,18 @@ impl IndexFile for FixedIndexReader {
     }
 }
 
+struct MmapPtr(NonNull<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
@@ -255,7 +265,7 @@ impl FixedIndexWriter {
         let mut tmp_path = full_path.clone();
         tmp_path.set_extension("tmp_fidx");
 
-        let mut file = std::fs::OpenOptions::new()
+        let file = std::fs::OpenOptions::new()
             .create(true)
             .truncate(true)
             .read(true)
@@ -274,19 +284,6 @@ impl FixedIndexWriter {
 
         let uuid = Uuid::generate();
 
-        let buffer = vec![0u8; header_size];
-        let header = unsafe { &mut *(buffer.as_ptr() as *mut FixedIndexHeader) };
-
-        header.magic = file_formats::FIXED_SIZED_CHUNK_INDEX_1_0;
-        header.ctime = i64::to_le(ctime);
-        header.size = u64::to_le(size as u64);
-        header.chunk_size = u64::to_le(chunk_size as u64);
-        header.uuid = *uuid.as_bytes();
-
-        header.index_csum = [0u8; 32];
-
-        file.write_all(&buffer)?;
-
         let (index_length, index_capacity) = match known_size {
             Some(s) => {
                 let len = s.div_ceil(chunk_size).try_into()?;
@@ -295,23 +292,25 @@ impl FixedIndexWriter {
             None => (0, Self::INITIAL_CAPACITY),
         };
 
-        let index_size = index_capacity * 32;
         let file_size = Self::file_size(index_capacity)?;
         nix::unistd::ftruncate(&file, file_size)?;
 
-        let data = unsafe {
+        let memory = MmapPtr(unsafe {
             nix::sys::mman::mmap(
                 None,
-                std::num::NonZeroUsize::new(index_size)
-                    .ok_or_else(|| format_err!("invalid index size"))?,
+                std::num::NonZeroUsize::new(file_size as usize).expect("has header"),
                 nix::sys::mman::ProtFlags::PROT_READ | nix::sys::mman::ProtFlags::PROT_WRITE,
                 nix::sys::mman::MapFlags::MAP_SHARED,
                 &file,
-                header_size as i64,
+                0,
             )
-        }?
-        .as_ptr()
-        .cast::<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,
@@ -321,11 +320,10 @@ impl FixedIndexWriter {
             size,
             index_length,
             index_capacity,
-            index: data,
+            memory: Some(memory),
             ctime,
             uuid: *uuid.as_bytes(),
             growable_size: known_size.is_none(),
-            write_size_on_close: known_size.is_none(),
         })
     }
 
@@ -351,27 +349,26 @@ impl FixedIndexWriter {
         if new_capacity == self.index_capacity {
             return Ok(());
         }
-        let old_index_size = self.index_capacity * 32;
-        let new_index_size = new_capacity * 32;
-        let new_file_size = Self::file_size(new_capacity)?;
+        let old_size = Self::file_size(self.index_capacity)?;
+        let new_size = Self::file_size(new_capacity)?;
 
-        let index_addr = NonNull::new(self.index as *mut std::ffi::c_void).ok_or_else(|| {
-            format_err!("Can't resize FixedIndexWriter index because the index pointer is null.")
-        })?;
+        let Some(MmapPtr(index_addr)) = self.memory else {
+            bail!("Can't resize unmapped FixedIndexWriter");
+        };
 
-        nix::unistd::ftruncate(&self.file, new_file_size)?;
+        nix::unistd::ftruncate(&self.file, new_size)?;
 
         let new_index = unsafe {
             nix::sys::mman::mremap(
                 index_addr,
-                old_index_size,
-                new_index_size,
+                old_size as usize,
+                new_size as usize,
                 nix::sys::mman::MRemapFlags::MREMAP_MAYMOVE,
                 None,
             )
         }?;
 
-        self.index = new_index.as_ptr().cast::<u8>();
+        self.memory = Some(MmapPtr(new_index));
         self.index_capacity = new_capacity;
         Ok(())
     }
@@ -384,7 +381,7 @@ impl FixedIndexWriter {
                 "failed to resize index capacity from {} to {new_capacity} with backing file: {:?}",
                 self.index_capacity, self.tmp_filename
             );
-            assert!(self.index.is_null(), "{message} {unmap_result:?}");
+            assert!(self.memory.is_none(), "{message} {unmap_result:?}");
             e.context(message)
         })
     }
@@ -430,30 +427,32 @@ impl FixedIndexWriter {
     }
 
     fn unmap(&mut self) -> Result<(), Error> {
-        let Some(index) = NonNull::new(self.index as *mut std::ffi::c_void) else {
-            return Ok(());
-        };
-
-        let index_size = self.index_capacity * 32;
-
-        if let Err(err) = unsafe { nix::sys::mman::munmap(index, index_size) } {
-            bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
+        if let Some(ptr) = self.memory.take() {
+            let len = Self::file_size(self.index_capacity).expect(
+                "this is the capacity that didn't cause an overflow when the memory was mapped",
+            );
+            if let Err(err) = unsafe { nix::sys::mman::munmap(ptr.0, len as usize) } {
+                bail!("unmap file {:?} failed - {}", self.tmp_filename, err);
+            }
         }
-
-        self.index = std::ptr::null_mut();
-
         Ok(())
     }
 
     pub fn close(&mut self) -> Result<[u8; 32], Error> {
-        if self.index.is_null() {
+        let Some(ptr) = &self.memory else {
             bail!("cannot close already closed index file.");
-        }
+        };
 
         let index_size = self.index_length * 32;
-        let data = unsafe { std::slice::from_raw_parts(self.index, index_size) };
+        let data = unsafe { std::slice::from_raw_parts(ptr.index().as_ptr(), index_size) };
         let index_csum = openssl::sha::sha256(data);
 
+        {
+            let header = unsafe { ptr.header().as_mut() };
+            header.index_csum = index_csum;
+            header.size = self.size.to_le();
+        }
+
         self.unmap()?;
 
         if self.index_length < self.index_capacity {
@@ -462,18 +461,6 @@ impl FixedIndexWriter {
             self.index_capacity = self.index_length;
         }
 
-        let csum_offset = std::mem::offset_of!(FixedIndexHeader, index_csum);
-        self.file.seek(SeekFrom::Start(csum_offset as u64))?;
-        self.file.write_all(&index_csum)?;
-
-        if self.write_size_on_close {
-            let size_offset = std::mem::offset_of!(FixedIndexHeader, size);
-            self.file.seek(SeekFrom::Start(size_offset as u64))?;
-            self.file.write_all(&(self.size as u64).to_le_bytes())?;
-        }
-
-        self.file.flush()?;
-
         if let Err(err) = std::fs::rename(&self.tmp_filename, &self.filename) {
             bail!("Atomic rename file {:?} failed - {}", self.filename, err);
         }
@@ -518,13 +505,13 @@ impl FixedIndexWriter {
             );
         }
 
-        if self.index.is_null() {
+        let Some(ptr) = &self.memory else {
             bail!("cannot write to closed index file.");
-        }
+        };
 
         let index_pos = index * 32;
         unsafe {
-            let dst = self.index.add(index_pos);
+            let dst = ptr.index().as_ptr().add(index_pos);
             dst.copy_from_nonoverlapping(digest.as_ptr(), 32);
         }
 
-- 
2.47.3



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