[RFC qemu/qemu-server/proxmox-backup-qemu 0/9] close #6169: backup: pbs: per-target dirty bitmaps

[RFC qemu/qemu-server/proxmox-backup-qemu 0/9] close #6169: backup: pbs: per-target dirty bitmaps

[Fiona Ebner]

To decide whether an incremental backup can be done, the checksum of
the latest backup on the target is compared against the checksum from
the previous backup the QEMU client library did.

This was previously tracked per device as the PVE backup code in QEMU
also only used a single bitmap per device. So backing up to target A,
dirtying the disk and backing up to target B would mean that the
checksum for target A is overwritten and a subsequent backup to target
A needs to discard the dirty bitmap.

Allow for tracking multiple targets with different bitmaps, by saving
the checksums per device+target combination, rather than just per
device.

Whether the new behavior or the old behavior is used is controlled by
QEMU, optionally passing along the target ID, and a flag for the
export and import functions for migration. These are machine version
guarded on the QEMU/qemu-server side to avoid a mismatch.


This is an ABI change for the backup library, so a versioned breaks
and a versioned depends in the other direction is needed.


QUESTION: with old qemu-server, target ID is not provided, but the new
machine version might be in use. In this case, the dirty bitmap state
is not migrated, in case of a new -> old -> new migration, which can
lead to stale info! Would it be okay to add a Breaks for old
qemu-server or should I try to resolve it differently somehow?

QUESTION: How to best avoid 'orphaned' bitmaps? Auto-remove for
storages no longer in configuration? Or have some knob/limit for how
many to keep?


The clippy fixes are independent.

proxmox-backup-qemu:

Fiona Ebner (6):
  clippy: restore: fix needless borrows
  clippy: commands: avoid manual implementation of ok()
  cargo: add dependency for serde
  close #6169: track checksums for incremental backups per target
  update current-api.h
  d/control: bump versioned breaks for pve-qemu-kvm

 Cargo.toml      |  1 +
 current-api.h   |  9 +++--
 debian/control  |  2 +-
 src/backup.rs   | 27 ++++++++++----
 src/commands.rs | 97 +++++++++++++++++++++++++++++++++++++++----------
 src/lib.rs      | 25 +++++++++----
 src/restore.rs  | 12 ++----
 7 files changed, 126 insertions(+), 47 deletions(-)


qemu:

Fiona Ebner (2):
  PVE backup: properly track if snapshot access was set up
  PVE backup: track per-target dirty bitmaps

 block/monitor/block-hmp-cmds.c |  1 +
 hw/core/machine.c              |  2 ++
 migration/pbs-state.c          | 42 ++++++++++++++++++++++++++++------
 migration/pbs-state.h          | 32 ++++++++++++++++++++++++++
 proxmox-backup-client.c        | 12 +++++++++-
 proxmox-backup-client.h        |  1 +
 pve-backup.c                   | 29 +++++++++++++++++------
 qapi/block-core.json           |  7 +++++-
 8 files changed, 110 insertions(+), 16 deletions(-)
 create mode 100644 migration/pbs-state.h


qemu-server:

Fiona Ebner (1):
  close #6169: backup: pbs: specify target ID so QEMU keeps track of
    per-target dirty bitmap

 src/PVE/VZDump/QemuServer.pm | 6 ++++++
 1 file changed, 6 insertions(+)


Summary over all repositories:
  16 files changed, 242 insertions(+), 63 deletions(-)

-- 
Generated by git-murpp 0.5.0

[PATCH proxmox-backup-qemu 1/9] clippy: restore: fix needless borrows

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/restore.rs | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/src/restore.rs b/src/restore.rs
index 6cafd78..bbe0016 100644
--- a/src/restore.rs
+++ b/src/restore.rs
@@ -151,9 +151,7 @@ impl RestoreTask {
             None => bail!("no manifest"),
         };
 
