[PATCH v1 proxmox-backup 0/3] fix incremental qemu backup

[PATCH v1 proxmox-backup 0/3] fix incremental qemu backup

[Robert Obkircher]

Fix a bug, improve api docs, and disallow API usage that we don't need.

Robert Obkircher (3):
  api: backup: don't verify total file size for incremental backups
  api: backup: improve schema documentation of fixed_close
  api: backup: disallow incremental backups with different index length

 src/api2/backup/environment.rs | 14 +++++++-------
 src/api2/backup/mod.rs         | 15 ++++++++++++++-
 2 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH v1 proxmox-backup 1/3] api: backup: don't verify total file size for incremental backups

[Robert Obkircher]

This fixes incremental qemu backups, where the submitted value is the
upload size instead of the total size.

Avoiding the check is preferable over breaking backwards compatability
of the API because it was only relevant for detecting potential bugs.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/api2/backup/environment.rs | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
index 657daa41..ab623f1f 100644
--- a/src/api2/backup/environment.rs
+++ b/src/api2/backup/environment.rs
@@ -632,14 +632,14 @@ impl BackupEnvironment {
                     );
                 }
             }
-        }
 
-        let writer_size = data.index.size();
-        if size != writer_size {
-            bail!(
-                "fixed writer '{}' close failed - unexpected size ({size} != {writer_size})",
-                data.name,
-            );
+            let writer_size = data.index.size();
+            if size != writer_size {
+                bail!(
+                    "fixed writer '{}' close failed - unexpected size ({size} != {writer_size})",
+                    data.name,
+                );
+            }
         }
 
         let expected_csum = data.index.close()?;
-- 
2.47.3

Re: [PATCH v1 proxmox-backup 1/3] api: backup: don't verify total file size for incremental backups

[Christian Ebner]

On 3/10/26 5:09 PM, Robert Obkircher wrote:
> This fixes incremental qemu backups, where the submitted value is the
> upload size instead of the total size.

Should we fix the `size` in the close_image() call on 
proxmox-backup-qemu as well? This is documented as the file size, not 
the upload size after all. Didn't look into it to deep, but 
`info.device_size` should be the correct value there. This can be a 
followup though and should not block/delay this crucial bugfix.

> 
> Avoiding the check is preferable over breaking backwards compatability
> of the API because it was only relevant for detecting potential bugs.
> 

It is best practice to blame the commit introducing the issue for reference:

Fixes: 760f40de ("api: verify fixed index writer size on close")

> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   src/api2/backup/environment.rs | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/api2/backup/environment.rs b/src/api2/backup/environment.rs
> index 657daa41..ab623f1f 100644
> --- a/src/api2/backup/environment.rs
> +++ b/src/api2/backup/environment.rs
> @@ -632,14 +632,14 @@ impl BackupEnvironment {
>                       );
>                   }
>               }
> -        }
>   
> -        let writer_size = data.index.size();
> -        if size != writer_size {
> -            bail!(
> -                "fixed writer '{}' close failed - unexpected size ({size} != {writer_size})",
> -                data.name,
> -            );
> +            let writer_size = data.index.size();
> +            if size != writer_size {
> +                bail!(
> +                    "fixed writer '{}' close failed - unexpected size ({size} != {writer_size})",
> +                    data.name,
> +                );
> +            }
>           }
>   
>           let expected_csum = data.index.close()?;

Re: [PATCH v1 proxmox-backup 1/3] api: backup: don't verify total file size for incremental backups

[Thomas Lamprecht]

Am 10.03.26 um 18:03 schrieb Christian Ebner:
> On 3/10/26 5:09 PM, Robert Obkircher wrote:
>> This fixes incremental qemu backups, where the submitted value is the
>> upload size instead of the total size.
> 
> Should we fix the `size` in the close_image() call on proxmox-backup-qemu as well? This is documented as the file size, not the upload size after all. Didn't look into it to deep, but `info.device_size` should be the correct value there. This can be a followup though and should not block/delay this crucial bugfix.
> 
>>
>> Avoiding the check is preferable over breaking backwards compatability
>> of the API because it was only relevant for detecting potential bugs.
>>
> 
> It is best practice to blame the commit introducing the issue for reference:
> 
> Fixes: 760f40de ("api: verify fixed index writer size on close")

