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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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