* [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction
@ 2025-10-08 15:21 Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 01/12] datastore: gc: inline single callsite method Christian Ebner
` (11 more replies)
0 siblings, 12 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
These patches fix 2 issues with the current s3 backend implementation
and reduce code duplication.
Patch 1 to 3 rework the garbage collection, deduplicating the common atime
check and garbage collection status update logic and marks the chunk removal
function signature as unsafe, as this requires specific pre-conditions.
Patch 4 to 8 fix an issue which could lead to backup restores failing
during concurrent backups, if the local datastore cache was small, leading
to chunks being evicted from the cache by truncating the contents, while the
chunk reader still accessed the chunk. This is now circumvented by replacing
the chunk file instead, leaving the contents on the already opened file
handle for the reader still accessible.
The remaining patches fix a possible race condition between s3 backend upload
and garbage collection, which can result in chunk loss. If the chunk upload
finished, garbage collection listed and checked the chunk's in-use marker, just
before it being written by the cache insert after the upload, garbage collection
can incorrectly delete the chunk. This is circumvented by setting and checking
an additional chunk marker file, which is created before starting the upload
and removed after cache insert, assuring that these chunks are not removed.
Changes since version 1 (thanks @Fabian):
- Refactor the garbage collection rework patches, using a callback to perform the
chunk removal, so both filesystem and s3 backend can use the same logic without
the need to readapt the gc status.
- Completely reworked the local datastore cache access method, so it not only
serves the contents from s3 backend if that needs to be fetched, but also
closes the download/insert race and drops quite some duplicate code,
completely getting rid of the now obsolete S3Cacher
- Rework the chunk insert for s3 to also cover cases were concurrent uploads of
the same object/key occurs, making sure that the upload marker creation will
not lead to failure and that the upload marker cleanup is handled correctly as
well. The only race still open is which of the two concurrent uploads inserts
to the local cache, but since both versions must encode for the same data (
as they have the same digest), this is not an issue. If one of the upload
fails however, both must be considered as failed, since then there is no
guarantee anymore that garbage collection did not cleanup the chunks from the
s3 backend in the meantime.
proxmox-backup:
Christian Ebner (12):
datastore: gc: inline single callsite method
gc: chunk store: rework atime check and gc status into common helper
chunk store: add unsafe signature to cache remove method
local store cache: replace evicted cache chunks instead of truncate
local store cache: serve response fetched from s3 backend
local store cache: refactor fetch and insert of chunks for s3 backend
local store cache: rework access cache fetching and insert logic
local store cache: drop obsolete cacher implementation
chunk store: refactor method for chunk insertion
api: chunk upload: fix race between chunk backend upload and insert
api: chunk upload: fix race with garbage collection for no-cache on s3
pull: guard chunk upload and only insert into cache after upload
pbs-datastore/src/chunk_store.rs | 287 +++++++++++++++---
pbs-datastore/src/datastore.rs | 171 +++++------
pbs-datastore/src/local_chunk_reader.rs | 27 +-
.../src/local_datastore_lru_cache.rs | 166 ++++------
src/api2/backup/upload_chunk.rs | 33 +-
src/api2/config/datastore.rs | 2 +
src/api2/reader/mod.rs | 34 +--
src/server/pull.rs | 16 +-
8 files changed, 449 insertions(+), 287 deletions(-)
Summary over all repositories:
8 files changed, 449 insertions(+), 287 deletions(-)
--
Generated by git-murpp 0.8.1
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 01/12] datastore: gc: inline single callsite method
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 02/12] gc: chunk store: rework atime check and gc status into common helper Christian Ebner
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
This method only has a single callsite and is better split by
deduplicating common code in `ChunkStore::swipe_unused_chunks`.
Therefore, inline the method in preparation for the following
code deduplication.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/datastore.rs | 127 +++++++++++++--------------------
1 file changed, 49 insertions(+), 78 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 2e62590f9..c37df94b7 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1653,21 +1653,56 @@ impl DataStore {
let lock = self.inner.chunk_store.mutex().lock().unwrap();
for content in list_bucket_result.contents {
- if self
- .mark_chunk_for_object_key(
- &content.key,
- content.size,
- min_atime,
- oldest_writer,
- &mut delete_list,
- &mut gc_status,
- )
- .with_context(|| {
- format!("failed to mark chunk for object key {}", content.key)
- })?
- {
- chunk_count += 1;
+ let (chunk_path, digest) = match self.chunk_path_from_object_key(&content.key) {
+ Some(path) => path,
+ None => continue,
+ };
+
+ // Check local markers (created or atime updated during phase1) and
+ // keep or delete chunk based on that.
+ let atime = match std::fs::metadata(&chunk_path) {
+ Ok(stat) => stat.accessed()?,
+ Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+ // File not found, delete by setting atime to unix epoch
+ info!("Not found, mark for deletion: {}", content.key);
+ SystemTime::UNIX_EPOCH
+ }
+ Err(err) => return Err(err.into()),
+ };
+ let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
+
+ let bad = chunk_path
+ .as_path()
+ .extension()
+ .is_some_and(|ext| ext == "bad");
+
+ if atime < min_atime {
+ if let Some(cache) = self.cache() {
+ // ignore errors, phase 3 will retry cleanup anyways
+ let _ = cache.remove(&digest);
+ }
+ delete_list.push(content.key);
+ if bad {
+ gc_status.removed_bad += 1;
+ } else {
+ gc_status.removed_chunks += 1;
+ }
+ gc_status.removed_bytes += content.size;
+ } else if atime < oldest_writer {
+ if bad {
+ gc_status.still_bad += 1;
+ } else {
+ gc_status.pending_chunks += 1;
+ }
+ gc_status.pending_bytes += content.size;
+ } else {
+ if !bad {
+ gc_status.disk_chunks += 1;
+ }
+ gc_status.disk_bytes += content.size;
}
+
+ chunk_count += 1;
}
drop(lock);
@@ -1796,70 +1831,6 @@ impl DataStore {
Ok(())
}
- // Mark the chunk marker in the local cache store for the given object key as in use
- // by updating it's atime.
- // Returns Ok(true) if the chunk was updated and Ok(false) if the object was not a chunk.
- fn mark_chunk_for_object_key(
- &self,
- object_key: &S3ObjectKey,
- size: u64,
- min_atime: i64,
- oldest_writer: i64,
- delete_list: &mut Vec<S3ObjectKey>,
- gc_status: &mut GarbageCollectionStatus,
- ) -> Result<bool, Error> {
- let (chunk_path, digest) = match self.chunk_path_from_object_key(object_key) {
- Some(path) => path,
- None => return Ok(false),
- };
-
- // Check local markers (created or atime updated during phase1) and
- // keep or delete chunk based on that.
- let atime = match std::fs::metadata(&chunk_path) {
- Ok(stat) => stat.accessed()?,
- Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
- // File not found, delete by setting atime to unix epoch
- info!("Not found, mark for deletion: {object_key}");
- SystemTime::UNIX_EPOCH
- }
- Err(err) => return Err(err.into()),
- };
- let atime = atime.duration_since(SystemTime::UNIX_EPOCH)?.as_secs() as i64;
-
- let bad = chunk_path
- .as_path()
- .extension()
- .is_some_and(|ext| ext == "bad");
-
- if atime < min_atime {
- if let Some(cache) = self.cache() {
- // ignore errors, phase 3 will retry cleanup anyways
- let _ = cache.remove(&digest);
- }
- delete_list.push(object_key.clone());
- if bad {
- gc_status.removed_bad += 1;
- } else {
- gc_status.removed_chunks += 1;
- }
- gc_status.removed_bytes += size;
- } else if atime < oldest_writer {
- if bad {
- gc_status.still_bad += 1;
- } else {
- gc_status.pending_chunks += 1;
- }
- gc_status.pending_bytes += size;
- } else {
- if !bad {
- gc_status.disk_chunks += 1;
- }
- gc_status.disk_bytes += size;
- }
-
- Ok(true)
- }
-
// Check and generate a chunk path from given object key
fn chunk_path_from_object_key(&self, object_key: &S3ObjectKey) -> Option<(PathBuf, [u8; 32])> {
// Check object is actually a chunk
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 02/12] gc: chunk store: rework atime check and gc status into common helper
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 01/12] datastore: gc: inline single callsite method Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 03/12] chunk store: add unsafe signature to cache remove method Christian Ebner
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Use the shared code paths for both, filesystem and s3 backend to a
common helper to avoid code duplication and adapt callsites
accordingly.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 90 +++++++++++++++++++++-----------
pbs-datastore/src/datastore.rs | 41 ++++++---------
2 files changed, 76 insertions(+), 55 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 3c59612bb..dc247886a 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -408,37 +408,28 @@ impl ChunkStore {
chunk_count += 1;
- if stat.st_atime < min_atime {
- //let age = now - stat.st_atime;
- //println!("UNLINK {} {:?}", age/(3600*24), filename);
- if let Err(err) = unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir) {
- if bad {
- status.still_bad += 1;
+ Self::check_atime_and_update_gc_status(
+ stat.st_atime,
+ min_atime,
+ oldest_writer,
+ stat.st_size as u64,
+ bad,
+ status,
+ |status| {
+ if let Err(err) =
+ unlinkat(Some(dirfd), filename, UnlinkatFlags::NoRemoveDir)
+ {
+ if bad {
+ status.still_bad += 1;
+ }
+ bail!(
+ "unlinking chunk {filename:?} failed on store '{}' - {err}",
+ self.name,
+ );
}
- bail!(
- "unlinking chunk {filename:?} failed on store '{}' - {err}",
- self.name,
- );
- }
- if bad {
- status.removed_bad += 1;
- } else {
- status.removed_chunks += 1;
- }
- status.removed_bytes += stat.st_size as u64;
- } else if stat.st_atime < oldest_writer {
- if bad {
- status.still_bad += 1;
- } else {
- status.pending_chunks += 1;
- }
- status.pending_bytes += stat.st_size as u64;
- } else {
- if !bad {
- status.disk_chunks += 1;
- }
- status.disk_bytes += stat.st_size as u64;
- }
+ Ok(())
+ },
+ )?;
}
drop(lock);
}
@@ -446,6 +437,45 @@ impl ChunkStore {
Ok(())
}
+ /// Check within what range the provided chunks atime falls and update the garbage collection
+ /// status accordingly.
+ ///
+ /// If the chunk should be removed, the [`remove_callback`] is executed.
+ pub(super) fn check_atime_and_update_gc_status<
+ T: FnOnce(&mut GarbageCollectionStatus) -> Result<(), Error>,
+ >(
+ atime: i64,
+ min_atime: i64,
+ oldest_writer: i64,
+ size: u64,
+ bad: bool,
+ gc_status: &mut GarbageCollectionStatus,
+ remove_callback: T,
+ ) -> Result<(), Error> {
+ if atime < min_atime {
+ remove_callback(gc_status)?;
+ if bad {
+ gc_status.removed_bad += 1;
+ } else {
+ gc_status.removed_chunks += 1;
+ }
+ gc_status.removed_bytes += size;
+ } else if atime < oldest_writer {
+ if bad {
+ gc_status.still_bad += 1;
+ } else {
+ gc_status.pending_chunks += 1;
+ }
+ gc_status.pending_bytes += size;
+ } else {
+ if !bad {
+ gc_status.disk_chunks += 1;
+ }
+ gc_status.disk_bytes += size;
+ }
+ Ok(())
+ }
+
/// Check if atime updates are honored by the filesystem backing the chunk store.
///
/// Checks if the atime is always updated by utimensat taking into consideration the Linux
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index c37df94b7..7ef16c31e 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1676,31 +1676,22 @@ impl DataStore {
.extension()
.is_some_and(|ext| ext == "bad");
- if atime < min_atime {
- if let Some(cache) = self.cache() {
- // ignore errors, phase 3 will retry cleanup anyways
- let _ = cache.remove(&digest);
- }
- delete_list.push(content.key);
- if bad {
- gc_status.removed_bad += 1;
- } else {
- gc_status.removed_chunks += 1;
- }
- gc_status.removed_bytes += content.size;
- } else if atime < oldest_writer {
- if bad {
- gc_status.still_bad += 1;
- } else {
- gc_status.pending_chunks += 1;
- }
- gc_status.pending_bytes += content.size;
- } else {
- if !bad {
- gc_status.disk_chunks += 1;
- }
- gc_status.disk_bytes += content.size;
- }
+ ChunkStore::check_atime_and_update_gc_status(
+ atime,
+ min_atime,
+ oldest_writer,
+ content.size,
+ bad,
+ &mut gc_status,
+ |_status| {
+ if let Some(cache) = self.cache() {
+ // ignore errors, phase 3 will retry cleanup anyways
+ let _ = cache.remove(&digest);
+ }
+ delete_list.push(content.key);
+ Ok(())
+ },
+ )?;
chunk_count += 1;
}
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 03/12] chunk store: add unsafe signature to cache remove method
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 01/12] datastore: gc: inline single callsite method Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 02/12] gc: chunk store: rework atime check and gc status into common helper Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 04/12] local store cache: replace evicted cache chunks instead of truncate Christian Ebner
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Removing a chunk file from the local datastore cache is rather unsafe
as several preconditions have to be met:
- The chunk store mutex guard has to be held, in order to avoid
concurrent operations on the chunk file
- It must be assured that the chunk to be removed is not referenced
by any visible index file.
- It must be assured that the chunk is not being indexed by an active
index writer (ongoing backup).
- It must be assured that the chunk is not being indexed by an active
index writer in an old process, still active after service reload
(ongoing backup in old process).
Add the unsafe signature to `LocalDatastoreLRUCache::remove()` to
signal these preconditions and limit the scope to be crate only.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/datastore.rs | 2 +-
pbs-datastore/src/local_datastore_lru_cache.rs | 10 +++++++++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 7ef16c31e..acf22e9b0 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1686,7 +1686,7 @@ impl DataStore {
|_status| {
if let Some(cache) = self.cache() {
// ignore errors, phase 3 will retry cleanup anyways
- let _ = cache.remove(&digest);
+ let _ = unsafe { cache.remove(&digest) };
}
delete_list.push(content.key);
Ok(())
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index c0edd3619..12b7f0aaa 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -86,8 +86,16 @@ impl LocalDatastoreLruCache {
/// Remove a chunk from the local datastore cache.
///
+ /// Callers to this method must assure that:
+ /// - no concurrent insert is being performed, the chunk store's mutex must be held.
+ /// - the chunk to be removed is no longer referenced by an index file.
+ /// - the chunk to be removed has not been inserted by an active writer (atime newer than
+ /// writer start time).
+ /// - there is no active writer in an old process, which could have inserted the chunk to be
+ /// deleted.
+ ///
/// Fails if the chunk cannot be deleted successfully.
- pub fn remove(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ pub(crate) unsafe fn remove(&self, digest: &[u8; 32]) -> Result<(), Error> {
self.cache.remove(*digest);
let (path, _digest_str) = self.store.chunk_path(digest);
std::fs::remove_file(path).map_err(Error::from)
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 04/12] local store cache: replace evicted cache chunks instead of truncate
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (2 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 03/12] chunk store: add unsafe signature to cache remove method Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 05/12] local store cache: serve response fetched from s3 backend Christian Ebner
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Evicted chunks have been truncated to size zero, keeping the chunk
file in place as in-use marker for the garbage collection but freeing
the chunk file contents. This can however lead to restores failing if
they already opened the chunk file for reading, as their contents are
now incomplete.
Fix this by instead replacing the chunk file with a zero sized file,
leaving the contents accessible for the already opened chunk readers.
By moving the logic from the local datastore cache to a helper method
on the chunk store, it is also assured that the operation is guarded
by the chunk store mutex lock to avoid races with chunk re-insert.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 20 +++++++++++++++
.../src/local_datastore_lru_cache.rs | 25 +++----------------
2 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index dc247886a..6e50327cb 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -658,6 +658,26 @@ impl ChunkStore {
(chunk_path, digest_str)
}
+ /// Replace a chunk file with a zero size file in the chunk store.
+ ///
+ /// Used to evict chunks from the local datastore cache, while keeping them as in-use markers
+ /// for garbage collection. Returns with success also if chunk file is not pre-existing.
+ pub fn clear_chunk(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ let (chunk_path, digest_str) = self.chunk_path(digest);
+ let mut create_options = CreateOptions::new();
+ if nix::unistd::Uid::effective().is_root() {
+ let uid = pbs_config::backup_user()?.uid;
+ let gid = pbs_config::backup_group()?.gid;
+ create_options = create_options.owner(uid).group(gid);
+ }
+
+ let _lock = self.mutex.lock();
+
+ proxmox_sys::fs::replace_file(&chunk_path, &[], create_options, false)
+ .map_err(|err| format_err!("clear chunk failed for {digest_str} - {err}"))?;
+ Ok(())
+ }
+
pub fn relative_path(&self, path: &Path) -> PathBuf {
// unwrap: only `None` in unit tests
assert!(self.locker.is_some());
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index 12b7f0aaa..fe0b336ff 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -71,17 +71,8 @@ impl LocalDatastoreLruCache {
/// Fails if the chunk cannot be inserted successfully.
pub fn insert(&self, digest: &[u8; 32], chunk: &DataBlob) -> Result<(), Error> {
self.store.insert_chunk(chunk, digest)?;
- self.cache.insert(*digest, (), |digest| {
- let (path, _digest_str) = self.store.chunk_path(&digest);
- // Truncate to free up space but keep the inode around, since that
- // is used as marker for chunks in use by garbage collection.
- if let Err(err) = nix::unistd::truncate(&path, 0) {
- if err != nix::errno::Errno::ENOENT {
- return Err(Error::from(err));
- }
- }
- Ok(())
- })
+ self.cache
+ .insert(*digest, (), |digest| self.store.clear_chunk(&digest))
}
/// Remove a chunk from the local datastore cache.
@@ -113,17 +104,7 @@ impl LocalDatastoreLruCache {
) -> Result<Option<DataBlob>, Error> {
if self
.cache
- .access(*digest, cacher, |digest| {
- let (path, _digest_str) = self.store.chunk_path(&digest);
- // Truncate to free up space but keep the inode around, since that
- // is used as marker for chunks in use by garbage collection.
- if let Err(err) = nix::unistd::truncate(&path, 0) {
- if err != nix::errno::Errno::ENOENT {
- return Err(Error::from(err));
- }
- }
- Ok(())
- })
+ .access(*digest, cacher, |digest| self.store.clear_chunk(&digest))
.await?
.is_some()
{
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 05/12] local store cache: serve response fetched from s3 backend
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (3 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 04/12] local store cache: replace evicted cache chunks instead of truncate Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 06/12] local store cache: refactor fetch and insert of chunks for " Christian Ebner
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
If the chunk file was not found in the local datastore cache or the
chunk file was the zero marker, it is fetched from the s3 backend and
re-inserted.
Currently, the chunk is then re-read from the file, but that is not
required. Serve the still in-memory chunk data as result of the cacher
access call.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/local_datastore_lru_cache.rs | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index fe0b336ff..7d0b3e114 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -124,7 +124,7 @@ impl LocalDatastoreLruCache {
let bytes = response.content.collect().await?.to_bytes();
let chunk = DataBlob::from_raw(bytes.to_vec())?;
self.store.insert_chunk(&chunk, digest)?;
- std::fs::File::open(&path)?
+ return Ok(Some(chunk));
}
}
} else {
@@ -147,8 +147,7 @@ impl LocalDatastoreLruCache {
let bytes = response.content.collect().await?.to_bytes();
let chunk = DataBlob::from_raw(bytes.to_vec())?;
self.store.insert_chunk(&chunk, digest)?;
- let mut file = std::fs::File::open(&path)?;
- DataBlob::load_from_reader(&mut file)?
+ return Ok(Some(chunk));
}
}
} else {
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 06/12] local store cache: refactor fetch and insert of chunks for s3 backend
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (4 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 05/12] local store cache: serve response fetched from s3 backend Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 07/12] local store cache: rework access cache fetching and insert logic Christian Ebner
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Move the common logic for both, the fetching when no local chunk
marker file is found and when the marker file has no content into a
common helper. This is in preparation for further restructuring in
the following patches/commits.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
.../src/local_datastore_lru_cache.rs | 47 +++++++++----------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index 7d0b3e114..ea92bc9b3 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -115,18 +115,8 @@ impl LocalDatastoreLruCache {
// Expected chunk to be present since LRU cache has it, but it is missing
// locally, try to fetch again
if err.kind() == std::io::ErrorKind::NotFound {
- let object_key = crate::s3::object_key_from_digest(digest)?;
- match cacher.client.get_object(object_key).await? {
- None => {
- bail!("could not fetch object with key {}", hex::encode(digest))
- }
- Some(response) => {
- let bytes = response.content.collect().await?.to_bytes();
- let chunk = DataBlob::from_raw(bytes.to_vec())?;
- self.store.insert_chunk(&chunk, digest)?;
- return Ok(Some(chunk));
- }
- }
+ let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
+ return Ok(Some(chunk));
} else {
return Err(Error::from(err));
}
@@ -138,18 +128,8 @@ impl LocalDatastoreLruCache {
use std::io::Seek;
// Check if file is empty marker file, try fetching content if so
if file.seek(std::io::SeekFrom::End(0))? == 0 {
- let object_key = crate::s3::object_key_from_digest(digest)?;
- match cacher.client.get_object(object_key).await? {
- None => {
- bail!("could not fetch object with key {}", hex::encode(digest))
- }
- Some(response) => {
- let bytes = response.content.collect().await?.to_bytes();
- let chunk = DataBlob::from_raw(bytes.to_vec())?;
- self.store.insert_chunk(&chunk, digest)?;
- return Ok(Some(chunk));
- }
- }
+ let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
+ return Ok(Some(chunk));
} else {
return Err(err);
}
@@ -165,4 +145,23 @@ impl LocalDatastoreLruCache {
pub fn contains(&self, digest: &[u8; 32]) -> bool {
self.cache.contains(*digest)
}
+
+ async fn fetch_and_insert(
+ &self,
+ client: Arc<S3Client>,
+ digest: &[u8; 32],
+ ) -> Result<DataBlob, Error> {
+ let object_key = crate::s3::object_key_from_digest(digest)?;
+ match client.get_object(object_key).await? {
+ None => {
+ bail!("could not fetch object with key {}", hex::encode(digest))
+ }
+ Some(response) => {
+ let bytes = response.content.collect().await?.to_bytes();
+ let chunk = DataBlob::from_raw(bytes.to_vec())?;
+ self.store.insert_chunk(&chunk, digest)?;
+ Ok(chunk)
+ }
+ }
+ }
}
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 07/12] local store cache: rework access cache fetching and insert logic
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (5 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 06/12] local store cache: refactor fetch and insert of chunks for " Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 08/12] local store cache: drop obsolete cacher implementation Christian Ebner
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
The local datastore cache has both, an in-memory LRU cache only
storing the digests and the chunk marks on the filesystem.
Chunks in the LRU cache have recently been accessed, therefore the
chunk contents are expected to be present in the local chunk file,
while no payload is present for evicted ones.
The current implementation relied on the cacher to fetch the chunk
data on cache misses, but required to re-read the chunk file after
the download, as the cacher interface does not allow to return a
payload value other than the one defined for the LRU cache, which is
however none.
Therefore, instead of using the LRU cache access method and in turn
the S3Cacher, rather try to access the local filesystem chunks
directly. They need to be accessed anyways, and further this avoids
possible races with download and insert, as now the held filehandle
either has a chunk with valid content and can bypass the backend,
or the chunk must be downloaded, serving the chunk from the fetched
data instead after inserting into the cache.
By unconditional re-insertion, it is assured that the chunk will be
marked as recently used in all cases and the least recently used one
is evicted.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
.../src/local_datastore_lru_cache.rs | 50 ++++++++-----------
1 file changed, 22 insertions(+), 28 deletions(-)
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index ea92bc9b3..f03265a5b 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -102,42 +102,36 @@ impl LocalDatastoreLruCache {
digest: &[u8; 32],
cacher: &mut S3Cacher,
) -> Result<Option<DataBlob>, Error> {
- if self
- .cache
- .access(*digest, cacher, |digest| self.store.clear_chunk(&digest))
- .await?
- .is_some()
- {
- let (path, _digest_str) = self.store.chunk_path(digest);
- let mut file = match std::fs::File::open(&path) {
- Ok(file) => file,
- Err(err) => {
- // Expected chunk to be present since LRU cache has it, but it is missing
- // locally, try to fetch again
- if err.kind() == std::io::ErrorKind::NotFound {
- let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
- return Ok(Some(chunk));
- } else {
- return Err(Error::from(err));
- }
+ let (path, _digest_str) = self.store.chunk_path(digest);
+ match std::fs::File::open(&path) {
+ Ok(mut file) => match DataBlob::load_from_reader(&mut file) {
+ // File was still cached with contents, load response from file
+ Ok(chunk) => {
+ self.cache
+ .insert(*digest, (), |digest| self.store.clear_chunk(&digest))?;
+ Ok(Some(chunk))
}
- };
- let chunk = match DataBlob::load_from_reader(&mut file) {
- Ok(chunk) => chunk,
+ // File was empty, might have been evicted since
Err(err) => {
use std::io::Seek;
// Check if file is empty marker file, try fetching content if so
if file.seek(std::io::SeekFrom::End(0))? == 0 {
let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
- return Ok(Some(chunk));
+ Ok(Some(chunk))
} else {
- return Err(err);
+ Err(err)
}
}
- };
- Ok(Some(chunk))
- } else {
- Ok(None)
+ },
+ Err(err) => {
+ // Failed to open file, missing
+ if err.kind() == std::io::ErrorKind::NotFound {
+ let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
+ Ok(Some(chunk))
+ } else {
+ Err(Error::from(err))
+ }
+ }
}
}
@@ -159,7 +153,7 @@ impl LocalDatastoreLruCache {
Some(response) => {
let bytes = response.content.collect().await?.to_bytes();
let chunk = DataBlob::from_raw(bytes.to_vec())?;
- self.store.insert_chunk(&chunk, digest)?;
+ self.insert(digest, &chunk)?;
Ok(chunk)
}
}
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 08/12] local store cache: drop obsolete cacher implementation
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (6 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 07/12] local store cache: rework access cache fetching and insert logic Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 09/12] chunk store: refactor method for chunk insertion Christian Ebner
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Since the local store cache uses no longer the inner lru cache on
chunk access, the S3Cacher implementation is obsolete and can be
replaced by the S3Client directly when fetching is required.
Drop all now obsolete code and adapt callsites accordingly.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/datastore.rs | 12 -----
pbs-datastore/src/local_chunk_reader.rs | 27 +++++-------
.../src/local_datastore_lru_cache.rs | 44 ++-----------------
src/api2/reader/mod.rs | 34 ++++++--------
4 files changed, 29 insertions(+), 88 deletions(-)
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index acf22e9b0..a6b17e3c3 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -41,7 +41,6 @@ use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
use crate::hierarchy::{ListGroups, ListGroupsType, ListNamespaces, ListNamespacesRecursive};
use crate::index::IndexFile;
-use crate::local_datastore_lru_cache::S3Cacher;
use crate::s3::S3_CONTENT_PREFIX;
use crate::task_tracking::{self, update_active_operations};
use crate::{DataBlob, LocalDatastoreLruCache};
@@ -291,17 +290,6 @@ impl DataStore {
Ok(())
}
- /// Returns the cacher for datastores backed by S3 object stores.
- /// This allows to fetch chunks to the local cache store on-demand.
- pub fn cacher(&self) -> Result<Option<S3Cacher>, Error> {
- self.backend().map(|backend| match backend {
- DatastoreBackend::S3(s3_client) => {
- Some(S3Cacher::new(s3_client, self.inner.chunk_store.clone()))
- }
- DatastoreBackend::Filesystem => None,
- })
- }
-
pub fn lookup_datastore(
name: &str,
operation: Option<Operation>,
diff --git a/pbs-datastore/src/local_chunk_reader.rs b/pbs-datastore/src/local_chunk_reader.rs
index 36bce1552..c50a63fb7 100644
--- a/pbs-datastore/src/local_chunk_reader.rs
+++ b/pbs-datastore/src/local_chunk_reader.rs
@@ -70,13 +70,11 @@ impl ReadChunk for LocalChunkReader {
DatastoreBackend::S3(s3_client) => match self.store.cache() {
None => proxmox_async::runtime::block_on(fetch(Arc::clone(s3_client), digest))?,
Some(cache) => {
- let mut cacher = self
- .store
- .cacher()?
- .ok_or(format_err!("no cacher for datastore"))?;
- proxmox_async::runtime::block_on(cache.access(digest, &mut cacher))?.ok_or(
- format_err!("unable to access chunk with digest {}", hex::encode(digest)),
- )?
+ proxmox_async::runtime::block_on(cache.access(digest, s3_client.clone()))?
+ .ok_or(format_err!(
+ "unable to access chunk with digest {}",
+ hex::encode(digest)
+ ))?
}
},
};
@@ -109,14 +107,13 @@ impl AsyncReadChunk for LocalChunkReader {
DatastoreBackend::S3(s3_client) => match self.store.cache() {
None => fetch(Arc::clone(s3_client), digest).await?,
Some(cache) => {
- let mut cacher = self
- .store
- .cacher()?
- .ok_or(format_err!("no cacher for datastore"))?;
- cache.access(digest, &mut cacher).await?.ok_or(format_err!(
- "unable to access chunk with digest {}",
- hex::encode(digest)
- ))?
+ cache
+ .access(digest, s3_client.clone())
+ .await?
+ .ok_or(format_err!(
+ "unable to access chunk with digest {}",
+ hex::encode(digest)
+ ))?
}
},
};
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index f03265a5b..fe3b51a55 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -1,53 +1,17 @@
//! Use a local datastore as cache for operations on a datastore attached via
//! a network layer (e.g. via the S3 backend).
-use std::future::Future;
use std::sync::Arc;
use anyhow::{bail, Error};
use http_body_util::BodyExt;
-use pbs_tools::async_lru_cache::{AsyncCacher, AsyncLruCache};
+use pbs_tools::async_lru_cache::AsyncLruCache;
use proxmox_s3_client::S3Client;
use crate::ChunkStore;
use crate::DataBlob;
-#[derive(Clone)]
-/// Cacher to fetch chunks from the S3 object store and insert them in the local cache store.
-pub struct S3Cacher {
- client: Arc<S3Client>,
- store: Arc<ChunkStore>,
-}
-
-impl AsyncCacher<[u8; 32], ()> for S3Cacher {
- fn fetch(
- &self,
- key: [u8; 32],
- ) -> Box<dyn Future<Output = Result<Option<()>, Error>> + Send + 'static> {
- let client = Arc::clone(&self.client);
- let store = Arc::clone(&self.store);
- Box::new(async move {
- let object_key = crate::s3::object_key_from_digest(&key)?;
- match client.get_object(object_key).await? {
- None => bail!("could not fetch object with key {}", hex::encode(key)),
- Some(response) => {
- let bytes = response.content.collect().await?.to_bytes();
- let chunk = DataBlob::from_raw(bytes.to_vec())?;
- store.insert_chunk(&chunk, &key)?;
- Ok(Some(()))
- }
- }
- })
- }
-}
-
-impl S3Cacher {
- pub fn new(client: Arc<S3Client>, store: Arc<ChunkStore>) -> Self {
- Self { client, store }
- }
-}
-
/// LRU cache using local datastore for caching chunks
///
/// Uses a LRU cache, but without storing the values in-memory but rather
@@ -100,7 +64,7 @@ impl LocalDatastoreLruCache {
pub async fn access(
&self,
digest: &[u8; 32],
- cacher: &mut S3Cacher,
+ client: Arc<S3Client>,
) -> Result<Option<DataBlob>, Error> {
let (path, _digest_str) = self.store.chunk_path(digest);
match std::fs::File::open(&path) {
@@ -116,7 +80,7 @@ impl LocalDatastoreLruCache {
use std::io::Seek;
// Check if file is empty marker file, try fetching content if so
if file.seek(std::io::SeekFrom::End(0))? == 0 {
- let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
+ let chunk = self.fetch_and_insert(client.clone(), digest).await?;
Ok(Some(chunk))
} else {
Err(err)
@@ -126,7 +90,7 @@ impl LocalDatastoreLruCache {
Err(err) => {
// Failed to open file, missing
if err.kind() == std::io::ErrorKind::NotFound {
- let chunk = self.fetch_and_insert(cacher.client.clone(), digest).await?;
+ let chunk = self.fetch_and_insert(client.clone(), digest).await?;
Ok(Some(chunk))
} else {
Err(Error::from(err))
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index 846493c61..155e862c6 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -327,28 +327,20 @@ fn download_chunk(
let body = match &env.backend {
DatastoreBackend::Filesystem => load_from_filesystem(env, &digest)?,
- DatastoreBackend::S3(s3_client) => {
- match env.datastore.cache() {
- None => fetch_from_object_store(s3_client, &digest).await?,
- Some(cache) => {
- let mut cacher = env
- .datastore
- .cacher()?
- .ok_or(format_err!("no cacher for datastore"))?;
- // Download from object store, insert to local cache store and read from
- // file. Can this be optimized?
- let chunk =
- cache
- .access(&digest, &mut cacher)
- .await?
- .ok_or(format_err!(
- "unable to access chunk with digest {}",
- hex::encode(digest)
- ))?;
- Body::from(chunk.raw_data().to_owned())
- }
+ DatastoreBackend::S3(s3_client) => match env.datastore.cache() {
+ None => fetch_from_object_store(s3_client, &digest).await?,
+ Some(cache) => {
+ let chunk =
+ cache
+ .access(&digest, s3_client.clone())
+ .await?
+ .ok_or(format_err!(
+ "unable to access chunk with digest {}",
+ hex::encode(digest)
+ ))?;
+ Body::from(chunk.raw_data().to_owned())
}
- }
+ },
};
// fixme: set other headers ?
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 09/12] chunk store: refactor method for chunk insertion
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (7 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 08/12] local store cache: drop obsolete cacher implementation Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 10/12] api: chunk upload: fix race between chunk backend upload and insert Christian Ebner
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Extend the current implementation of the chunk insertion by a method
to provide a callback to be executed before writing the chunk file,
but after checking if the file is pre-existing.
This is in preparation for different methods for chunk insertion,
with differentiated handling of the non pre-existing chunk file case.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 6e50327cb..65d74d68e 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -558,6 +558,15 @@ impl ChunkStore {
}
pub fn insert_chunk(&self, chunk: &DataBlob, digest: &[u8; 32]) -> Result<(bool, u64), Error> {
+ self.insert_chunk_impl(chunk, digest, |_, _| Ok(()))
+ }
+
+ fn insert_chunk_impl(
+ &self,
+ chunk: &DataBlob,
+ digest: &[u8; 32],
+ before_create: impl FnOnce(&[u8; 32], bool) -> Result<(), Error>,
+ ) -> Result<(bool, u64), Error> {
// unwrap: only `None` in unit tests
assert!(self.locker.is_some());
@@ -572,7 +581,7 @@ impl ChunkStore {
let name = &self.name;
- if let Ok(metadata) = std::fs::metadata(&chunk_path) {
+ let pre_existing = if let Ok(metadata) = std::fs::metadata(&chunk_path) {
if !metadata.is_file() {
bail!("got unexpected file type on store '{name}' for chunk {digest_str}");
}
@@ -612,7 +621,12 @@ impl ChunkStore {
} else {
log::debug!("Got another copy of chunk with digest '{digest_str}', existing chunk is bigger, replacing with uploaded one.");
}
- }
+ true
+ } else {
+ false
+ };
+
+ before_create(digest, pre_existing)?;
let chunk_dir_path = chunk_path
.parent()
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 10/12] api: chunk upload: fix race between chunk backend upload and insert
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (8 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 09/12] chunk store: refactor method for chunk insertion Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 11/12] api: chunk upload: fix race with garbage collection for no-cache on s3 Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 12/12] pull: guard chunk upload and only insert into cache after upload Christian Ebner
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Chunk are first uploaded to the object store for S3 backed
datastores, only then inserted into the local datastore cache.
By this, it is assured that the chunk is only ever considered as
valid after successful upload. Garbage collection does however rely
on the local marker file to be present, only then considering the chunk
as in-use. While this marker is created if not present during phase 1
of garbage collection, this only happens for chunks which are already
referenced by a complete index file.
Therefore, there remains a race window between garbage collection
listing the chunks (upload completed) and lookup of the local marker
file being present (chunk cache insert after upload). This can lead to
chunks which just finished upload, but were not yet inserted into the
local cache store to be removed again.
To close this race window, mark chunks which are currently being
uploaded to the backend by an additional marker file, checked by the
garbage collection as well if the regular marker is not found, and
only removed after successful chunk insert.
Concurrent chunk upload is still possible, only creating the upload
marker file if not already present. There is still a race between the
concurrent uploads, however since they encode for the same data
(except compression level) both are valid, even when the winner of
the backup upload is not the winner of the chunk insert. However, if
one of the concurrent uploads fails, the other one must fail chunk
insertion and the backup job fail as well, as then there is no
guarantee that garbage collection could not have cleaned up the chunk
in the mean time, even if such a constellation is very unlikely.
Avoid this overhead for regular datastores by passing and checking
the backend type for the chunk store.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 136 +++++++++++++++++-
pbs-datastore/src/datastore.rs | 32 +++++
.../src/local_datastore_lru_cache.rs | 3 +-
src/api2/backup/upload_chunk.rs | 19 ++-
src/api2/config/datastore.rs | 2 +
5 files changed, 179 insertions(+), 13 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 65d74d68e..5b1f397bd 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -7,7 +7,7 @@ use std::time::Duration;
use anyhow::{bail, format_err, Context, Error};
use tracing::{info, warn};
-use pbs_api_types::{DatastoreFSyncLevel, GarbageCollectionStatus};
+use pbs_api_types::{DatastoreBackendType, DatastoreFSyncLevel, GarbageCollectionStatus};
use proxmox_io::ReadExt;
use proxmox_s3_client::S3Client;
use proxmox_sys::fs::{create_dir, create_path, file_type_from_file_stat, CreateOptions};
@@ -22,6 +22,10 @@ use crate::file_formats::{
};
use crate::DataBlob;
+/// Filename extension for local datastore cache marker files indicating
+/// the chunk being uploaded to the backend
+const BACKEND_UPLOAD_MARKER_EXT: &str = "backend-upload";
+
/// File system based chunk store
pub struct ChunkStore {
name: String, // used for error reporting
@@ -30,6 +34,7 @@ pub struct ChunkStore {
mutex: Mutex<()>,
locker: Option<Arc<Mutex<ProcessLocker>>>,
sync_level: DatastoreFSyncLevel,
+ datastore_backend_type: DatastoreBackendType,
}
// TODO: what about sysctl setting vm.vfs_cache_pressure (0 - 100) ?
@@ -77,6 +82,7 @@ impl ChunkStore {
mutex: Mutex::new(()),
locker: None,
sync_level: Default::default(),
+ datastore_backend_type: DatastoreBackendType::default(),
}
}
@@ -97,6 +103,7 @@ impl ChunkStore {
uid: nix::unistd::Uid,
gid: nix::unistd::Gid,
sync_level: DatastoreFSyncLevel,
+ datastore_backend_type: DatastoreBackendType,
) -> Result<Self, Error>
where
P: Into<PathBuf>,
@@ -151,7 +158,7 @@ impl ChunkStore {
}
}
- Self::open(name, base, sync_level)
+ Self::open(name, base, sync_level, datastore_backend_type)
}
fn lockfile_path<P: Into<PathBuf>>(base: P) -> PathBuf {
@@ -185,6 +192,7 @@ impl ChunkStore {
name: &str,
base: P,
sync_level: DatastoreFSyncLevel,
+ datastore_backend_type: DatastoreBackendType,
) -> Result<Self, Error> {
let base: PathBuf = base.into();
@@ -201,6 +209,7 @@ impl ChunkStore {
locker: Some(locker),
mutex: Mutex::new(()),
sync_level,
+ datastore_backend_type,
})
}
@@ -557,10 +566,96 @@ impl ChunkStore {
Ok(())
}
+ // Try to insert a new backend upload marker, signaling to garbage collection that there is an
+ // in-progress upload for this chunk.
+ //
+ // Returns true if the marker was created or pre-existed (concurrent upload), returns false
+ // if the chunk has been inserted since, the marker file not being created and the upload
+ // must be avoided.
+ pub(crate) fn insert_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<bool, Error> {
+ if self.datastore_backend_type == DatastoreBackendType::Filesystem {
+ bail!("cannot create backend upload marker, not a cache store");
+ }
+ let (marker_path, digest_str) = self.chunk_backed_upload_marker_path(digest);
+
+ let _lock = self.mutex.lock();
+
+ if self.cond_touch_chunk(digest, false)? {
+ return Ok(false);
+ }
+
+ if let Err(err) = std::fs::File::options()
+ .write(true)
+ .create_new(true)
+ .open(&marker_path)
+ {
+ if err.kind() != std::io::ErrorKind::AlreadyExists {
+ return Err(err).with_context(|| {
+ format!("failed to create backend upload marker for chunk {digest_str}")
+ });
+ }
+ }
+ Ok(true)
+ }
+
+ pub(crate) fn cleanup_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ if self.datastore_backend_type == DatastoreBackendType::Filesystem {
+ bail!("cannot cleanup backend upload marker, not a cache store");
+ }
+ let (marker_path, _digest_str) = self.chunk_backed_upload_marker_path(digest);
+
+ let _lock = self.mutex.lock();
+
+ if let Err(err) = std::fs::remove_file(marker_path) {
+ if err.kind() != std::io::ErrorKind::NotFound {
+ bail!("failed to cleanup backend upload marker: {err}");
+ }
+ }
+ Ok(())
+ }
+
+ pub(crate) fn backend_upload_marker_exists(&self, digest: &[u8; 32]) -> Result<bool, Error> {
+ let (marker_path, _digest_str) = self.chunk_backed_upload_marker_path(digest);
+ if let Err(err) = std::fs::metadata(marker_path) {
+ if err.kind() != std::io::ErrorKind::NotFound {
+ bail!("failed to check backend upload marker: {err}");
+ }
+ return Ok(false);
+ }
+ Ok(true)
+ }
+
pub fn insert_chunk(&self, chunk: &DataBlob, digest: &[u8; 32]) -> Result<(bool, u64), Error> {
self.insert_chunk_impl(chunk, digest, |_, _| Ok(()))
}
+ pub fn insert_chunk_and_remove_upload_marker(
+ &self,
+ chunk: &DataBlob,
+ digest: &[u8; 32],
+ ) -> Result<(bool, u64), Error> {
+ self.insert_chunk_impl(chunk, digest, |digest, pre_existing| {
+ if self.datastore_backend_type != DatastoreBackendType::Filesystem {
+ // Must fail as if the marker is no longer present but the chunk file is not
+ // present, as this indicates that a concurrent upload failed, no guarantee that
+ // garbage collection did not cleanup chunks in the mean time.
+ let (chunk_path, digest_str) = self.chunk_path(digest);
+ if let Err(err) =
+ std::fs::remove_file(chunk_path.with_extension(BACKEND_UPLOAD_MARKER_EXT))
+ {
+ if !(pre_existing && err.kind() == std::io::ErrorKind::NotFound) {
+ bail!(
+ "concurrent upload failed on store '{}' for {digest_str} - {err}",
+ self.name,
+ )
+ }
+ }
+ }
+
+ Ok(())
+ })
+ }
+
fn insert_chunk_impl(
&self,
chunk: &DataBlob,
@@ -692,6 +787,15 @@ impl ChunkStore {
Ok(())
}
+ /// Generate file path for backend upload marker of given chunk digest.
+ fn chunk_backed_upload_marker_path(&self, digest: &[u8; 32]) -> (PathBuf, String) {
+ let (chunk_path, digest_str) = self.chunk_path(digest);
+ (
+ chunk_path.with_extension(BACKEND_UPLOAD_MARKER_EXT),
+ digest_str,
+ )
+ }
+
pub fn relative_path(&self, path: &Path) -> PathBuf {
// unwrap: only `None` in unit tests
assert!(self.locker.is_some());
@@ -777,14 +881,26 @@ fn test_chunk_store1() {
if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
- let chunk_store = ChunkStore::open("test", &path, DatastoreFSyncLevel::None);
+ let chunk_store = ChunkStore::open(
+ "test",
+ &path,
+ DatastoreFSyncLevel::None,
+ DatastoreBackendType::Filesystem,
+ );
assert!(chunk_store.is_err());
let user = nix::unistd::User::from_uid(nix::unistd::Uid::current())
.unwrap()
.unwrap();
- let chunk_store =
- ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None).unwrap();
+ let chunk_store = ChunkStore::create(
+ "test",
+ &path,
+ user.uid,
+ user.gid,
+ DatastoreFSyncLevel::None,
+ DatastoreBackendType::Filesystem,
+ )
+ .unwrap();
let (chunk, digest) = crate::data_blob::DataChunkBuilder::new(&[0u8, 1u8])
.build()
@@ -796,8 +912,14 @@ fn test_chunk_store1() {
let (exists, _) = chunk_store.insert_chunk(&chunk, &digest).unwrap();
assert!(exists);
- let chunk_store =
- ChunkStore::create("test", &path, user.uid, user.gid, DatastoreFSyncLevel::None);
+ let chunk_store = ChunkStore::create(
+ "test",
+ &path,
+ user.uid,
+ user.gid,
+ DatastoreFSyncLevel::None,
+ DatastoreBackendType::Filesystem,
+ );
assert!(chunk_store.is_err());
if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index a6b17e3c3..e40b6883b 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -336,10 +336,14 @@ impl DataStore {
DatastoreTuning::API_SCHEMA
.parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
)?;
+ let backend_config: DatastoreBackendConfig =
+ config.backend.as_deref().unwrap_or("").parse()?;
+ let backend_type = backend_config.ty.unwrap_or_default();
Arc::new(ChunkStore::open(
name,
config.absolute_path(),
tuning.sync_level.unwrap_or_default(),
+ backend_type,
)?)
};
@@ -424,10 +428,16 @@ impl DataStore {
DatastoreTuning::API_SCHEMA
.parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
)?;
+ let backend_config: DatastoreBackendConfig = serde_json::from_value(
+ DatastoreBackendConfig::API_SCHEMA
+ .parse_property_string(config.backend.as_deref().unwrap_or(""))?,
+ )?;
+ let backend_type = backend_config.ty.unwrap_or_default();
let chunk_store = ChunkStore::open(
&name,
config.absolute_path(),
tuning.sync_level.unwrap_or_default(),
+ backend_type,
)?;
let inner = Arc::new(Self::with_store_and_config(
Arc::new(chunk_store),
@@ -1651,6 +1661,13 @@ impl DataStore {
let atime = match std::fs::metadata(&chunk_path) {
Ok(stat) => stat.accessed()?,
Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+ if self
+ .inner
+ .chunk_store
+ .backend_upload_marker_exists(&digest)?
+ {
+ continue;
+ }
// File not found, delete by setting atime to unix epoch
info!("Not found, mark for deletion: {}", content.key);
SystemTime::UNIX_EPOCH
@@ -1857,6 +1874,21 @@ impl DataStore {
self.inner.chunk_store.insert_chunk(chunk, digest)
}
+ // Try to insert a new backend upload marker, signaling to garbage collection that there is an
+ // in-progress upload for this chunk.
+ //
+ // Returns true if the marker was created or pre-existed (concurrent upload), returns false
+ // if the chunk has been inserted since, the marker file not being created and the upload
+ // must be avoided.
+ pub fn insert_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<bool, Error> {
+ self.inner.chunk_store.insert_backend_upload_marker(digest)
+ }
+
+ /// Remove the marker file signaling an in-progress upload to the backend
+ pub fn cleanup_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ self.inner.chunk_store.cleanup_backend_upload_marker(digest)
+ }
+
pub fn stat_chunk(&self, digest: &[u8; 32]) -> Result<std::fs::Metadata, Error> {
let (chunk_path, _digest_str) = self.inner.chunk_store.chunk_path(digest);
std::fs::metadata(chunk_path).map_err(Error::from)
diff --git a/pbs-datastore/src/local_datastore_lru_cache.rs b/pbs-datastore/src/local_datastore_lru_cache.rs
index fe3b51a55..cdad77031 100644
--- a/pbs-datastore/src/local_datastore_lru_cache.rs
+++ b/pbs-datastore/src/local_datastore_lru_cache.rs
@@ -34,7 +34,8 @@ impl LocalDatastoreLruCache {
///
/// Fails if the chunk cannot be inserted successfully.
pub fn insert(&self, digest: &[u8; 32], chunk: &DataBlob) -> Result<(), Error> {
- self.store.insert_chunk(chunk, digest)?;
+ self.store
+ .insert_chunk_and_remove_upload_marker(chunk, digest)?;
self.cache
.insert(*digest, (), |digest| self.store.clear_chunk(&digest))
}
diff --git a/src/api2/backup/upload_chunk.rs b/src/api2/backup/upload_chunk.rs
index 8dd7e4d52..7d1f863ed 100644
--- a/src/api2/backup/upload_chunk.rs
+++ b/src/api2/backup/upload_chunk.rs
@@ -259,6 +259,7 @@ async fn upload_to_backend(
data.len()
);
}
+ let datastore = env.datastore.clone();
if env.no_cache {
let object_key = pbs_datastore::s3::object_key_from_digest(&digest)?;
@@ -272,11 +273,11 @@ async fn upload_to_backend(
// Avoid re-upload to S3 if the chunk is either present in the LRU cache or the chunk
// file exists on filesystem. The latter means that the chunk has been present in the
// past an was not cleaned up by garbage collection, so contained in the S3 object store.
- if env.datastore.cache_contains(&digest) {
+ if datastore.cache_contains(&digest) {
tracing::info!("Skip upload of cached chunk {}", hex::encode(digest));
return Ok((digest, size, encoded_size, true));
}
- if let Ok(true) = env.datastore.cond_touch_chunk(&digest, false) {
+ if let Ok(true) = datastore.cond_touch_chunk(&digest, false) {
tracing::info!(
"Skip upload of already encountered chunk {}",
hex::encode(digest)
@@ -286,16 +287,24 @@ async fn upload_to_backend(
tracing::info!("Upload of new chunk {}", hex::encode(digest));
let object_key = pbs_datastore::s3::object_key_from_digest(&digest)?;
- let is_duplicate = s3_client
+ if !datastore.insert_backend_upload_marker(&digest)? {
+ return Ok((digest, size, encoded_size, true));
+ }
+ let is_duplicate = match s3_client
.upload_no_replace_with_retry(object_key, data.clone())
.await
- .map_err(|err| format_err!("failed to upload chunk to s3 backend - {err:#}"))?;
+ {
+ Ok(is_duplicate) => is_duplicate,
+ Err(err) => {
+ datastore.cleanup_backend_upload_marker(&digest)?;
+ bail!("failed to upload chunk to s3 backend - {err:#}");
+ }
+ };
// Only insert the chunk into the cache after it has been successufuly uploaded.
// Although less performant than doing this in parallel, it is required for consisency
// since chunks are considered as present on the backend if the file exists in the local
// cache store.
- let datastore = env.datastore.clone();
tracing::info!("Caching of chunk {}", hex::encode(digest));
let _ = tokio::task::spawn_blocking(move || {
let chunk = DataBlob::from_raw(data.to_vec())?;
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 3b03c0466..541bd0a04 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -173,6 +173,7 @@ pub(crate) fn do_create_datastore(
&datastore.name,
&path,
tuning.sync_level.unwrap_or_default(),
+ backend_type,
)
})?
} else {
@@ -204,6 +205,7 @@ pub(crate) fn do_create_datastore(
backup_user.uid,
backup_user.gid,
tuning.sync_level.unwrap_or_default(),
+ backend_type,
)?
};
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 11/12] api: chunk upload: fix race with garbage collection for no-cache on s3
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (9 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 10/12] api: chunk upload: fix race between chunk backend upload and insert Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 12/12] pull: guard chunk upload and only insert into cache after upload Christian Ebner
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Chunks uploaded to the s3 backend are never inserted into the local
datastore cache. The presence of the chunk marker file is however
required for garbage collection to not cleanup the chunks. While the
marker files are created during phase 1 of the garbage collection for
indexed chunks, this is not the case for in progress backups with the
no-cache flag set.
Therefore, mark chunks as in-progress while being uploaded just like
for the regular mode with cache, but replace this with the zero-sized
chunk marker file after upload finished to avoid incorrect garbage
collection cleanup.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-datastore/src/chunk_store.rs | 23 +++++++++++++++++++++++
pbs-datastore/src/datastore.rs | 7 +++++++
src/api2/backup/upload_chunk.rs | 14 ++++++++++++--
3 files changed, 42 insertions(+), 2 deletions(-)
diff --git a/pbs-datastore/src/chunk_store.rs b/pbs-datastore/src/chunk_store.rs
index 5b1f397bd..323ba06e6 100644
--- a/pbs-datastore/src/chunk_store.rs
+++ b/pbs-datastore/src/chunk_store.rs
@@ -598,6 +598,29 @@ impl ChunkStore {
Ok(true)
}
+ pub(crate) fn persist_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ if self.datastore_backend_type == DatastoreBackendType::Filesystem {
+ bail!("cannot create backend upload marker, not a cache store");
+ }
+ let (marker_path, _digest_str) = self.chunk_backed_upload_marker_path(digest);
+ let (chunk_path, digest_str) = self.chunk_path(digest);
+ let _lock = self.mutex.lock();
+
+ if let Err(err) = std::fs::rename(marker_path, chunk_path) {
+ // Check if the chunk has been inserted since. Otherwise it is not safe to continue,
+ // as the concurrent chunk upload has failed and the marker file has been cleaned up,
+ // which leaves a race window open for garbage collection to remove the chunk.
+ if self.cond_touch_chunk(digest, false)? {
+ return Ok(());
+ }
+
+ return Err(format_err!(
+ "persisting backup upload marker failed for {digest_str} - {err}"
+ ));
+ }
+ Ok(())
+ }
+
pub(crate) fn cleanup_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
if self.datastore_backend_type == DatastoreBackendType::Filesystem {
bail!("cannot cleanup backend upload marker, not a cache store");
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e40b6883b..1f6eb9a7a 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1884,6 +1884,13 @@ impl DataStore {
self.inner.chunk_store.insert_backend_upload_marker(digest)
}
+ /// Persist the backend upload marker to be a zero size chunk marker.
+ ///
+ /// Marks the chunk as present in the local store cache without inserting its payload.
+ pub fn persist_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
+ self.inner.chunk_store.persist_backend_upload_marker(digest)
+ }
+
/// Remove the marker file signaling an in-progress upload to the backend
pub fn cleanup_backend_upload_marker(&self, digest: &[u8; 32]) -> Result<(), Error> {
self.inner.chunk_store.cleanup_backend_upload_marker(digest)
diff --git a/src/api2/backup/upload_chunk.rs b/src/api2/backup/upload_chunk.rs
index 7d1f863ed..2f09938b7 100644
--- a/src/api2/backup/upload_chunk.rs
+++ b/src/api2/backup/upload_chunk.rs
@@ -263,10 +263,20 @@ async fn upload_to_backend(
if env.no_cache {
let object_key = pbs_datastore::s3::object_key_from_digest(&digest)?;
- let is_duplicate = s3_client
+ if !datastore.insert_backend_upload_marker(&digest)? {
+ return Ok((digest, size, encoded_size, true));
+ }
+ let is_duplicate = match s3_client
.upload_no_replace_with_retry(object_key, data)
.await
- .map_err(|err| format_err!("failed to upload chunk to s3 backend - {err:#}"))?;
+ {
+ Ok(is_duplicate) => is_duplicate,
+ Err(err) => {
+ datastore.cleanup_backend_upload_marker(&digest)?;
+ bail!("failed to upload chunk to s3 backend - {err:#}");
+ }
+ };
+ env.datastore.persist_backend_upload_marker(&digest)?;
return Ok((digest, size, encoded_size, is_duplicate));
}
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 12/12] pull: guard chunk upload and only insert into cache after upload
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
` (10 preceding siblings ...)
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 11/12] api: chunk upload: fix race with garbage collection for no-cache on s3 Christian Ebner
@ 2025-10-08 15:21 ` Christian Ebner
11 siblings, 0 replies; 13+ messages in thread
From: Christian Ebner @ 2025-10-08 15:21 UTC (permalink / raw)
To: pbs-devel
Inserting the chunk into the local datastore cache leads to it being
considered as present on the backend. The upload might however still
fail, leading to missing chunks.
Therefore, only insert the chunk after the upload completed with
success and guard the upload by the backend upload marker file to
avoid races with garbage collection.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
src/server/pull.rs | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 08e6e6b64..dd03a3dc8 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -176,14 +176,22 @@ async fn pull_index_chunks<I: IndexFile>(
if target2.cache_contains(&digest) {
return Ok(());
}
- target2.cache_insert(&digest, &chunk)?;
let data = chunk.raw_data().to_vec();
let upload_data = hyper::body::Bytes::from(data);
let object_key = pbs_datastore::s3::object_key_from_digest(&digest)?;
- let _is_duplicate = proxmox_async::runtime::block_on(
+ if !target2.insert_backend_upload_marker(&digest)? {
+ return Ok(());
+ }
+ match proxmox_async::runtime::block_on(
s3_client.upload_no_replace_with_retry(object_key, upload_data),
- )
- .context("failed to upload chunk to s3 backend")?;
+ ) {
+ Ok(_is_duplicate) => (),
+ Err(err) => {
+ target2.cleanup_backend_upload_marker(&digest)?;
+ return Err(err.context("failed to upload chunk to s3 backend"));
+ }
+ }
+ target2.cache_insert(&digest, &chunk)?;
}
}
Ok(())
--
2.47.3
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-10-08 15:22 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-10-08 15:21 [pbs-devel] [PATCH proxmox-backup v2 00/12] s3 store: fix issues with chunk s3 backend upload and cache eviction Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 01/12] datastore: gc: inline single callsite method Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 02/12] gc: chunk store: rework atime check and gc status into common helper Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 03/12] chunk store: add unsafe signature to cache remove method Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 04/12] local store cache: replace evicted cache chunks instead of truncate Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 05/12] local store cache: serve response fetched from s3 backend Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 06/12] local store cache: refactor fetch and insert of chunks for " Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 07/12] local store cache: rework access cache fetching and insert logic Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 08/12] local store cache: drop obsolete cacher implementation Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 09/12] chunk store: refactor method for chunk insertion Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 10/12] api: chunk upload: fix race between chunk backend upload and insert Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 11/12] api: chunk upload: fix race with garbage collection for no-cache on s3 Christian Ebner
2025-10-08 15:21 ` [pbs-devel] [PATCH proxmox-backup v2 12/12] pull: guard chunk upload and only insert into cache after upload Christian Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox