Re: [pbs-devel] [PATCH proxmox-backup] gc: attach context to index reader errors and ignore NotFound

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

On September 8, 2020 11:18 am, Stefan Reiter wrote:
> Ignore NotFound errors during phase 1, this just means that a snapshot
> was forgotten or pruned between scanning for .fidx/.didx files and
> actually opening the index to touch the chunks.

I originally had a similar patch already lying around, but I am not sure 
whether this is not too dangerous in the face of transient errors?

I'd much rather get to a point where we are sure that no concurrent 
prune/forget operation can happen, and treat all errors as errors, 
instead of treating all not found errors as benign 'must have happened 
cause of concurrent actions'.

this is not pull, or download/restore, where we can just retry later - 
if we skip the index here, all the chunks it referenced are up for 
garbage collection unless they are saved by another index!

> 
> ignore_notfound has to be a real function, since generics are not
> supported for closures.
> 
> The open methods for dynamic and fixed indices are switched from the
> usual format_err! to err.context() to allow checking for the root error
> (and thus the io::ErrorKind) further up the call chain.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  src/backup/datastore.rs     | 28 ++++++++++++++++++++++++----
>  src/backup/dynamic_index.rs |  5 ++++-
>  src/backup/fixed_index.rs   |  7 +++++--
>  3 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
> index ebe47487..1f455d64 100644
> --- a/src/backup/datastore.rs
> +++ b/src/backup/datastore.rs
> @@ -426,6 +426,20 @@ impl DataStore {
>          Ok(())
>      }
>  
> +    fn ignore_notfound<T>(res: Result<T, Error>) -> Result<Option<T>, Error> {
> +        match res {
> +            Ok(t) => Ok(Some(t)),
> +            Err(err) => {
> +                if let Some(ioerr) = err.downcast_ref::<std::io::Error>() {
> +                    if ioerr.kind() == std::io::ErrorKind::NotFound {
> +                        return Ok(None);
> +                    }
> +                }
> +                Err(err)
> +            }
> +        }
> +    }
> +
>      fn mark_used_chunks(&self, status: &mut GarbageCollectionStatus, worker: &WorkerTask) -> Result<(), Error> {
>  
>          let image_list = self.list_images()?;
> @@ -443,11 +457,17 @@ impl DataStore {
>  
>              if let Ok(archive_type) = archive_type(&path) {
>                  if archive_type == ArchiveType::FixedIndex {
> -                    let index = self.open_fixed_reader(&path)?;
> -                    self.index_mark_used_chunks(index, &path, status, worker)?;
> +                    if let Some(index) = Self::ignore_notfound(self.open_fixed_reader(&path))? {
> +                        self.index_mark_used_chunks(index, &path, status, worker)?;
> +                    } else {
> +                        worker.warn(format!("warning: could no longer find fixed index '{:?}'", &path));
> +                    }
>                  } else if archive_type == ArchiveType::DynamicIndex {
> -                    let index = self.open_dynamic_reader(&path)?;
> -                    self.index_mark_used_chunks(index, &path, status, worker)?;
> +                    if let Some(index) = Self::ignore_notfound(self.open_dynamic_reader(&path))? {
> +                        self.index_mark_used_chunks(index, &path, status, worker)?;
> +                    } else {
> +                        worker.warn(format!("warning: could no longer find dynamic index '{:?}'", &path));
> +                    }
>                  }
>              }
>              done += 1;
> diff --git a/src/backup/dynamic_index.rs b/src/backup/dynamic_index.rs
> index f70aa44f..a7ce0f24 100644
> --- a/src/backup/dynamic_index.rs
> +++ b/src/backup/dynamic_index.rs
> @@ -86,7 +86,10 @@ impl DynamicIndexReader {
>          File::open(path)
>              .map_err(Error::from)
>              .and_then(Self::new)
> -            .map_err(|err| format_err!("Unable to open dynamic index {:?} - {}", path, err))
> +            .map_err(|err| {
> +                let msg = format!("Unable to open dynamic index {:?} - {}", path, err);
> +                err.context(msg)
> +            })
>      }
>  
>      pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
> diff --git a/src/backup/fixed_index.rs b/src/backup/fixed_index.rs
> index 5d6cc1ff..bf864173 100644
> --- a/src/backup/fixed_index.rs
> +++ b/src/backup/fixed_index.rs
> @@ -1,4 +1,4 @@
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, Error};
>  use std::io::{Seek, SeekFrom};
>  
>  use super::chunk_stat::*;
> @@ -62,7 +62,10 @@ impl FixedIndexReader {
>          File::open(path)
>              .map_err(Error::from)
>              .and_then(|file| Self::new(file))
> -            .map_err(|err| format_err!("Unable to open fixed index {:?} - {}", path, err))
> +            .map_err(|err| {
> +                let msg = format!("Unable to open fixed index {:?} - {}", path, err);
> +                err.context(msg)
> +            })
>      }
>  
>      pub fn new(mut file: std::fs::File) -> Result<Self, Error> {
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>