btw. b4 picked this up, so many thanks for replying with the correct
format!

[PATCH v1 proxmox-backup 2/3] api: backup: improve schema documentation of fixed_close

[Robert Obkircher]

The chunk-count has always been compared to the number of digests
written to the fixed index file. It was never ignored.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/api2/backup/mod.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 4d0846ca..3c044c78 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -765,7 +765,7 @@ pub const API_METHOD_CLOSE_FIXED_INDEX: ApiMethod = ApiMethod::new(
             (
                 "chunk-count",
                 false,
-                &IntegerSchema::new("Chunk count. This is used to verify that the server got all chunks. Ignored for incremental backups.")
+                &IntegerSchema::new("Chunk count. This is used to verify that the server got all chunks.")
                     .minimum(0)
                     .schema()
             ),
-- 
2.47.3

Re: [PATCH v1 proxmox-backup 2/3] api: backup: improve schema documentation of fixed_close

[Christian Ebner]

On 3/10/26 5:08 PM, Robert Obkircher wrote:
> The chunk-count has always been compared to the number of digests
> written to the fixed index file. It was never ignored.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   src/api2/backup/mod.rs | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 4d0846ca..3c044c78 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -765,7 +765,7 @@ pub const API_METHOD_CLOSE_FIXED_INDEX: ApiMethod = ApiMethod::new(
>               (
>                   "chunk-count",
>                   false,
> -                &IntegerSchema::new("Chunk count. This is used to verify that the server got all chunks. Ignored for incremental backups.")
> +                &IntegerSchema::new("Chunk count. This is used to verify that the server got all chunks.")

This should probably state that this is the count of new and/or 
re-indexed chunks. That is true for both incremental and non-incremental 
backups.

It is however not always the total chunk count of the index file, which 
is only checked if this is not an incremental backup.

>                       .minimum(0)
>                       .schema()
>               ),

[PATCH v1 proxmox-backup 3/3] api: backup: disallow incremental backups with different index length

[Robert Obkircher]

Restore the previous restrictions because fixed index files are only
reused for qemu backups where the total size never changes.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/api2/backup/mod.rs | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 3c044c78..4a37947b 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -498,6 +498,11 @@ fn create_fixed_index(
     let mut incremental = false;
     if let Some(csum) = reuse_csum {
         incremental = true;
+
+        if size.is_none() {
+            bail!("size is required for incremental backups");
+        }
+
         let last_backup = match &env.last_backup {
             Some(info) => info,
             None => {
@@ -531,6 +536,14 @@ fn create_fixed_index(
     let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
 
     if let Some(reader) = reader {
+        // compares chunk count instead of size for backwards compatability
+        if writer.index_length() != reader.index_count() {
+            bail!(
+                "cannot reuse index - index sizes not equal ({} != {})",
+                writer.index_length(),
+                reader.index_count()
+            );
+        }
         writer.clone_data_from(&reader)?;
     }
 
-- 
2.47.3

Re: [PATCH v1 proxmox-backup 3/3] api: backup: disallow incremental backups with different index length

[Christian Ebner]

On 3/10/26 5:09 PM, Robert Obkircher wrote:
> Restore the previous restrictions because fixed index files are only
> reused for qemu backups where the total size never changes.
> 

Fixes: 28fb8143 ("api: backup: make fixed index file size optional")

> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   src/api2/backup/mod.rs | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 3c044c78..4a37947b 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -498,6 +498,11 @@ fn create_fixed_index(
>       let mut incremental = false;
>       if let Some(csum) = reuse_csum {
>           incremental = true;
> +
> +        if size.is_none() {
> +            bail!("size is required for incremental backups");
> +        }
> +
>           let last_backup = match &env.last_backup {
>               Some(info) => info,
>               None => {
> @@ -531,6 +536,14 @@ fn create_fixed_index(
>       let mut writer = env.datastore.create_fixed_writer(&path, size, chunk_size)?;
>   
>       if let Some(reader) = reader {
> +        // compares chunk count instead of size for backwards compatability

typo: s/compatability/compatibility

> +        if writer.index_length() != reader.index_count() {
> +            bail!(
> +                "cannot reuse index - index sizes not equal ({} != {})",
> +                writer.index_length(),
> +                reader.index_count()
> +            );
> +        }
>           writer.clone_data_from(&reader)?;
>       }
>   

Re: [PATCH v1 proxmox-backup 3/3] api: backup: disallow incremental backups with different index length

[Thomas Lamprecht]

Am 10.03.26 um 18:15 schrieb Christian Ebner:
> On 3/10/26 5:09 PM, Robert Obkircher wrote:
>> Restore the previous restrictions because fixed index files are only
>> reused for qemu backups where the total size never changes.

btw. when increasing a vdisk size we (or others that want to backup a
fixed index to PBS) theoretically could reuse the previous bitmap,
extend it and mark the new area as dirty for efficiency, so this is
not something that is bogus out of principle.

Also, a bit more rationale would be appreciated here, especially in
combination with sending this along with a quite important bug fix.

Re: [PATCH v1 proxmox-backup 0/3] fix incremental qemu backup

[Stoiko Ivanov]

Thanks for quickly coming up with a patch.

applying this series fixes the broken backups with qemu-vms we ran into
today.

Insofar consider this:
Tested-by: Stoiko Ivanov <s.ivanov@proxmox.com>


On Tue, 10 Mar 2026 17:03:57 +0100
Robert Obkircher <r.obkircher@proxmox.com> wrote:

> Fix a bug, improve api docs, and disallow API usage that we don't need.
> 
> Robert Obkircher (3):
>   api: backup: don't verify total file size for incremental backups
>   api: backup: improve schema documentation of fixed_close
>   api: backup: disallow incremental backups with different index length
> 
>  src/api2/backup/environment.rs | 14 +++++++-------
>  src/api2/backup/mod.rs         | 15 ++++++++++++++-
>  2 files changed, 21 insertions(+), 8 deletions(-)
> 

Re: [PATCH v1 proxmox-backup 0/3] fix incremental qemu backup

[Christian Ebner]

On 3/10/26 5:08 PM, Robert Obkircher wrote:
> Fix a bug, improve api docs, and disallow API usage that we don't need.
> 
> Robert Obkircher (3):
>    api: backup: don't verify total file size for incremental backups
>    api: backup: improve schema documentation of fixed_close
>    api: backup: disallow incremental backups with different index length
> 
>   src/api2/backup/environment.rs | 14 +++++++-------
>   src/api2/backup/mod.rs         | 15 ++++++++++++++-
>   2 files changed, 21 insertions(+), 8 deletions(-)
> 

Thanks for the quick fix and sorry that this slipped trough during 
review/testing.

Also can confirm that this fixes the incremental backups with dirty 
bitmap tracking as used by proxmox-backup-qemu.

Tested new and incremental VM backup.
Confirmed streaming backup inputs via /dev/stdin still works as 
expected, also when done incrementally.

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
Tested-by: Christian Ebner <c.ebner@proxmox.com>

applied: [PATCH v1 proxmox-backup 0/3] fix incremental qemu backup

[Thomas Lamprecht]

On Tue, 10 Mar 2026 17:03:57 +0100, Robert Obkircher wrote:
> Fix a bug, improve api docs, and disallow API usage that we don't need.
> 
> Robert Obkircher (3):
>   api: backup: don't verify total file size for incremental backups
>   api: backup: improve schema documentation of fixed_close
>   api: backup: disallow incremental backups with different index length
> 
> [...]

Applied, with the minor improvements Chris pointed out squashed in, thanks!

[1/3] api: backup: don't verify total file size for incremental backups
      commit: 09ca2b578bc446cd54444e903cd4ba42b3d2968b
[2/3] api: backup: improve schema documentation of fixed_close
      commit: 041a8f688e897ffa3204f158b888244d04ab8c8e
[3/3] api: backup: disallow incremental backups with different index length
      commit: 389bcff4a6641ccbc071f4ed7c3dd5ffbcc4bd5d