From: Christian Ebner <c.ebner@proxmox.com>
To: pbs-devel@lists.proxmox.com
Subject: [pbs-devel] [PATCH proxmox-backup] Revert "fix #5710: api: backup: stat known chunks on backup finish"
Date: Thu, 12 Dec 2024 08:52:04 +0100 [thread overview]
Message-ID: <20241212075204.36931-1-c.ebner@proxmox.com> (raw)
Commit da11d226 ("fix #5710: api: backup: stat known chunks on backup
finish") introduced a seemingly cheap server side check to verify
existence of known chunks in the chunk store by stating. This check
however does not scale for large backup snapshots which might contain
millions of known chunks, as reported in the community forum [0].
Revert the changes for now instead of making this opt-in/opt-out, a
more general approach has to be thought out to mark backup snapshots
which fail verification.
Link to the report in the forum:
[0] https://forum.proxmox.com/threads/158812/
Fixes: da11d226 ("fix #5710: api: backup: stat known chunks on backup finish")
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
src/api2/backup/environment.rs | 54 +++++-----------------------------
src/api2/backup/mod.rs | 22 +-------------
2 files changed, 8 insertions(+), 68 deletions(-)
diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 19624fae3..99d885e2e 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -1,4 +1,4 @@
-use anyhow::{bail, format_err, Context, Error};
+use anyhow::{bail, format_err, Error};
use nix::dir::Dir;
use std::collections::HashMap;
use std::sync::{Arc, Mutex};
@@ -72,14 +72,8 @@ struct FixedWriterState {
incremental: bool,
}
-#[derive(Copy, Clone)]
-struct KnownChunkInfo {
- uploaded: bool,
- length: u32,
-}
-
-// key=digest, value=KnownChunkInfo
-type KnownChunksMap = HashMap<[u8; 32], KnownChunkInfo>;
+// key=digest, value=length
+type KnownChunksMap = HashMap<[u8; 32], u32>;
struct SharedBackupState {
finished: bool,
@@ -165,13 +159,7 @@ impl BackupEnvironment {
state.ensure_unfinished()?;
- state.known_chunks.insert(
- digest,
- KnownChunkInfo {
- uploaded: false,
- length,
- },
- );
+ state.known_chunks.insert(digest, length);
Ok(())
}
@@ -225,13 +213,7 @@ impl BackupEnvironment {
}
// register chunk
- state.known_chunks.insert(
- digest,
- KnownChunkInfo {
- uploaded: true,
- length: size,
- },
- );
+ state.known_chunks.insert(digest, size);
Ok(())
}
@@ -266,13 +248,7 @@ impl BackupEnvironment {
}
// register chunk
- state.known_chunks.insert(
- digest,
- KnownChunkInfo {
- uploaded: true,
- length: size,
- },
- );
+ state.known_chunks.insert(digest, size);
Ok(())
}
@@ -280,23 +256,7 @@ impl BackupEnvironment {
pub fn lookup_chunk(&self, digest: &[u8; 32]) -> Option<u32> {
let state = self.state.lock().unwrap();
- state
- .known_chunks
- .get(digest)
- .map(|known_chunk_info| known_chunk_info.length)
- }
-
- /// stat known chunks from previous backup, so excluding newly uploaded ones
- pub fn stat_prev_known_chunks(&self) -> Result<(), Error> {
- let state = self.state.lock().unwrap();
- for (digest, known_chunk_info) in &state.known_chunks {
- if !known_chunk_info.uploaded {
- self.datastore
- .stat_chunk(digest)
- .with_context(|| format!("stat failed on {}", hex::encode(digest)))?;
- }
- }
- Ok(())
+ state.known_chunks.get(digest).copied()
}
/// Store the writer with an unique ID
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 31334b59c..0373d135b 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -1,6 +1,6 @@
//! Backup protocol (HTTP2 upgrade)
-use anyhow::{bail, format_err, Context, Error};
+use anyhow::{bail, format_err, Error};
use futures::*;
use hex::FromHex;
use hyper::header::{HeaderValue, CONNECTION, UPGRADE};
@@ -788,26 +788,6 @@ fn finish_backup(
) -> Result<Value, Error> {
let env: &BackupEnvironment = rpcenv.as_ref();
- if let Err(err) = env.stat_prev_known_chunks() {
- env.debug(format!("stat registered chunks failed - {err:?}"));
-
- if let Some(last) = env.last_backup.as_ref() {
- // No need to acquire snapshot lock, already locked when starting the backup
- let verify_state = SnapshotVerifyState {
- state: VerifyState::Failed,
- upid: env.worker.upid().clone(), // backup writer UPID
- };
- let verify_state = serde_json::to_value(verify_state)?;
- last.backup_dir
- .update_manifest(|manifest| {
- manifest.unprotected["verify_state"] = verify_state;
- })
- .with_context(|| "manifest update failed")?;
- }
-
- bail!("stat known chunks failed - {err:?}");
- }
-
env.finish_backup()?;
env.log("successfully finished backup");
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
reply other threads:[~2024-12-12 7:52 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241212075204.36931-1-c.ebner@proxmox.com \
--to=c.ebner@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox