[pbs-devel] [PATCH v4 proxmox-backup 0/2] fix rare race in garbage collection

[pbs-devel] [PATCH v4 proxmox-backup 0/2] fix rare race in garbage collection

[Christian Ebner]

Fixes a rare race in phase 1 of garbage collection, as described in
more detail in the commit message of patch 2.
This patches are intended as stop gap for the time being, until a
different approach to guarantee the presence of index files also
after pruning has been explored and implemented, as suggested by
Thomas [0].

Changes since version 3:
- Refactor open_index_reader to allow distinction between missing index
  file and ignored archive type blob.
- Iterate snapshots from newest to oldest to further reduce race window
  and simplify accounting logic.

[0] https://lore.proxmox.com/pbs-devel/7bac779b-e564-4f24-b58c-a4411c2a59aa@proxmox.com/

Christian Ebner (2):
  garbage collection: distinguish variants for failed open index reader
  garbage collection: fix rare race in chunk marking phase

 pbs-datastore/src/datastore.rs | 141 ++++++++++++++++++++++-----------
 1 file changed, 96 insertions(+), 45 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH v4 proxmox-backup 1/2] garbage collection: distinguish variants for failed open index reader

[Christian Ebner]

In preparation for adapting the garbage collection to be able to
distinguish cases for the index file not being accessible because it
is no longer present or because it is a blob and therefore should be
ignored.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index aa38e2ac1..4630b1b39 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1033,11 +1033,14 @@ impl DataStore {
 
     // Similar to open index, but ignore index files with blob or unknown archive type.
     // Further, do not fail if file vanished.
-    fn open_index_reader(&self, absolute_path: &Path) -> Result<Option<Box<dyn IndexFile>>, Error> {
+    fn open_index_reader(
+        &self,
+        absolute_path: &Path,
+    ) -> Result<IndexReaderOption<Box<dyn IndexFile>>, Error> {
         let archive_type = match ArchiveType::from_path(absolute_path) {
-            Ok(archive_type) => archive_type,
             // ignore archives with unknown archive type
-            Err(_) => return Ok(None),
+            Ok(ArchiveType::Blob) | Err(_) => return Ok(IndexReaderOption::NoneWrongType),
+            Ok(archive_type) => archive_type,
         };
 
         if absolute_path.is_relative() {
@@ -1047,7 +1050,9 @@ impl DataStore {
         let file = match std::fs::File::open(absolute_path) {
             Ok(file) => file,
             // ignore vanished files
-            Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(None),
+            Err(err) if err.kind() == io::ErrorKind::NotFound => {
+                return Ok(IndexReaderOption::NoneNotFound)
+            }
             Err(err) => {
                 return Err(Error::from(err).context(format!("can't open file '{absolute_path:?}'")))
             }
@@ -1057,14 +1062,14 @@ impl DataStore {
             ArchiveType::FixedIndex => {
                 let reader = FixedIndexReader::new(file)
                     .with_context(|| format!("can't open fixed index '{absolute_path:?}'"))?;
-                Ok(Some(Box::new(reader)))
+                Ok(IndexReaderOption::Some(Box::new(reader)))
             }
             ArchiveType::DynamicIndex => {
                 let reader = DynamicIndexReader::new(file)
                     .with_context(|| format!("can't open dynamic index '{absolute_path:?}'"))?;
-                Ok(Some(Box::new(reader)))
+                Ok(IndexReaderOption::Some(Box::new(reader)))
             }
-            ArchiveType::Blob => Ok(None),
+            ArchiveType::Blob => Ok(IndexReaderOption::NoneWrongType),
         }
     }
 
@@ -1155,8 +1160,8 @@ impl DataStore {
                         path.push(file);
 
                         let index = match self.open_index_reader(&path)? {
-                            Some(index) => index,
-                            None => {
+                            IndexReaderOption::Some(index) => index,
+                            IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
                                 unprocessed_index_list.remove(&path);
                                 continue;
                             }
@@ -1191,8 +1196,8 @@ impl DataStore {
         let mut strange_paths_count = unprocessed_index_list.len();
         for path in unprocessed_index_list {
             let index = match self.open_index_reader(&path)? {
-                Some(index) => index,
-                None => {
+                IndexReaderOption::Some(index) => index,
+                IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
                     // do not count vanished (pruned) backup snapshots as strange paths.
                     strange_paths_count -= 1;
                     continue;
@@ -1695,3 +1700,9 @@ impl DataStore {
         *OLD_LOCKING
     }
 }
+
+enum IndexReaderOption<T> {
+    Some(T),
+    NoneWrongType,
+    NoneNotFound,
+}
-- 
2.39.5



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

[pbs-devel] [PATCH v4 proxmox-backup 2/2] garbage collection: fix rare race in chunk marking phase

[Christian Ebner]

During phase 1 of garbage collection referenced chunks are marked as
in use by iterating over all index files and updating the atime on
the chunks referenced by these.

In an edge case for long running garbage collection jobs, where a
newly added snapshot (created after the start of GC) reused known
chunks from a previous snapshot, but the previous snapshot index
referencing them disappeared before the marking phase could reach
that index (e.g. pruned because only 1 snapshot to be kept by
retention setting), known chunks from that previous index file might
not be marked (given that by none of the other index files it was
marked).

Since commit 74361da8 ("garbage collection: generate index file list
via datastore iterators") this is even less likely as now the
iteration reads also index files added during phase 1, and
therefore either the new or the previous index file will account for
these chunks (the previous backup snapshot can only be prunded after
the new one finished, since locked). There remains however a small
race window between the reading of the snapshots in the backup group
and the reading of the actual index files for marking.

Fix this race by:
1. Checking if the last snapshot of a group disappeared and if so
2. generate the list again, looking for new index files previously
   not accounted for
3. To avoid possible endless looping, lock the group if the snapshot
   list changed even after the 10th time (which will lead to
   concurrent operations to this group failing).

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 112 ++++++++++++++++++++++-----------
 1 file changed, 76 insertions(+), 36 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 4630b1b39..8a5075786 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1148,47 +1148,87 @@ impl DataStore {
             let namespace = namespace.context("iterating namespaces failed")?;
             for group in arc_self.iter_backup_groups(namespace)? {
                 let group = group.context("iterating backup groups failed")?;
-                let mut snapshots = group.list_backups().context("listing snapshots failed")?;
-                // Sort by snapshot timestamp to iterate over consecutive snapshots for each image.
-                BackupInfo::sort_list(&mut snapshots, true);
-                for snapshot in snapshots {
-                    for file in snapshot.files {
-                        worker.check_abort()?;
-                        worker.fail_on_shutdown()?;
-
-                        let mut path = snapshot.backup_dir.full_path();
-                        path.push(file);
-
-                        let index = match self.open_index_reader(&path)? {
-                            IndexReaderOption::Some(index) => index,
-                            IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
-                                unprocessed_index_list.remove(&path);
-                                continue;
+
+                // Avoid race between listing/marking of snapshots by GC and pruning the last
+                // snapshot in the group, following a new snapshot creation. Otherwise known chunks
+                // might only be referenced by the new snapshot, so it must be read as well.
+                let mut retry_counter = 0;
+                'retry: loop {
+                    let _lock = match retry_counter {
+                        0..=9 => None,
+                        10 => Some(
+                            group
+                                .lock()
+                                .context("exhausted retries and failed to lock group")?,
+                        ),
+                        _ => bail!("exhausted retires and unexpected counter overrun"),
+                    };
+
+                    let mut snapshots = match group.list_backups() {
+                        Ok(snapshots) => snapshots,
+                        Err(err) => {
+                            if group.exists() {
+                                return Err(err).context("listing snapshots failed")?;
                             }
-                        };
-                        self.index_mark_used_chunks(
-                            index,
-                            &path,
-                            &mut chunk_lru_cache,
-                            status,
-                            worker,
-                        )?;
-
-                        if !unprocessed_index_list.remove(&path) {
-                            info!("Encountered new index file '{path:?}', increment total index file count");
-                            index_count += 1;
+                            break 'retry;
                         }
+                    };
+
+                    // Always start iteration with the last snapshot of the group to reduce race
+                    // window with concurrent backup+prune previous last snapshot. Allows to retry
+                    // without the need to keep track of already processed index files for the
+                    // current group.
+                    BackupInfo::sort_list(&mut snapshots, true);
+                    for (count, snapshot) in snapshots.into_iter().rev().enumerate() {
+                        for file in snapshot.files {
+                            worker.check_abort()?;
+                            worker.fail_on_shutdown()?;
+
+                            let mut path = snapshot.backup_dir.full_path();
+                            path.push(file);
+
+                            let index = match self.open_index_reader(&path)? {
+                                IndexReaderOption::Some(index) => index,
+                                IndexReaderOption::NoneWrongType => {
+                                    unprocessed_index_list.remove(&path);
+                                    continue;
+                                }
+                                IndexReaderOption::NoneNotFound => {
+                                    if count == 0 {
+                                        retry_counter += 1;
+                                        continue 'retry;
+                                    }
+                                    unprocessed_index_list.remove(&path);
+                                    continue;
+                                }
+                            };
+
+                            self.index_mark_used_chunks(
+                                index,
+                                &path,
+                                &mut chunk_lru_cache,
+                                status,
+                                worker,
+                            )?;
+
+                            if !unprocessed_index_list.remove(&path) {
+                                info!("Encountered new index file '{path:?}', increment total index file count");
+                                index_count += 1;
+                            }
 
-                        let percentage = (processed_index_files + 1) * 100 / index_count;
-                        if percentage > last_percentage {
-                            info!(
-                                "marked {percentage}% ({} of {index_count} index files)",
-                                processed_index_files + 1,
-                            );
-                            last_percentage = percentage;
+                            let percentage = (processed_index_files + 1) * 100 / index_count;
+                            if percentage > last_percentage {
+                                info!(
+                                    "marked {percentage}% ({} of {index_count} index files)",
+                                    processed_index_files + 1,
+                                );
+                                last_percentage = percentage;
+                            }
+                            processed_index_files += 1;
                         }
-                        processed_index_files += 1;
                     }
+
+                    break;
                 }
             }
         }
-- 
2.39.5



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

Re: [pbs-devel] [PATCH v4 proxmox-backup 2/2] garbage collection: fix rare race in chunk marking phase

[Fabian Grünbichler]

On April 16, 2025 11:17 am, Christian Ebner wrote:
> During phase 1 of garbage collection referenced chunks are marked as
> in use by iterating over all index files and updating the atime on
> the chunks referenced by these.
> 
> In an edge case for long running garbage collection jobs, where a
> newly added snapshot (created after the start of GC) reused known
> chunks from a previous snapshot, but the previous snapshot index
> referencing them disappeared before the marking phase could reach
> that index (e.g. pruned because only 1 snapshot to be kept by
> retention setting), known chunks from that previous index file might
> not be marked (given that by none of the other index files it was
> marked).
> 
> Since commit 74361da8 ("garbage collection: generate index file list
> via datastore iterators") this is even less likely as now the
> iteration reads also index files added during phase 1, and
> therefore either the new or the previous index file will account for
> these chunks (the previous backup snapshot can only be prunded after
> the new one finished, since locked). There remains however a small
> race window between the reading of the snapshots in the backup group
> and the reading of the actual index files for marking.
> 
> Fix this race by:
> 1. Checking if the last snapshot of a group disappeared and if so
> 2. generate the list again, looking for new index files previously
>    not accounted for
> 3. To avoid possible endless looping, lock the group if the snapshot
>    list changed even after the 10th time (which will lead to
>    concurrent operations to this group failing).
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 112 ++++++++++++++++++++++-----------
>  1 file changed, 76 insertions(+), 36 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 4630b1b39..8a5075786 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1148,47 +1148,87 @@ impl DataStore {
>              let namespace = namespace.context("iterating namespaces failed")?;
>              for group in arc_self.iter_backup_groups(namespace)? {
>                  let group = group.context("iterating backup groups failed")?;
> -                let mut snapshots = group.list_backups().context("listing snapshots failed")?;
> -                // Sort by snapshot timestamp to iterate over consecutive snapshots for each image.
> -                BackupInfo::sort_list(&mut snapshots, true);
> -                for snapshot in snapshots {
> -                    for file in snapshot.files {
> -                        worker.check_abort()?;
> -                        worker.fail_on_shutdown()?;
> -
> -                        let mut path = snapshot.backup_dir.full_path();
> -                        path.push(file);
> -
> -                        let index = match self.open_index_reader(&path)? {
> -                            IndexReaderOption::Some(index) => index,
> -                            IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
> -                                unprocessed_index_list.remove(&path);
> -                                continue;
> +
> +                // Avoid race between listing/marking of snapshots by GC and pruning the last
> +                // snapshot in the group, following a new snapshot creation. Otherwise known chunks
> +                // might only be referenced by the new snapshot, so it must be read as well.
> +                let mut retry_counter = 0;
> +                'retry: loop {
> +                    let _lock = match retry_counter {
> +                        0..=9 => None,
> +                        10 => Some(
> +                            group
> +                                .lock()
> +                                .context("exhausted retries and failed to lock group")?,
> +                        ),
> +                        _ => bail!("exhausted retires and unexpected counter overrun"),

typo 'retires'

other than that and the comments on patch#1, this looks good to me, so
consider this

Acked-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> +                    };
> +
> +                    let mut snapshots = match group.list_backups() {
> +                        Ok(snapshots) => snapshots,
> +                        Err(err) => {
> +                            if group.exists() {
> +                                return Err(err).context("listing snapshots failed")?;
>                              }
> -                        };
> -                        self.index_mark_used_chunks(
> -                            index,
> -                            &path,
> -                            &mut chunk_lru_cache,
> -                            status,
> -                            worker,
> -                        )?;
> -
> -                        if !unprocessed_index_list.remove(&path) {
> -                            info!("Encountered new index file '{path:?}', increment total index file count");
> -                            index_count += 1;
> +                            break 'retry;
>                          }
> +                    };
> +
> +                    // Always start iteration with the last snapshot of the group to reduce race
> +                    // window with concurrent backup+prune previous last snapshot. Allows to retry
> +                    // without the need to keep track of already processed index files for the
> +                    // current group.
> +                    BackupInfo::sort_list(&mut snapshots, true);
> +                    for (count, snapshot) in snapshots.into_iter().rev().enumerate() {
> +                        for file in snapshot.files {
> +                            worker.check_abort()?;
> +                            worker.fail_on_shutdown()?;
> +
> +                            let mut path = snapshot.backup_dir.full_path();
> +                            path.push(file);
> +
> +                            let index = match self.open_index_reader(&path)? {
> +                                IndexReaderOption::Some(index) => index,
> +                                IndexReaderOption::NoneWrongType => {
> +                                    unprocessed_index_list.remove(&path);
> +                                    continue;
> +                                }
> +                                IndexReaderOption::NoneNotFound => {
> +                                    if count == 0 {
> +                                        retry_counter += 1;
> +                                        continue 'retry;
> +                                    }
> +                                    unprocessed_index_list.remove(&path);
> +                                    continue;
> +                                }
> +                            };
> +
> +                            self.index_mark_used_chunks(
> +                                index,
> +                                &path,
> +                                &mut chunk_lru_cache,
> +                                status,
> +                                worker,
> +                            )?;
> +
> +                            if !unprocessed_index_list.remove(&path) {
> +                                info!("Encountered new index file '{path:?}', increment total index file count");
> +                                index_count += 1;
> +                            }
>  
> -                        let percentage = (processed_index_files + 1) * 100 / index_count;
> -                        if percentage > last_percentage {
> -                            info!(
> -                                "marked {percentage}% ({} of {index_count} index files)",
> -                                processed_index_files + 1,
> -                            );
> -                            last_percentage = percentage;
> +                            let percentage = (processed_index_files + 1) * 100 / index_count;
> +                            if percentage > last_percentage {
> +                                info!(
> +                                    "marked {percentage}% ({} of {index_count} index files)",
> +                                    processed_index_files + 1,
> +                                );
> +                                last_percentage = percentage;
> +                            }
> +                            processed_index_files += 1;
>                          }
> -                        processed_index_files += 1;
>                      }
> +
> +                    break;
>                  }
>              }
>          }
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

Re: [pbs-devel] [PATCH v4 proxmox-backup 1/2] garbage collection: distinguish variants for failed open index reader

[Fabian Grünbichler]

On April 16, 2025 11:17 am, Christian Ebner wrote:
> In preparation for adapting the garbage collection to be able to
> distinguish cases for the index file not being accessible because it
> is no longer present or because it is a blob and therefore should be
> ignored.

this seems very involved..

> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index aa38e2ac1..4630b1b39 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1033,11 +1033,14 @@ impl DataStore {
>  
>      // Similar to open index, but ignore index files with blob or unknown archive type.
>      // Further, do not fail if file vanished.
> -    fn open_index_reader(&self, absolute_path: &Path) -> Result<Option<Box<dyn IndexFile>>, Error> {
> +    fn open_index_reader(
> +        &self,
> +        absolute_path: &Path,
> +    ) -> Result<IndexReaderOption<Box<dyn IndexFile>>, Error> {
>          let archive_type = match ArchiveType::from_path(absolute_path) {
> -            Ok(archive_type) => archive_type,
>              // ignore archives with unknown archive type
> -            Err(_) => return Ok(None),
> +            Ok(ArchiveType::Blob) | Err(_) => return Ok(IndexReaderOption::NoneWrongType),

or filter the snapshot.files by archive type first to only call this
with index paths? see below

the unprocessed_index_list never contains blobs anyway..

> +            Ok(archive_type) => archive_type,
>          };
>  
>          if absolute_path.is_relative() {
> @@ -1047,7 +1050,9 @@ impl DataStore {
>          let file = match std::fs::File::open(absolute_path) {
>              Ok(file) => file,
>              // ignore vanished files
> -            Err(err) if err.kind() == io::ErrorKind::NotFound => return Ok(None),
> +            Err(err) if err.kind() == io::ErrorKind::NotFound => {
> +                return Ok(IndexReaderOption::NoneNotFound)
> +            }
>              Err(err) => {
>                  return Err(Error::from(err).context(format!("can't open file '{absolute_path:?}'")))
>              }
> @@ -1057,14 +1062,14 @@ impl DataStore {
>              ArchiveType::FixedIndex => {
>                  let reader = FixedIndexReader::new(file)
>                      .with_context(|| format!("can't open fixed index '{absolute_path:?}'"))?;
> -                Ok(Some(Box::new(reader)))
> +                Ok(IndexReaderOption::Some(Box::new(reader)))
>              }
>              ArchiveType::DynamicIndex => {
>                  let reader = DynamicIndexReader::new(file)
>                      .with_context(|| format!("can't open dynamic index '{absolute_path:?}'"))?;
> -                Ok(Some(Box::new(reader)))
> +                Ok(IndexReaderOption::Some(Box::new(reader)))
>              }
> -            ArchiveType::Blob => Ok(None),
> +            ArchiveType::Blob => Ok(IndexReaderOption::NoneWrongType),
>          }
>      }
>  
> @@ -1155,8 +1160,8 @@ impl DataStore {
>                          path.push(file);
>  
>                          let index = match self.open_index_reader(&path)? {
> -                            Some(index) => index,
> -                            None => {
> +                            IndexReaderOption::Some(index) => index,
> +                            IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
>                                  unprocessed_index_list.remove(&path);
>                                  continue;
>                              }
> @@ -1191,8 +1196,8 @@ impl DataStore {
>          let mut strange_paths_count = unprocessed_index_list.len();
>          for path in unprocessed_index_list {
>              let index = match self.open_index_reader(&path)? {
> -                Some(index) => index,
> -                None => {
> +                IndexReaderOption::Some(index) => index,
> +                IndexReaderOption::NoneWrongType | IndexReaderOption::NoneNotFound => {
>                      // do not count vanished (pruned) backup snapshots as strange paths.
>                      strange_paths_count -= 1;
>                      continue;
> @@ -1695,3 +1700,9 @@ impl DataStore {
>          *OLD_LOCKING
>      }
>  }
> +
> +enum IndexReaderOption<T> {
> +    Some(T),
> +    NoneWrongType,
> +    NoneNotFound,
> +}

this is a NACK from me anyway, either this is an enum, then it should be
Index(T) / Blob / Vanished, or it is a custom error, then the Option
should be dropped and we have

Result<T, GCIndexError>

where GCIndexError has WrongType / NotFound / Other or something like
that, and the call site can choose to ignore NotFound and/or WrongType.

but I am not sure that is worth it for such an internal helper in any
case, we can just make blobs a hard error in the helper, and filter the
blobs out in the single code path where they might be included..

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


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

[pbs-devel] superseded: [PATCH v4 proxmox-backup 0/2] fix rare race in garbage collection

[Christian Ebner]

superseded-by version 5:
https://lore.proxmox.com/pbs-devel/20250416105000.270166-1-c.ebner@proxmox.com/T/


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