-        let index = client
-            .download_fixed_index(&manifest, &archive_name)
-            .await?;
+        let index = client.download_fixed_index(&manifest, archive_name).await?;
 
         let (_, zero_chunk_digest) = DataChunkBuilder::build_zero_chunk(
             self.crypt_config.as_ref().map(Arc::as_ref),
@@ -163,7 +161,7 @@ impl RestoreTask {
 
         let most_used = index.find_most_used_chunks(8);
 
-        let file_info = manifest.lookup_file_info(&archive_name)?;
+        let file_info = manifest.lookup_file_info(archive_name)?;
 
         let chunk_reader = RemoteChunkReader::new(
             Arc::clone(&client),
@@ -274,13 +272,11 @@ impl RestoreTask {
             None => bail!("no manifest"),
         };
 
-        let index = client
-            .download_fixed_index(&manifest, &archive_name)
-            .await?;
+        let index = client.download_fixed_index(&manifest, archive_name).await?;
         let archive_size = index.index_bytes();
         let most_used = index.find_most_used_chunks(8);
 
-        let file_info = manifest.lookup_file_info(&archive_name)?;
+        let file_info = manifest.lookup_file_info(archive_name)?;
 
         let chunk_reader = RemoteChunkReader::new(
             Arc::clone(&client),
-- 
2.47.3

[PATCH proxmox-backup-qemu 2/9] clippy: commands: avoid manual implementation of ok()

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/commands.rs | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/commands.rs b/src/commands.rs
index 565649f..749a0d2 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -195,14 +195,11 @@ pub(crate) async fn register_image(
 
     let index = match manifest {
         Some(manifest) => {
-            match client
+            // not having a previous index is not fatal, so ignore errors, using ok()
+            client
                 .download_previous_fixed_index(&archive_name, &manifest, Arc::clone(&known_chunks))
                 .await
-            {
-                Ok(index) => Some(index),
-                // not having a previous index is not fatal, so ignore errors
-                Err(_) => None,
-            }
+                .ok()
         }
         None => None,
     };
-- 
2.47.3

[RFC proxmox-backup-qemu 3/9] cargo: add dependency for serde

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 Cargo.toml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Cargo.toml b/Cargo.toml
index 2719a80..c6952d5 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -44,6 +44,7 @@ pbs-datastore  = { path = "submodules/proxmox-backup/pbs-datastore" }
 pbs-key-config = { path = "submodules/proxmox-backup/pbs-key-config" }
 pbs-tools      = { path = "submodules/proxmox-backup/pbs-tools" }
 
+serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 tokio = { version = "1.6", features = [ "fs", "io-util", "macros", "net", "rt-multi-thread", "signal", "time" ] }
 tokio-stream = "0.1.1"
-- 
2.47.3

[RFC proxmox-backup-qemu 4/9] close #6169: track checksums for incremental backups per target

[Fiona Ebner]

To decide whether an incremental backup can be done, the checksum of
the latest backup on the target is compared against the checksum from
the previous backup the QEMU client library did.

This was previously tracked per device as the PVE backup code in QEMU
also only used a single bitmap per device. So backing up to target A,
dirtying the disk and backing up to target B would mean that the
checksum for target A is overwritten and a subsequent backup to target
A needs to discard the dirty bitmap.

Allow for tracking multiple targets with different bitmaps, by saving
the checksums per device+target combination, rather than just per
device.

Whether the new behavior or the old behavior is used is controlled by
QEMU, optionally passing along the target ID, and a flag for the
export and import functions for migration. These are machine version
guarded on the QEMU/qemu-server side to avoid a mismatch.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/backup.rs   | 27 ++++++++++-----
 src/commands.rs | 88 +++++++++++++++++++++++++++++++++++++++++--------
 src/lib.rs      | 25 +++++++++-----
 3 files changed, 111 insertions(+), 29 deletions(-)

diff --git a/src/backup.rs b/src/backup.rs
index cf1806a..8b3a736 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -234,7 +234,12 @@ impl BackupTask {
         self.last_manifest.get().map(Arc::clone)
     }
 
-    pub fn check_incremental(&self, device_name: String, size: u64) -> bool {
+    pub fn check_incremental(
+        &self,
+        device_name: String,
+        size: u64,
+        target_id: Option<String>,
+    ) -> bool {
         match self.last_manifest() {
             Some(ref manifest) => {
                 let archive_name = if let Ok(archive) = archive_name_from_device_name(&device_name)
@@ -244,13 +249,17 @@ impl BackupTask {
                     return false;
                 };
 
-                check_last_incremental_csum(Arc::clone(manifest), &archive_name, &device_name, size)
-                    && check_last_encryption_mode(
-                        Arc::clone(manifest),
-                        &archive_name,
-                        self.crypt_mode,
-                    )
-                    && check_last_encryption_key(self.crypt_config.clone())
+                check_last_incremental_csum(
+                    Arc::clone(manifest),
+                    &archive_name,
+                    &device_name,
+                    size,
+                    target_id.as_deref(),
+                ) && check_last_encryption_mode(
+                    Arc::clone(manifest),
+                    &archive_name,
+                    self.crypt_mode,
+                ) && check_last_encryption_key(self.crypt_config.clone())
             }
             None => false,
         }
@@ -260,6 +269,7 @@ impl BackupTask {
         &self,
         device_name: String,
         size: u64,
+        target_id: Option<String>,
         incremental: bool,
     ) -> Result<c_int, Error> {
         self.check_aborted()?;
@@ -273,6 +283,7 @@ impl BackupTask {
             Arc::clone(&self.known_chunks),
             device_name,
             size,
+            target_id,
             self.setup.chunk_size,
             incremental,
         );
diff --git a/src/commands.rs b/src/commands.rs
index 749a0d2..050c1e3 100644
--- a/src/commands.rs
+++ b/src/commands.rs
@@ -4,6 +4,7 @@ use std::os::raw::c_int;
 use std::sync::{Arc, Mutex};
 
 use futures::future::{Future, TryFutureExt};
+use serde::{Deserialize, Serialize};
 use serde_json::json;
 
 use pbs_api_types::{BackupArchiveName, CryptMode, ENCRYPTED_KEY_BLOB_NAME, MANIFEST_BLOB_NAME};
@@ -21,6 +22,12 @@ use crate::upload_queue::{
 
 use lazy_static::lazy_static;
 
+#[derive(PartialEq, Eq, Hash, Serialize, Deserialize)]
+struct DeviceOnTarget {
+    device_name: String,
+    target_id: String,
+}
+
 lazy_static! {
     // Note: Any state stored here that needs to be sent along with migration
     // needs to be specified in (de)serialize_state as well!
@@ -29,6 +36,10 @@ lazy_static! {
         Mutex::new(HashMap::new())
     };
 
+    static ref PREVIOUS_CSUMS_PER_TARGET: Mutex<HashMap<DeviceOnTarget, [u8;32]>> = {
+        Mutex::new(HashMap::new())
+    };
+
     static ref PREVIOUS_KEY_FINGERPRINT: Mutex<Option<[u8;32]>> = {
         Mutex::new(None)
     };
@@ -39,21 +50,66 @@ pub struct ImageUploadInfo {
     device_name: String,
     zero_chunk_digest: [u8; 32],
     device_size: u64,
+    target_id: Option<String>,
     upload_queue: Option<UploadQueueSender>,
     upload_result: Option<UploadResultReceiver>,
 }
 
-pub(crate) fn serialize_state() -> Vec<u8> {
-    let prev_csums = &*PREVIOUS_CSUMS.lock().unwrap();
-    let prev_key_fingerprint = &*PREVIOUS_KEY_FINGERPRINT.lock().unwrap();
-    bincode::serialize(&(prev_csums, prev_key_fingerprint)).unwrap()
+fn get_last_incremental_csum(device_name: &str, target_id: Option<&str>) -> Option<[u8; 32]> {
+    if let Some(target_id) = target_id {
+        let device_on_target = DeviceOnTarget {
+            device_name: device_name.to_string(),
+            target_id: target_id.to_string(),
+        };
+        PREVIOUS_CSUMS_PER_TARGET
+            .lock()
+            .unwrap()
+            .get(&device_on_target)
+            .copied()
+    } else {
+        PREVIOUS_CSUMS.lock().unwrap().get(device_name).copied()
+    }
 }
 
-pub(crate) fn deserialize_state(data: &[u8]) -> Result<(), Error> {
-    let (prev_csums, prev_key_fingerprint) = bincode::deserialize(data)?;
-    let mut prev_csums_guard = PREVIOUS_CSUMS.lock().unwrap();
+fn set_last_incremental_csum(device_name: String, target_id: Option<String>, csum: [u8; 32]) {
+    if let Some(target_id) = target_id {
+        let mut prev_csum_guard = PREVIOUS_CSUMS_PER_TARGET.lock().unwrap();
+        let device_on_target = DeviceOnTarget {
+            device_name: device_name.to_string(),
+            target_id: target_id.to_string(),
+        };
+        prev_csum_guard.insert(device_on_target, csum);
+    } else {
+        let mut prev_csum_guard = PREVIOUS_CSUMS.lock().unwrap();
+        prev_csum_guard.insert(device_name, csum);
+    }
+}
+
+pub(crate) fn serialize_state(per_target_bitmaps: bool) -> Vec<u8> {
+    let prev_key_fingerprint = &*PREVIOUS_KEY_FINGERPRINT.lock().unwrap();
+    if per_target_bitmaps {
+        let prev_csums = &*PREVIOUS_CSUMS_PER_TARGET.lock().unwrap();
+        bincode::serialize(&(prev_csums, prev_key_fingerprint)).unwrap()
+    } else {
+        let prev_csums = &*PREVIOUS_CSUMS.lock().unwrap();
+        bincode::serialize(&(prev_csums, prev_key_fingerprint)).unwrap()
+    }
+}
+
+pub(crate) fn deserialize_state(data: &[u8], per_target_bitmaps: bool) -> Result<(), Error> {
+    let prev_key_fingerprint;
+    if per_target_bitmaps {
+        let prev_csums;
+        (prev_csums, prev_key_fingerprint) = bincode::deserialize(data)?;
+        let mut prev_csums_guard = PREVIOUS_CSUMS_PER_TARGET.lock().unwrap();
+        *prev_csums_guard = prev_csums;
+    } else {
+        let prev_csums;
+        (prev_csums, prev_key_fingerprint) = bincode::deserialize(data)?;
+        let mut prev_csums_guard = PREVIOUS_CSUMS.lock().unwrap();
+        *prev_csums_guard = prev_csums;
+    }
     let mut prev_key_fingerprint_guard = PREVIOUS_KEY_FINGERPRINT.lock().unwrap();
-    *prev_csums_guard = prev_csums;
     *prev_key_fingerprint_guard = prev_key_fingerprint;
     Ok(())
 }
@@ -139,10 +195,11 @@ pub(crate) fn check_last_incremental_csum(
     archive_name: &BackupArchiveName,
     device_name: &str,
     device_size: u64,
+    target_id: Option<&str>,
 ) -> bool {
-    match PREVIOUS_CSUMS.lock().unwrap().get(device_name) {
+    match get_last_incremental_csum(device_name, target_id) {
         Some(csum) => manifest
-            .verify_file(archive_name, csum, device_size)
+            .verify_file(archive_name, &csum, device_size)
             .is_ok(),
         None => false,
     }
@@ -188,6 +245,7 @@ pub(crate) async fn register_image(
     known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
     device_name: String,
     device_size: u64,
+    target_id: Option<String>,
     chunk_size: u64,
     incremental: bool,
 ) -> Result<c_int, Error> {
@@ -208,7 +266,7 @@ pub(crate) async fn register_image(
     let mut initial_index = Arc::new(None);
 
     if incremental {
-        let csum = PREVIOUS_CSUMS.lock().unwrap().get(&device_name).copied();
+        let csum = get_last_incremental_csum(&device_name, target_id.as_deref());
 
         if let Some(csum) = csum {
             param["reuse-csum"] = hex::encode(csum).into();
@@ -264,6 +322,7 @@ pub(crate) async fn register_image(
         device_name,
         zero_chunk_digest,
         device_size,
+        target_id,
         upload_queue: Some(upload_queue),
         upload_result: Some(upload_result),
     };
@@ -320,8 +379,11 @@ pub(crate) async fn close_image(
 
     let mut guard = registry.lock().unwrap();
     let info = guard.lookup(dev_id)?;
-    let mut prev_csum_guard = PREVIOUS_CSUMS.lock().unwrap();
-    prev_csum_guard.insert(info.device_name.clone(), upload_result.csum);
+    set_last_incremental_csum(
+        info.device_name.clone(),
+        info.target_id.clone(),
+        upload_result.csum,
+    );
 
     let mut guard = manifest.lock().unwrap();
     guard.add_file(
diff --git a/src/lib.rs b/src/lib.rs
index 1d4ea21..a21573a 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -408,6 +408,7 @@ pub extern "C" fn proxmox_backup_check_incremental(
     handle: *mut ProxmoxBackupHandle,
     device_name: *const c_char, // expect utf8 here
     size: u64,
+    target_id: *const c_char, // expect utf8 here
 ) -> c_int {
     let task = backup_handle_to_task(handle);
 
@@ -415,9 +416,11 @@ pub extern "C" fn proxmox_backup_check_incremental(
         return 0;
     }
 
-    match tools::utf8_c_string_lossy(device_name) {
-        None => 0,
-        Some(device_name) => i32::from(task.check_incremental(device_name, size)),
+    if let Some(device_name) = tools::utf8_c_string_lossy(device_name) {
+        let target_id = tools::utf8_c_string_lossy(target_id);
+        i32::from(task.check_incremental(device_name, size, target_id))
+    } else {
+        0
     }
 }
 
@@ -428,6 +431,7 @@ pub extern "C" fn proxmox_backup_register_image(
     handle: *mut ProxmoxBackupHandle,
     device_name: *const c_char, // expect utf8 here
     size: u64,
+    target_id: *const c_char, // expect utf8 here
     incremental: bool,
     error: *mut *mut c_char,
 ) -> c_int {
@@ -441,6 +445,7 @@ pub extern "C" fn proxmox_backup_register_image(
         handle,
         device_name,
         size,
+        target_id,
         incremental,
         callback_info.callback,
         callback_info.callback_data,
@@ -464,6 +469,7 @@ pub extern "C" fn proxmox_backup_register_image_async(
     handle: *mut ProxmoxBackupHandle,
     device_name: *const c_char, // expect utf8 here
     size: u64,
+    target_id: *const c_char, // expect utf8 here
     incremental: bool,
     callback: extern "C" fn(*mut c_void),
     callback_data: *mut c_void,
@@ -481,9 +487,12 @@ pub extern "C" fn proxmox_backup_register_image_async(
     param_not_null!(device_name, callback_info);
 
     let device_name = unsafe { tools::utf8_c_string_lossy_non_null(device_name) };
+    let target_id = tools::utf8_c_string_lossy(target_id);
 
     task.runtime().spawn(async move {
-        let result = task.register_image(device_name, size, incremental).await;
+        let result = task
+            .register_image(device_name, size, target_id, incremental)
+            .await;
         callback_info.send_result(result);
     });
 }
@@ -1175,8 +1184,8 @@ pub extern "C" fn proxmox_restore_read_image_at_async(
 /// be freed with proxmox_free_state_buf.
 #[no_mangle]
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
-pub extern "C" fn proxmox_export_state(buf_size: *mut usize) -> *mut u8 {
-    let data = commands::serialize_state();
+pub extern "C" fn proxmox_export_state(buf_size: *mut usize, per_target_bitmaps: bool) -> *mut u8 {
+    let data = commands::serialize_state(per_target_bitmaps);
     let len = data.len();
     // Allocate via libc::malloc so the matching free in proxmox_free_state_buf
     // does not need to know the layout used by Rust's global allocator.
@@ -1200,10 +1209,10 @@ pub extern "C" fn proxmox_export_state(buf_size: *mut usize) -> *mut u8 {
 /// will be logged to stderr, but the function will not fail.
 #[no_mangle]
 #[allow(clippy::not_unsafe_ptr_arg_deref)]
-pub extern "C" fn proxmox_import_state(buf: *const u8, buf_size: usize) {
+pub extern "C" fn proxmox_import_state(buf: *const u8, buf_size: usize, per_target_bitmaps: bool) {
     let data = unsafe { std::slice::from_raw_parts(buf, buf_size) };
     // ignore errors, just log what happened
-    if let Err(err) = commands::deserialize_state(data) {
+    if let Err(err) = commands::deserialize_state(data, per_target_bitmaps) {
         eprintln!("error deserializing PBS state - {}", err);
     }
 }
-- 
2.47.3

[RFC proxmox-backup-qemu 5/9] update current-api.h

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 current-api.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/current-api.h b/current-api.h
index 60d7558..c7ebc49 100644
--- a/current-api.h
+++ b/current-api.h
@@ -155,7 +155,8 @@ void proxmox_backup_abort(struct ProxmoxBackupHandle *handle, const char *reason
  */
 int proxmox_backup_check_incremental(struct ProxmoxBackupHandle *handle,
                                      const char *device_name,
-                                     uint64_t size);
+                                     uint64_t size,
+                                     const char *target_id);
 
 /**
  * Register a backup image (sync)
@@ -163,6 +164,7 @@ int proxmox_backup_check_incremental(struct ProxmoxBackupHandle *handle,
 int proxmox_backup_register_image(struct ProxmoxBackupHandle *handle,
                                   const char *device_name,
                                   uint64_t size,
+                                  const char *target_id,
                                   bool incremental,
                                   char **error);
 
@@ -176,6 +178,7 @@ int proxmox_backup_register_image(struct ProxmoxBackupHandle *handle,
 void proxmox_backup_register_image_async(struct ProxmoxBackupHandle *handle,
                                          const char *device_name,
                                          uint64_t size,
+                                         const char *target_id,
                                          bool incremental,
                                          void (*callback)(void*),
                                          void *callback_data,
@@ -431,13 +434,13 @@ void proxmox_restore_read_image_at_async(struct ProxmoxRestoreHandle *handle,
  * Length of the returned buffer is written to buf_size. Returned buffer must
  * be freed with proxmox_free_state_buf.
  */
-uint8_t *proxmox_export_state(uintptr_t *buf_size);
+uint8_t *proxmox_export_state(uintptr_t *buf_size, bool per_target_bitmaps);
 
 /**
  * Load state serialized by proxmox_export_state. If loading fails, a message
  * will be logged to stderr, but the function will not fail.
  */
-void proxmox_import_state(const uint8_t *buf, uintptr_t buf_size);
+void proxmox_import_state(const uint8_t *buf, uintptr_t buf_size, bool per_target_bitmaps);
 
 /**
  * Free a buffer acquired from proxmox_export_state.
-- 
2.47.3

[RFC proxmox-backup-qemu 6/9] d/control: bump versioned breaks for pve-qemu-kvm

[Fiona Ebner]

FIXME: use the proper version for when the feature is actually introduced

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 debian/control | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/debian/control b/debian/control
index cc8c90c..ebbf857 100644
--- a/debian/control
+++ b/debian/control
@@ -118,7 +118,7 @@ Package: libproxmox-backup-qemu0
 Architecture: any
 Depends: ${misc:Depends},
          ${shlibs:Depends}
-Breaks: pve-qemu-kvm (<< 5.2.0-1)
+Breaks: pve-qemu-kvm (<< 11.0.3-3)
 Description: Proxmox Backup Server client library for QEMU
  This library contains the library to access the Proxmox Backup server from
  within QEMU.
-- 
2.47.3

[RFC qemu 7/9] PVE backup: properly track if snapshot access was set up

[Fiona Ebner]

Regular backups set the target ID to "Proxmox". When using QMP
'backup-access-teardown' there is a check to error out when it's not a
regular backup, but this wrongly compared against "Proxmox VE",
rendering the check moot. This should not happen via the Proxmox VE
stack anyway, but still good to fix up. Avoid deciding implicitly
based on the target ID, keep track of whether the backup was set up
for snapshot access or if it is a regular backup explicitly.

In preparation to support different target IDs for regular backups
too.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 pve-backup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/pve-backup.c b/pve-backup.c
index ad0f8668fd..8a97f0425e 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -74,6 +74,7 @@ static struct PVEBackupState {
     CoMutex backup_mutex;
     CoMutex dump_callback_mutex;
     char *target_id;
+    bool is_backup_access;
 } backup_state;
 
 static void pvebackup_init(void)
@@ -1168,6 +1169,7 @@ BackupAccessInfoList *coroutine_fn qmp_backup_access_setup(
     qemu_mutex_unlock(&backup_state.stat.lock);
 
     backup_state_set_target_id(target_id);
+    backup_state.is_backup_access = true;
 
     backup_state.vmaw = NULL;
     backup_state.pbs = NULL;
@@ -1316,7 +1318,7 @@ void coroutine_fn qmp_backup_access_teardown(const char *target_id, bool success
         return;
     }
 
-    if (!strcmp(backup_state.target_id, "Proxmox VE")) {
+    if (!backup_state.is_backup_access) {
         error_setg(errp, "cannot teardown backup access for PVE - use backup-cancel instead");
         qemu_co_mutex_unlock(&backup_state.backup_mutex);
         return;
@@ -1594,6 +1596,7 @@ UuidInfo coroutine_fn *qmp_backup(
     backup_state.pbs = pbs;
 
     backup_state_set_target_id("Proxmox");
+    backup_state.is_backup_access = false;
 
     backup_state.di_list = di_list;
 
-- 
2.47.3

[RFC qemu 8/9] PVE backup: track per-target dirty bitmaps

[Fiona Ebner]

Allow users of the QMP 'backup' call to specify a target ID, so a
dirty bitmap can be tracked per target.

The PBS state migration needs to be guarded by machine version for
migration compatibility to nodes that don't yet have the feature. For
this to work, qemu-server is responsible to pass in the target ID when
the machine version is new enough.

Make the PBS state a proper object to make use of the existing
machinery for feature guarding by machine version.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 block/monitor/block-hmp-cmds.c |  1 +
 hw/core/machine.c              |  2 ++
 migration/pbs-state.c          | 42 ++++++++++++++++++++++++++++------
 migration/pbs-state.h          | 32 ++++++++++++++++++++++++++
 proxmox-backup-client.c        | 12 +++++++++-
 proxmox-backup-client.h        |  1 +
 pve-backup.c                   | 24 ++++++++++++++-----
 qapi/block-core.json           |  7 +++++-
 8 files changed, 106 insertions(+), 15 deletions(-)
 create mode 100644 migration/pbs-state.h

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 3b9c3d223e..2d72e2dacb 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -1052,6 +1052,7 @@ void coroutine_fn hmp_backup(Monitor *mon, const QDict *qdict)
         devlist, qdict_haskey(qdict, "speed"), speed,
         false, 0, // BackupPerf max-workers
         false, false, // fleecing
+        NULL, // target ID
         &error);
 
     hmp_handle_error(mon, error);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0aa77a57e9..ae1eb9c900 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -41,6 +41,8 @@
 GlobalProperty hw_compat_10_2[] = {
     { "scsi-block", "migrate-pr", "off" },
     { "isa-cirrus-vga", "global-vmstate", "true" },
+    // FIXME add to correct machine version
+    { "pbs-state", "per-target-bitmaps", "false" },
 };
 const size_t hw_compat_10_2_len = G_N_ELEMENTS(hw_compat_10_2);
 
diff --git a/migration/pbs-state.c b/migration/pbs-state.c
index a97187e4d7..3dbbfc8c68 100644
--- a/migration/pbs-state.c
+++ b/migration/pbs-state.c
@@ -5,14 +5,11 @@
 #include "qemu/osdep.h"
 #include "migration/misc.h"
 #include "qemu-file.h"
+#include "migration/pbs-state.h"
 #include "migration/vmstate.h"
 #include "migration/register.h"
 #include "proxmox-backup-qemu.h"
 
-typedef struct PBSState {
-    bool active;
-} PBSState;
-
 /* state is accessed via this static variable directly, 'opaque' is NULL */
 static PBSState pbs_state;
 
@@ -36,7 +33,7 @@ static int pbs_state_load(QEMUFile *f, void *opaque, int version_id)
         return -EIO;
     }
 
-    proxmox_import_state(buf, buf_size);
+    proxmox_import_state(buf, buf_size, pbs_state.per_target_bitmaps);
 
     free(buf);
     return 0;
@@ -46,7 +43,7 @@ static int pbs_state_load(QEMUFile *f, void *opaque, int version_id)
 static int pbs_state_save_setup(QEMUFile *f, void *opaque, Error **errp)
 {
     size_t buf_size;
-    uint8_t *buf = proxmox_export_state(&buf_size);
+    uint8_t *buf = proxmox_export_state(&buf_size, pbs_state.per_target_bitmaps);
 
     /* LV encoding */
     qemu_put_be64(f, buf_size);
@@ -97,8 +94,39 @@ static SaveVMHandlers savevm_pbs_state_handlers = {
 
 void pbs_state_mig_init(void)
 {
-    pbs_state.active = true;
+    object_initialize(&pbs_state, sizeof(pbs_state), TYPE_PBS_STATE);
     register_savevm_live("pbs-state", 0, 1,
                          &savevm_pbs_state_handlers,
                          NULL);
 }
+
+static void pbs_state_class_init(ObjectClass *klass, const void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->user_creatable = false;
+    device_class_set_props(dc, pbs_state_properties);
+}
+
+static const TypeInfo pbs_state_type = {
+    .name = TYPE_PBS_STATE,
+    /*
+     * NOTE: TYPE_PBS_STATE is not really a device, as the object is
+     * not created using qdev_new(), it is not attached to the qdev
+     * device tree, and it is never realized.
+     *
+     * TODO: Make this TYPE_OBJECT once QOM provides something like
+     * TYPE_DEVICE's "-global" properties.
+     */
+    .parent = TYPE_DEVICE,
+    .class_init = pbs_state_class_init,
+    .class_size = sizeof(PBSStateClass),
+    .instance_size = sizeof(PBSState),
+};
+
+static void register_pbs_state_type(void)
+{
+    type_register_static(&pbs_state_type);
+}
+
+type_init(register_pbs_state_type);
diff --git a/migration/pbs-state.h b/migration/pbs-state.h
new file mode 100644
index 0000000000..d2a3be8b0b
--- /dev/null
+++ b/migration/pbs-state.h
@@ -0,0 +1,32 @@
+/*
+ * PBS (dirty-bitmap) state migration
+ */
+
+#ifndef QEMU_MIGRATION_PBS_STATE_H
+#define QEMU_MIGRATION_PBS_STATE_H
+
+#include "hw/core/qdev-properties.h"
+#include "hw/core/qdev-properties-system.h"
+
+#define TYPE_PBS_STATE "pbs-state"
+
+typedef struct PBSStateClass PBSStateClass;
+OBJECT_DECLARE_TYPE(PBSState, PBSStateClass, PBS_STATE)
+
+struct PBSStateClass {
+    DeviceClass parent_class;
+};
+
+typedef struct PBSState {
+    DeviceState parent_obj;
+
+    bool active;
+    bool per_target_bitmaps;
+} PBSState;
+
+const Property pbs_state_properties[] = {
+    DEFINE_PROP_BOOL("active", PBSState, active, true),
+    DEFINE_PROP_BOOL("per-target-bitmaps", PBSState, per_target_bitmaps, true),
+};
+
+#endif
diff --git a/proxmox-backup-client.c b/proxmox-backup-client.c
index 166604b3a6..e617332fb4 100644
--- a/proxmox-backup-client.c
+++ b/proxmox-backup-client.c
@@ -58,6 +58,7 @@ proxmox_backup_co_register_image(
     ProxmoxBackupHandle *pbs,
     const char *device_name,
     uint64_t size,
+    const char *target_id,
     bool incremental,
     Error **errp)
 {
@@ -68,7 +69,16 @@ proxmox_backup_co_register_image(
     int pbs_res = -1;
 
     proxmox_backup_register_image_async(
-        pbs, device_name, size, incremental, proxmox_backup_schedule_wake, &waker, &pbs_res, &pbs_err);
+        pbs,
+        device_name,
+        size,
+        target_id,
+        incremental,
+        proxmox_backup_schedule_wake,
+        &waker,
+        &pbs_res,
+        &pbs_err
+    );
     qemu_coroutine_yield();
     if (pbs_res < 0) {
         if (errp) error_setg(errp, "backup register image failed: %s", pbs_err ? pbs_err : "unknown error");
diff --git a/proxmox-backup-client.h b/proxmox-backup-client.h
index 8cbf645b2c..3ef06b1a77 100644
--- a/proxmox-backup-client.h
+++ b/proxmox-backup-client.h
@@ -32,6 +32,7 @@ proxmox_backup_co_register_image(
     ProxmoxBackupHandle *pbs,
     const char *device_name,
     uint64_t size,
+    const char *target_id,
     bool incremental,
     Error **errp);
 
diff --git a/pve-backup.c b/pve-backup.c
index 8a97f0425e..0cfd1065ec 100644
--- a/pve-backup.c
+++ b/pve-backup.c
@@ -41,7 +41,6 @@
  *
  */
 
-const char *PBS_BITMAP_NAME = "pbs-incremental-dirty-bitmap";
 const char *BACKGROUND_BITMAP_NAME = "backup-access-background-bitmap";
 
 static struct PVEBackupState {
@@ -1356,6 +1355,7 @@ UuidInfo coroutine_fn *qmp_backup(
     bool has_speed, int64_t speed,
     bool has_max_workers, int64_t max_workers,
     bool has_fleecing, bool fleecing,
+    const char *target_id,
     Error **errp)
 {
     assert(qemu_in_coroutine());
@@ -1427,6 +1427,14 @@ UuidInfo coroutine_fn *qmp_backup(
     clear_backup_state_bitmap_list();
 
     if (format == BACKUP_FORMAT_PBS) {
+        const char *bitmap_name = NULL;
+
+        if (target_id) {
+            bitmap_name = target_id;
+        } else {
+            bitmap_name = "pbs-incremental-dirty-bitmap";
+        }
+
         if (!password) {
             error_set(errp, ERROR_CLASS_GENERIC_ERROR, "missing parameter 'password'");
             goto err_mutex;
@@ -1481,19 +1489,19 @@ UuidInfo coroutine_fn *qmp_backup(
             PBSBitmapAction action = PBS_BITMAP_ACTION_NOT_USED;
             size_t dirty = di->size;
 
-            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(di->bs, PBS_BITMAP_NAME);
+            BdrvDirtyBitmap *bitmap = bdrv_find_dirty_bitmap(di->bs, bitmap_name);
             bool expect_only_dirty = false;
 
             if (has_use_dirty_bitmap && use_dirty_bitmap) {
                 if (bitmap == NULL) {
-                    bitmap = bdrv_create_dirty_bitmap(di->bs, dump_cb_block_size, PBS_BITMAP_NAME, errp);
+                    bitmap = bdrv_create_dirty_bitmap(di->bs, dump_cb_block_size, bitmap_name, errp);
                     if (!bitmap) {
                         goto err_mutex;
                     }
                     action = PBS_BITMAP_ACTION_NEW;
                 } else {
                     expect_only_dirty =
-                        proxmox_backup_check_incremental(pbs, di->device_name, di->size) != 0;
+                        proxmox_backup_check_incremental(pbs, di->device_name, di->size, target_id) != 0;
                 }
 
                 if (expect_only_dirty) {
@@ -1517,7 +1525,7 @@ UuidInfo coroutine_fn *qmp_backup(
                 }
             }
 
-            int dev_id = proxmox_backup_co_register_image(pbs, di->device_name, di->size,
+            int dev_id = proxmox_backup_co_register_image(pbs, di->device_name, di->size, target_id,
                                                           expect_only_dirty, errp);
             if (dev_id < 0) {
                 goto err_mutex;
@@ -1595,7 +1603,11 @@ UuidInfo coroutine_fn *qmp_backup(
     backup_state.vmaw = vmaw;
     backup_state.pbs = pbs;
 
-    backup_state_set_target_id("Proxmox");
+    if (target_id) {
+        backup_state_set_target_id(target_id);
+    } else {
+        backup_state_set_target_id("Proxmox");
+    }
     backup_state.is_backup_access = false;
 
     backup_state.di_list = di_list;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ed37a4a22f..fbad78c56c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1058,6 +1058,10 @@
 #     @devlist, a corresponing '-fleecing' device with the same size
 #     already needs to be present.
 #
+# @target-id: the unique ID of the backup target.  A dirty bitmap is
+#     used for each target.  If no target ID is specified, the default
+#     dirty bitmap is used.
+#
 # Returns: the uuid of the backup job
 #
 ##
@@ -1079,7 +1083,8 @@
                                     '*devlist': 'str',
                                     '*speed': 'int',
                                     '*max-workers': 'int',
-                                    '*fleecing': 'bool' },
+                                    '*fleecing': 'bool',
+                                    '*target-id': 'str' },
   'returns': 'UuidInfo', 'coroutine': true }
 
 ##
-- 
2.47.3

[RFC qemu-server 9/9] close #6169: backup: pbs: specify target ID so QEMU keeps track of per-target dirty bitmap

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/VZDump/QemuServer.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/VZDump/QemuServer.pm b/src/PVE/VZDump/QemuServer.pm
index b1240f8f..8e555817 100644
--- a/src/PVE/VZDump/QemuServer.pm
+++ b/src/PVE/VZDump/QemuServer.pm
@@ -839,6 +839,12 @@ sub archive_pbs {
         $params->{'use-dirty-bitmap'} = JSON::true
             if $qemu_support->{'pbs-dirty-bitmap'} && !$is_template;
 
+        my $machine_type = PVE::QemuServer::Machine::get_current_qemu_machine($vmid);
+        # TODO use correct version for guard!
+        if (PVE::QemuServer::Machine::is_machine_version_at_least($machine_type, 11, 0)) {
+            $params->{'target-id'} = "pbs:$opts->{storage}";
+        }
+
         $params->{timeout} = 125; # give some time to connect to the backup server
 
         $self->loginfo("starting backup via QMP command");
-- 
2.47.3

Re: [RFC qemu/qemu-server/proxmox-backup-qemu 0/9] close #6169: backup: pbs: per-target dirty bitmaps

[Dominik Csapak]

On 9/2/26 3:56 PM, Fiona Ebner wrote:
> To decide whether an incremental backup can be done, the checksum of
> the latest backup on the target is compared against the checksum from
> the previous backup the QEMU client library did.
> 
> This was previously tracked per device as the PVE backup code in QEMU
> also only used a single bitmap per device. So backing up to target A,
> dirtying the disk and backing up to target B would mean that the
> checksum for target A is overwritten and a subsequent backup to target
> A needs to discard the dirty bitmap.
> 
> Allow for tracking multiple targets with different bitmaps, by saving
> the checksums per device+target combination, rather than just per
> device.
> 
> Whether the new behavior or the old behavior is used is controlled by
> QEMU, optionally passing along the target ID, and a flag for the
> export and import functions for migration. These are machine version
> guarded on the QEMU/qemu-server side to avoid a mismatch.
> 
> 
> This is an ABI change for the backup library, so a versioned breaks
> and a versioned depends in the other direction is needed.
> 

i guess it's not really necessary, but couldn't we add new methods
instead of modifying the existing ones? then the breaks pve-qemu-kvm
wouldn't be necessary? (i think?)

not that important since we must bump both anyway

> 
> QUESTION: with old qemu-server, target ID is not provided, but the new
> machine version might be in use. In this case, the dirty bitmap state
> is not migrated, in case of a new -> old -> new migration, which can
> lead to stale info! Would it be okay to add a Breaks for old
> qemu-server or should I try to resolve it differently somehow?

is such migration to an older qemu-server even supported?
IIUC the issue is that the
1. there is a bitmap with a target
2. old qemu-server makes a backup without a target creating
    a new dirty-bitmap (actually invalidating the old one, but
    since the code doesn't know about it doesn't do anything with
    it)
3. the new code uses the target name again, bitmap still existing

the checksum would be different though and it would not be an
incremental backup once, or?

question is, should we support both modes simultaniously at all?
we could require a target always, or if no target is given,
invalidate all named target bitmaps.

also, the same can happen with new versions only too?
backup to target storage X
on pve add identical storage Y -> backup to same storage as
different id
backup with X again -> stale bitmap?

> 
> QUESTION: How to best avoid 'orphaned' bitmaps? Auto-remove for
> storages no longer in configuration? Or have some knob/limit for how
> many to keep?
> 

i think it depends how big these are? I doubt many users
make many backups to storages they abandon later

also how could we differ between a user just recreating
a storage entry (for whatever reason) and a genuine
deletion? (if the backup timing is right)

> 
> The clippy fixes are independent.
> 
> proxmox-backup-qemu:
> 
> Fiona Ebner (6):
>    clippy: restore: fix needless borrows
>    clippy: commands: avoid manual implementation of ok()
>    cargo: add dependency for serde
>    close #6169: track checksums for incremental backups per target
>    update current-api.h
>    d/control: bump versioned breaks for pve-qemu-kvm
> 
>   Cargo.toml      |  1 +
>   current-api.h   |  9 +++--
>   debian/control  |  2 +-
>   src/backup.rs   | 27 ++++++++++----
>   src/commands.rs | 97 +++++++++++++++++++++++++++++++++++++++----------
>   src/lib.rs      | 25 +++++++++----
>   src/restore.rs  | 12 ++----
>   7 files changed, 126 insertions(+), 47 deletions(-)
> 
> 
> qemu:
> 
> Fiona Ebner (2):
>    PVE backup: properly track if snapshot access was set up
>    PVE backup: track per-target dirty bitmaps
> 
>   block/monitor/block-hmp-cmds.c |  1 +
>   hw/core/machine.c              |  2 ++
>   migration/pbs-state.c          | 42 ++++++++++++++++++++++++++++------
>   migration/pbs-state.h          | 32 ++++++++++++++++++++++++++
>   proxmox-backup-client.c        | 12 +++++++++-
>   proxmox-backup-client.h        |  1 +
>   pve-backup.c                   | 29 +++++++++++++++++------
>   qapi/block-core.json           |  7 +++++-
>   8 files changed, 110 insertions(+), 16 deletions(-)
>   create mode 100644 migration/pbs-state.h
> 
> 
> qemu-server:
> 
> Fiona Ebner (1):
>    close #6169: backup: pbs: specify target ID so QEMU keeps track of
>      per-target dirty bitmap
> 
>   src/PVE/VZDump/QemuServer.pm | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> 
> Summary over all repositories:
>    16 files changed, 242 insertions(+), 63 deletions(-)
>